Warning

• These commits modify unsafe code. Please review it carefully!
• These commits modify layout code, but no reftests are modified. Please consider adding a reftest!

@bors-servo: try

⌛ Trying commit 3f49255 with merge a88898e...

💔 Test failed - gonk

@bors-servo: try

⌛ Trying commit 64be00d with merge 03b3a95...

💔 Test failed - gonk

  • 14 tests crashed unexpectedly
  • 7 tests timed out unexpectedly
  • 16 tests unexpectedly okay
  • 30 tests had unexpected subtest results

Exciting!

☔ The latest upstream changes (presumably #8599) made this pull request unmergeable. Please resolve the merge conflicts.

☔ The latest upstream changes (presumably #8622) made this pull request unmergeable. Please resolve the merge conflicts.

Reviewed 3 of 3 files at r1, 2 of 2 files at r2, 48 of 48 files at r3.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

components/script/dom/browsercontext.rs, line 117 [r3] (raw file):
Can't we just make this MaybeRemoteRef<T> and MaybeRemoteField<T>? Less dupilication that way

components/script/dom/browsercontext.rs, line 119 [r3] (raw file):
I don't see this being used in this PR. What's this going to be used for?

components/script/dom/dedicatedworkerglobalscope.rs, line 99 [r1] (raw file):
I think we can just stick try! here? Not necessary though.

components/script/dom/htmliframeelement.rs, line 506 [r2] (raw file):
nit: spec link (https://html.spec.whatwg.org/multipage/#iframe-load-event-steps)

components/script/dom/htmliframeelement.rs, line 71 [r3] (raw file):
❤️

components/script/script_task.rs, line 600 [r3] (raw file):
Should have documentation that mentions that it can only be called from the script thread.

(Most other script-thread-only functions require you to have an existing JS object with you, which is hard to get off-thread. This one only needs a PipelineId, which can be found on any thread.)

tests/wpt/metadata/dom/nodes/Element-matches.html.ini, line 58 [r3] (raw file):
?

tests/wpt/metadata/dom/nodes/ParentNode-querySelector-All.html.ini, line 250 [r3] (raw file):
?

tests/wpt/metadata/dom/ranges/Range-surroundContents.html.ini, line 0 [r3] (raw file):
Ditto

tests/wpt/metadata/html/browsers/browsing-the-web/read-text/load-text-plain.html.ini, line 8 [r3] (raw file):
Ditto

Comments from the review on Reviewable.io

Design looks good to me; though Ms2ger should still have a look at it IMO.

Idle musing, not to be taken as a suggestion that I understand the design yet: should as_local return an Option?

I don't think so. It's meant as a hack right now to make it clear what the choke points in the API are.

☔ The latest upstream changes (presumably #13227) made this pull request unmergeable. Please resolve the merge conflicts.

From 9/19 in #servo:

08:08 Ms2ger: jdm, I had another look at your about:blank PR, and tried to rewrite it without the sync sender dance, but got stuck on the PaintThread
08:09 * jdm wonders how PaintThread is involved
08:09 Ms2ger: jdm, though hopefully wr will land soon, and the paint thread will be gone...
08:09 jdm: ooh, interesting point
08:09 Ms2ger: jdm, I'm trying to create the layout thread without talking to the constellation
08:10 jdm: right, that sounds familiar
08:10 Ms2ger: So I'm hoping we could make it work once wr lands

Webrender has landed, but I don't think we've gotten rid of the non-WR code yet. It is not clear to me how valuable it is to spend time reviewing these changes until we get a chance to see how to post-webrender world affects the design space.

The code I was working on is in https://github.com/Ms2ger/servo/tree/my-loadexperiments

I'm getting confused why the nested event loop in start_page_load is necessary; I'll try to look at that soonish.

Do you mean start_page_load_about_blank? That's necessary to process the network notifications for fetching about:blank, right?

Redesigned in #13996.