Warning

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

@jdm This introduces the core functionality that allows iframes to generate their own pipeline IDs. This is the first step towards allowing us to simplify all management of SubpageIds. This should be a much simpler PR to review than doing the entire change at once. r?

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

^{components/compositing/constellation.rs, line 336 [r1] (raw file):}
I'm finding the following difficult to reason about:

• the script thread for a pipeline in namespace 1 with index 1 is created
• it starts loading a URL in an iframe, sending an ID with namespace 1 with index 2 to the constellation
• the constellation creates a new pipeline with ID of namespace 1 and index 2, but the contained pipeline_namespace_id in PipelineContent is namespace 2
• when the new pipeine creates a new script thread, any iframes created in that script thread will have IDs of namespace 2, index N?

I guess I haven't found anything that seems wrong, per se, but the setup is feels less intuitive than it was before. I fear what will happen when we start grouping pipelines in the same script task by origin, even for simple setups like clicking a link on a.com that goes to a.com/page and uses the same script thread for the load.

^{components/compositing/constellation.rs, line 620 [r1] (raw file):}
The interaction between this and initializing the next namespace as 1 in the constructor is extremely subtle. Remind me why we can't use new here instead?

^{components/devtools/lib.rs, line 436 [r1] (raw file):}
If this ends up being the only legitimate use for PipelineId::nil, it should probably be called fake_me_a_pipeline instead.

^{components/gfx/paint_task.rs, line 186 [r1] (raw file):}
Same note as further down.

^{components/layout/layout_task.rs, line 280 [r1] (raw file):}
Same note as further down.

^{components/msg/constellation_msg.rs, line 226 [r1] (raw file):}
We're going to need to either turn this into a struct variant or a simple struct wrapper and provide meaningful names.

^{components/msg/constellation_msg.rs, line 397 [r1] (raw file):}
The similarity between PipelineIdNamespace and PipelineNamespaceId is unfortunate. What about PipelineNamespace? Documentation of the relationship between pipeline IDs and namespaces would also be lovely.

^{components/msg/constellation_msg.rs, line 428 [r1] (raw file):}
Is it possible to use Cell instead?

^{components/msg/constellation_msg.rs, line 451 [r1] (raw file):}
I'd love to get rid of this method if we can; I find its existence very confusing. At minimum we should clearly document when it's appropriate to use.

^{components/script/dom/htmliframeelement.rs, line 123 [r1] (raw file):}
Let's use a local binding to the new pipeline ID instead of using unwrap here.

^{components/script/script_task.rs, line 496 [r1] (raw file):}
We should have a custom representation like (namespace,index); I assume the actual output we get here right now will be less than optimal.

Comments from the review on Reviewable.io

I wonder if this fixes #7826.

Review status: 3 of 9 files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

^{components/compositing/constellation.rs, line 336 [r1] (raw file):}
Does it help to think about it as the namespace ID being an identifier for the thread that created the pipeline ID? It is less intuitive, I agree - but I feel that it's a worthwhile tradeoff for the benefits (we will be able to remove all subpage concepts from the compositor, layout, script task etc).

^{components/compositing/constellation.rs, line 620 [r1] (raw file):}
Only because of the code in devtools you noted below - I will ping @jgragam to find out what we can do with that.

^{components/devtools/lib.rs, line 0 [r1] (raw file):}
Agreed - I'll discuss with jgraham

^{components/gfx/paint_task.rs, line 0 [r1] (raw file):}
Done.

^{components/layout/layout_task.rs, line 0 [r1] (raw file):}
Done.

^{components/msg/constellation_msg.rs, line 0 [r1] (raw file):}
Done.

^{components/msg/constellation_msg.rs, line 0 [r1] (raw file):}
I have added a TODO comment for now.

^{components/msg/constellation_msg.rs, line 0 [r1] (raw file):}
Done.

^{components/msg/constellation_msg.rs, line 0 [r1] (raw file):}
This comment seems to be about a line not included in this PR?

^{components/script/dom/htmliframeelement.rs, line 0 [r1] (raw file):}
Done.

^{components/script/script_task.rs, line 0 [r1] (raw file):}
Done.

Comments from the review on Reviewable.io

-S-awaiting-review +S-needs-code-changes -S-awaiting-answer

Reviewed 6 of 6 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

^{components/compositing/constellation.rs, line 336 [r1] (raw file):}
Actually, when you phrase it like that it makes a lot more sense to me. Let's make sure to document that somewhere :)

^{components/msg/constellation_msg.rs, line 226 [r1] (raw file):}
Not if you look at the original context by dragging r1->r2 in reviewable.

^{components/msg/constellation_msg.rs, line 397 [r1] (raw file):}
Can we turn it into doc comments? Otherwise it won't show up on doc.servo.org.

^{components/msg/constellation_msg.rs, line 406 [r2] (raw file):}
I first read this as script threads and constellations create namespaces willy-nilly, but we should be clear that only the constellation creates new namespaces so there's no danger of collisions.

Comments from the review on Reviewable.io

Review status: 5 of 9 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

^{components/compositing/constellation.rs, line 0 [r1] (raw file):}
Done.

^{components/msg/constellation_msg.rs, line 226 [r1] (raw file):}
Done.

^{components/msg/constellation_msg.rs, line 396 [r2] (raw file):}
Done.

^{components/msg/constellation_msg.rs, line 397 [r1] (raw file):}
Done.

Comments from the review on Reviewable.io

Looks good! Squash and merge!
-S-awaiting-review +S-needs-squash

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from the review on Reviewable.io

@bors-servo r=jdm

📌 Commit 5645dba has been approved by jdm

⌛ Testing commit 5645dba with merge 098bdb5...

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt