Record the frame type (IFrame or MozBrowserIFrame) in the pipeline.

[asajeffrey]

Thank you for contributing to Servo! Please replace each [ ] by [X] when the step is complete, and replace __ with appropriate data:

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes do not require tests because this is a refactoring

Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process.

This is a first step towards supporting the notion of multiple top-level browsing contexts in Servo, by making the constellation aware of which content is loaded in a mozbrowser iframe.

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @asajeffrey: components/constellation/pipeline.rs, components/constellation/constellation.rs
• @KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/bindings/trace.rs, components/script_traits/lib.rs, components/script_traits/lib.rs

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[asajeffrey]

cc @ConnorGBrewster

[KiChjang]

@bors-servo delegate=ConnorGBrewster

r? @ConnorGBrewster

[bors-servo]

✌️ @ConnorGBrewster can now approve this pull request

[cbrewster]

I think these changes look good! Just a few nits with some of the comments.

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

Previously, bors-servo wrote…

✌️ @ConnorGBrewster can now approve this pull request

---

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

components/constellation/constellation.rs, line 1844 [r1] (raw file):

                return true;
            }
            // TODO: terminate the search when we hit a mozbrowser iframe?

I would think you would terminate the search when hitting a mozbrowser iframe, but I am not 100% sure.

---

components/constellation/constellation.rs, line 2030 [r1] (raw file):

                    None => return None,
                },
                None => return None,

Should the warn! be kept here?

---

components/constellation/constellation.rs, line 2036 [r1] (raw file):

    // https://developer.mozilla.org/en-US/docs/Web/Events/mozbrowserlocationchange
    // Note that this is a no-op if the pipeline is not an immediate child iframe of a mozbrowser iframe

Maybe this should say // Note that this is a no-op if the pipeline is not a mozbrowser iframe

---

components/constellation/constellation.rs, line 2046 [r1] (raw file):

        });

        // If this is an iframe, then send the event with new url

Could we update this comment to say If this is a mozbrowser iframe, then send the event with new url

---

Comments from Reviewable

[highfive]

New code was committed to pull request.

[asajeffrey]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

components/constellation/constellation.rs, line 1844 [r1] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

I would think you would terminate the search when hitting a mozbrowser iframe, but I am not 100% sure.

Done.

---

components/constellation/constellation.rs, line 2030 [r1] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

Should the warn! be kept here?

Done.

---

components/constellation/constellation.rs, line 2036 [r1] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

Maybe this should say // Note that this is a no-op if the pipeline is not a mozbrowser iframe

Done.

---

components/constellation/constellation.rs, line 2046 [r1] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

Could we update this comment to say If this is a mozbrowser iframe, then send the event with new url

Done.

---

Comments from Reviewable

[cbrewster]

r=me after the TODO comment is removed and this is squashed

-S-awaiting-review

Previously, highfive wrote…

New code was committed to pull request.

---

Review status: 5 of 6 files reviewed at latest revision, 5 unresolved discussions.

---

components/script/dom/htmliframeelement.rs, line 449 [r2] (raw file):

    fn Mozbrowser(&self) -> bool {
        // We don't want to allow mozbrowser iframes within iframes
        // TODO: why not?

Remove comment?

---

Comments from Reviewable

[asajeffrey]

Review status: 5 of 6 files reviewed at latest revision, 5 unresolved discussions.

---

components/script/dom/htmliframeelement.rs, line 449 [r2] (raw file):

Previously, ConnorGBrewster (Connor Brewster) wrote…

Remove comment?

Well, I still don't understand why not, but done.

---

Comments from Reviewable

[highfive]

New code was committed to pull request.

[asajeffrey]

Fixed and squashed. @bors-servo: r=ConnorGBrewster

[bors-servo]

📌 Commit d92dfe1 has been approved by ConnorGBrewster

[bors-servo]

⌛ Testing commit d92dfe1 with merge 64cca22...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows