Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some sort of navigation-params container to stop threading stuff through navigation-related algorithms #5727

Closed
domenic opened this issue Jul 10, 2020 · 3 comments · Fixed by #5736
Labels
clarification Standard could be clearer topic: navigation

Comments

@domenic
Copy link
Member

domenic commented Jul 10, 2020

We currently have a serious problem with threading things through the navigate algorithms, and I would love some sort of container to make it better. This has come up a lot with COOP, and e.g. #5518 runs into it as well.

Concretely, a bunch of interesting setup stuff happens in create and initialize a Document object: it creates some combination of browsing context group, browsing context, agent cluster, agent, realm/Window, and Document. To do this, it needs the following data:

  • document type (XML or HTML)
  • content type
  • request
  • response
  • browsingContext (which one? not immediately clear from the algorithm... sometimes it gets overridden)
  • sandboxing flag set
  • origin (not clear precisely which one)
  • incumbent navigation origin
  • active document navigation origin
  • reserved environment (can be null)
  • cross-origin opener policy
  • browsing-context switch needed boolean

These values then need to be threaded through many places:

  • They usually originate in the main navigate algorithm
  • They end in either "process a navigate fetch" or directly in "process a navigate response"
  • They then get passed through, unchanged, to 6x "page load processing model for X" algorithms
  • Those algorithms finally call "create and initialize a Document object"

I think we should create some sort of container struct for these values, which is created in the main navigate algorithm, might get mutated a bit in navigate/process a navigate fetch/process a navigate response, and then gets passed through unperturbed to "page load processing model for X" and "create and initialize a Document".

This should be significantly clearer, and much more maintainable. The cost will be some rebasing for #5518 (/cc @camillelamy), but I can probably help with that.

@domenic domenic added clarification Standard could be clearer topic: navigation labels Jul 10, 2020
@jugglinmike
Copy link
Contributor

Introducing a struct like that would also make the algorithms' descriptions a little more vague. Instead of a precise list of each algorithm's dependencies, we'll have a general sense of what each might reference. Kind of like add(x, y) versus add(objectWithFifteenProperties).

The "passed through unperturbed" part is really important because it gives readers confidence that values aren't being switched out from the call site. We get that "for free" when invoking algorithms with explicit parameter values. Would we explicitly communicate the invariant whenever it held for the new struct?

As a drive-by contributor, I'm definitely more tolerant of the maintenance burden imposed on full-time editors, so take this with a grain of salt. I wanted to at least recognize these drawbacks to fill out the discussion.

@domenic
Copy link
Member Author

domenic commented Jul 14, 2020

Interestingly at this point "create and initialize a Document object" no longer uses incumbentNavigationOrigin and activeDocumentNavigationOrigin; those are threaded through several levels for no benefit. Some refactoring must have used them up earlier.

domenic added a commit that referenced this issue Jul 14, 2020
Closes #5727. Along the way this includes some minor fixes and cleanups
which this refactoring made more obvious or necessary:

