LibWeb: Navigables #18219
Merged
LibWeb: Navigables #18219
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
👀 pr-needs-review
github-actions
bot
removed
the
👀 pr-needs-review
kalenikaliaksandr
force-pushed
the
navigable
branch
14 times, most recently
from
c4f5db1
to
4e287cd
Compare
kalenikaliaksandr
force-pushed
the
navigable
branch
2 times, most recently
from
066e071
to
2ca3afe
Compare
kalenikaliaksandr
force-pushed
the
navigable
branch
from
2ca3afe
to
c673e19
Compare
This was referenced Apr 19, 2023
kalenikaliaksandr
force-pushed
the
navigable
branch
from
c673e19
to
c17ca54
Compare
This was referenced Apr 22, 2023
kalenikaliaksandr
force-pushed
the
navigable
branch
2 times, most recently
from
228bfed
to
57ca5ce
Compare
Was replaced by close_top_level_traversable()
Those are not used anymore after moving to navigables.
Reported issue in the spec whatwg/html#9686
During the destruction of a navigable, we need to use the pointer to the navigable that was saved at the beginning of the function. This is because `Node::navigable()` will return a nullptr in subsequent steps after the navigable's document becomes inactive.
This is not in the spec, but we need to make sure that "apply the history step" for initial navigation to about:blank in iframe is applied before subsequent navigations. Otherwise, "set ongoing navigation" call during "about:blank" traversal might abort subsequent ongoing navigation which is not expected to happen.
This fixes issue reproducing with following steps: 1. Node::insert_before() adopts a node into another document. 2. Node::insert_before() runs insertion steps for adopted node (adopted node style is not invalidated yet). 3. Insertion steps execute spin_until() on event loop. 4. The next task on event loop does Document::update_style() which requires layout tree rebuild. 5. Layout tree rebuild fails because there is a node with invalidated style.
Those are superseded by methods to navigate `javascript:` url in navigables.
Adding this check was a mistake because although the navigation id changes to null in step 2, it still has to proceed and apply the history step.
If `load_document()` is called with a response that has a mime type we can't use to build a document, we should return nullptr as the spec says, instead of crashing. Also we should not crash if error happened during parsing.
kalenikaliaksandr
force-pushed
the
navigable
branch
from
53802be
to
779ad23
Compare
|
Okay, I've tested this quite a bit and it seems stable enough that we can continue iterating in tree. Nice work! :^) |
github-actions
bot
removed
the
👀 pr-needs-review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces all instances of FrameLoader with navigables navigation, which uses Fetch under the hood, in the following places:
I also attempted to remove code that is no longer used after this transition, but there may still be something left.
In addition to using Fetch in navigation, another notable outcome of implementing navigables is that now all traversal history is centralized and managed by traversables. Although, with this PR, history still lives on the Chrome side, it will be possible to modify that by introducing new IPC calls for page reloading and history traversal.
Correctness progression I am aware of:
srcdociframe attribute supportjavasript:urls can navigate to a new documentCollateral damage I am aware of:
RemoteBrowsingContextis gone becauseBrowsingContextdoes not have navigate method anymore so it does not make sense. This breaks (?) navigation from WebDriver APIs. We will have to fix that in the future by introducingRemoveNavigableor inventing better abstraction for that.