* Stops unnecessarily passing along the incumbent navigation origin and
  active document navigation origin. They are used early in the
  navigation process and don't need to be threaded through the later
  algorithms (and thus aren't included in the navigation params struct).

* Ensures that all error cases (i.e. "Page load processing model for
  inline content that doesn't have a DOM") result in an opaque-origin
  Document. Previously only one call site set that, and did so
  awkwardly, from a distance.

* Fixes error cases to use "unsafe-none" COOP, instead of null COOP
  (which is not a thing).

* Explicitly passes the browsing context to "Page load processing model
  for inline content that doesn't have a DOM" (which makes it more clear
  that the browsing context is the only input; the navigation params are
  not used in this case).

* Adds two XXX boxes where the current architecture is not quite right:
  multipart/x-mixed-replace needs to use the navigation params somehow,
  and error pages probably need to synthesize a response instead of
  passing null to "create and initialize a Document object".

* Minor algorithm flow cleanups, e.g. using numbered steps instead of
  long sentences with several "and"s.
@camillelamy
Copy link
Member

It would be helpful indeed! When writing the COOP spec PR it was hard to keep track of all the places I was supposed to add/pass a parameter. In fact, it's probably this PR which removed usage of incumbentNavigationOrigin and activeDocumentNavigationOrigin inside "create and initialize a Document object" but missed the fact that all those call sites needed to be updated.

domenic added a commit that referenced this issue Jul 15, 2020
Closes #5727. Along the way this includes some minor fixes and cleanups
which this refactoring made more obvious or necessary:

* Stops unnecessarily passing along the incumbent navigation origin and
  active document navigation origin. They are used early in the
  navigation process and don't need to be threaded through the later
  algorithms (and thus aren't included in the navigation params struct).

* Ensures that all error cases (i.e. "Page load processing model for
  inline content that doesn't have a DOM") result in an opaque-origin
  Document. Previously only one call site set that, and did so
  awkwardly, from a distance.

* Fixes error cases to use "unsafe-none" COOP, instead of null COOP
  (which is not a thing).

* Explicitly passes the browsing context to "Page load processing model
  for inline content that doesn't have a DOM" (which makes it more clear
  that the browsing context is the only input; the navigation params are
  not used in this case).

* Adds two XXX boxes where the current architecture is not quite right:
  multipart/x-mixed-replace needs to use the navigation params somehow,
  and error pages probably need to synthesize a response instead of
  passing null to "create and initialize a Document object".

* Minor algorithm flow cleanups, e.g. using numbered steps instead of
  long sentences with several "and"s.
domenic added a commit that referenced this issue Jul 15, 2020
Closes #5727. Along the way this includes some minor fixes and cleanups
which this refactoring made more obvious or necessary:

* Stops unnecessarily passing along the incumbent navigation origin and
  active document navigation origin. They are used early in the
  navigation process and don't need to be threaded through the later
  algorithms (and thus aren't included in the navigation params struct).

* Ensures that all error cases (i.e. "Page load processing model for
  inline content that doesn't have a DOM") result in an opaque-origin
  Document. Previously only one call site set that, and did so
  awkwardly, from a distance.

* Fixes error cases to use "unsafe-none" COOP, instead of null COOP
  (which is not a thing).

* Explicitly passes the browsing context to "Page load processing model
  for inline content that doesn't have a DOM" (which makes it more clear
  that the browsing context is the only input; the navigation params are
  not used in this case).

* Adds two XXX boxes where the current architecture is not quite right:
  multipart/x-mixed-replace needs to use the navigation params somehow,
  and error pages probably need to synthesize a response instead of
  passing null to "create and initialize a Document object".

* Minor algorithm flow cleanups, e.g. using numbered steps instead of
  long sentences with several "and"s.
domenic added a commit that referenced this issue Jul 17, 2020
Closes #5727. Along the way this includes some minor fixes and cleanups
which this refactoring made more obvious or necessary:

* Stops unnecessarily passing along the incumbent navigation origin and
  active document navigation origin. They are used early in the
  navigation process and don't need to be threaded through the later
  algorithms (and thus aren't included in the navigation params struct).

* Ensures that all error cases (i.e. "Page load processing model for
  inline content that doesn't have a DOM") result in an opaque-origin
  Document. Previously only one call site set that, and did so
  awkwardly, from a distance.

* Fixes error cases to use "unsafe-none" COOP, instead of null COOP
  (which is not a thing).

* Explicitly passes the browsing context to "Page load processing model
  for inline content that doesn't have a DOM" (which makes it more clear
  that the browsing context is the only input; the navigation params are
  not used in this case).

* Adds two XXX boxes where the current architecture is not quite right:
  multipart/x-mixed-replace needs to use the navigation params somehow,
  and error pages probably need to synthesize a response instead of
  passing null to "create and initialize a Document object".

* Minor algorithm flow cleanups, e.g. using numbered steps instead of
  long sentences with several "and"s.
mfreed7 pushed a commit to mfreed7/html that referenced this issue Sep 11, 2020
Closes whatwg#5727. Along the way this includes some minor fixes and cleanups
which this refactoring made more obvious or necessary:

* Stops unnecessarily passing along the incumbent navigation origin and
  active document navigation origin. They are used early in the
  navigation process and don't need to be threaded through the later
  algorithms (and thus aren't included in the navigation params struct).

* Ensures that all error cases (i.e. "Page load processing model for
  inline content that doesn't have a DOM") result in an opaque-origin
  Document. Previously only one call site set that, and did so
  awkwardly, from a distance.

* Fixes error cases to use "unsafe-none" COOP, instead of null COOP
  (which is not a thing).

* Explicitly passes the browsing context to "Page load processing model
  for inline content that doesn't have a DOM" (which makes it more clear
  that the browsing context is the only input; the navigation params are
  not used in this case).

* Adds two XXX boxes where the current architecture is not quite right:
  multipart/x-mixed-replace needs to use the navigation params somehow,
  and error pages probably need to synthesize a response instead of
  passing null to "create and initialize a Document object".

* Minor algorithm flow cleanups, e.g. using numbered steps instead of
  long sentences with several "and"s.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Standard could be clearer topic: navigation
Development

Successfully merging a pull request may close this issue.

3 participants