Script lookup iframes by frameid #13646

asajeffrey · 2016-10-07T21:55:11Z

Lookup iframes by FrameId rather than PipelineId. This should make lookup much more reliable, since the frame id doesn't change.

cc @ConnorGBrewster @aneeshusa

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

This change is

highfive · 2016-10-07T21:55:15Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script_traits/lib.rs, components/script_traits/lib.rs

highfive · 2016-10-07T21:55:16Z

Warning

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

asajeffrey · 2016-10-07T21:56:46Z

The first commit in this PR is actually #13627, we shouldn't merge this one until it lands.

bors-servo · 2016-10-10T20:52:12Z

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

bors-servo · 2016-10-13T15:40:47Z

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

asajeffrey · 2016-10-13T22:32:58Z

Rebased.

nox

Just a question, otherwise r=me.

nox · 2016-10-17T07:50:50Z

components/script_traits/script_msg.rs

@@ -85,7 +85,7 @@ pub enum ScriptMsg {
    LoadUrl(PipelineId, LoadData, bool),
    /// Dispatch a mozbrowser event to a given iframe,
    /// or to the window if no subpage id is provided.
-    /// First PipelineId is for the parent, second PipelineId is for the actual pipeline.
+    /// First PipelineId is for the parent, second PipelineId is in the frame.


What does it mean that it is "in the frame"? It still lives here so it's a but confusing.

Good point, that second pipeline id should be a frame id.

asajeffrey · 2016-10-21T01:52:32Z

Working on this, turns out there were bugs, sigh.

bors-servo · 2016-10-21T13:35:20Z

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

jdm · 2016-10-24T18:37:25Z

I haven't done more than skim recent changes in this PR, but I'm concerned about the changes to how the document load event is initiated. The old design was that it was fine to optimistically enqueue a "I think the document is finished loading everything" event, because when being run it would first check if that were actually the case. Some of the changes and comments around them make me worried that this is no longer true.

asajeffrey · 2016-10-24T19:14:48Z

@jdm yes, this is what I've been spending my time debugging. I now understand better what the document loading code is doing, I even wrote some comments so that future me might remember this. At least the wpt test suite caught this.

asajeffrey · 2016-10-28T18:20:50Z

This PR triggered issue #13964, fixed by #13965, which should land before this does.


        Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=<…

@jdm

…try> Script thread with no root document  This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633. The last commit is the one that matters, the ones before are #13646. cc @jdm @Ms2ger @ConnorGBrewster ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14013)

asajeffrey · 2016-11-02T20:54:07Z

Rebased. @bors-servo try

bors-servo · 2016-11-02T20:54:10Z

⌛ Trying commit e886199 with merge ac21b50...


        Auto merge of #13646 - asajeffrey:script-lookup-iframes-by-frameid, r…

@aneeshusa

…=<try> Script lookup iframes by frameid  Lookup iframes by `FrameId` rather than `PipelineId`. This should make lookup much more reliable, since the frame id doesn't change. cc @ConnorGBrewster @aneeshusa ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13646)

bors-servo · 2016-11-02T22:09:55Z

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


        Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=<…

@jdm

…try> Script thread with no root document  This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633. The last commit is the one that matters, the ones before are #13646. cc @jdm @Ms2ger @ConnorGBrewster ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14013)


        Lookup frames by frame_id, not pipeline_id.

asajeffrey · 2016-11-03T20:42:22Z

Squashed and rebased @bors-servo r=ConnorGBrewster

bors-servo · 2016-11-03T20:42:23Z

📌 Commit f3993d9 has been approved by ConnorGBrewster


        Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=<…

@jdm

…try> Script thread with no root document  This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633. The last commit is the one that matters, the ones before are #13646. cc @jdm @Ms2ger @ConnorGBrewster ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14013)

bors-servo · 2016-11-03T23:11:06Z

⌛ Testing commit f3993d9 with merge ce725c9...


        Auto merge of #13646 - asajeffrey:script-lookup-iframes-by-frameid, r…

@aneeshusa

…=ConnorGBrewster Script lookup iframes by frameid  Lookup iframes by `FrameId` rather than `PipelineId`. This should make lookup much more reliable, since the frame id doesn't change. cc @ConnorGBrewster @aneeshusa ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13646)

bors-servo · 2016-11-04T00:09:52Z

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


        Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=jdm

@jdm

Script thread with no root document  This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633. The last commit is the one that matters, the ones before are #13646. cc @jdm @Ms2ger @ConnorGBrewster ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14013)


        Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=jdm

@jdm

Script thread with no root document  This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633. The last commit is the one that matters, the ones before are #13646. cc @jdm @Ms2ger @ConnorGBrewster ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14013)

asajeffrey added I-refactor A-content/script A-constellation labels Oct 7, 2016

highfive assigned nox Oct 7, 2016

highfive added the S-awaiting-review label Oct 7, 2016

highfive added the S-needs-rebase label Oct 10, 2016

asajeffrey force-pushed the asajeffrey:script-lookup-iframes-by-frameid branch from b1d34f7 to 7948fd5 Oct 11, 2016

highfive added the S-needs-rebase label Oct 13, 2016

asajeffrey force-pushed the asajeffrey:script-lookup-iframes-by-frameid branch from 7948fd5 to 94c7391 Oct 13, 2016

KiChjang removed the S-needs-rebase label Oct 13, 2016

nox reviewed Oct 17, 2016

View changes

nox added S-awaiting-answer and removed S-awaiting-review labels Oct 17, 2016

asajeffrey force-pushed the asajeffrey:script-lookup-iframes-by-frameid branch from 94c7391 to bc290df Oct 21, 2016

highfive added the S-awaiting-review label Oct 21, 2016

highfive added the S-needs-rebase label Oct 21, 2016

asajeffrey force-pushed the asajeffrey:script-lookup-iframes-by-frameid branch 3 times, most recently from 5129976 to 9631237 Oct 24, 2016

asajeffrey force-pushed the asajeffrey:script-lookup-iframes-by-frameid branch from 7ecee09 to e886199 Nov 2, 2016

asajeffrey added S-needs-squash and removed S-needs-rebase labels Nov 2, 2016

asajeffrey removed the S-awaiting-review label Nov 3, 2016

Lookup frames by frame_id, not pipeline_id.

Loading status checks…

f3993d9

asajeffrey force-pushed the asajeffrey:script-lookup-iframes-by-frameid branch from e886199 to f3993d9 Nov 3, 2016

highfive added the S-awaiting-review label Nov 3, 2016

highfive assigned cbrewster and unassigned nox Nov 3, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Nov 3, 2016

bors-servo merged commit f3993d9 into servo:master Nov 4, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 4, 2016

This was referenced Nov 4, 2016

Storage notifications routed via the constellation. #14023

Merged

[WIP] Implement History State #13781

Closed

servo / servo

Script lookup iframes by frameid #13646

Script lookup iframes by frameid #13646

asajeffrey commented Oct 7, 2016 •

edited by larsbergstrom

highfive commented Oct 7, 2016

highfive commented Oct 7, 2016

asajeffrey commented Oct 7, 2016

bors-servo commented Oct 10, 2016

bors-servo commented Oct 13, 2016

asajeffrey commented Oct 13, 2016

nox left a comment

This comment has been minimized.

This comment has been minimized.

asajeffrey commented Oct 21, 2016

bors-servo commented Oct 21, 2016

jdm commented Oct 24, 2016

asajeffrey commented Oct 24, 2016

asajeffrey commented Oct 28, 2016

asajeffrey commented Nov 2, 2016

bors-servo commented Nov 2, 2016

bors-servo commented Nov 2, 2016

asajeffrey commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 4, 2016

servo / servo

Join GitHub today

Script lookup iframes by frameid #13646

Script lookup iframes by frameid #13646

Conversation

asajeffrey commented Oct 7, 2016 • edited by larsbergstrom

highfive commented Oct 7, 2016

highfive commented Oct 7, 2016

asajeffrey commented Oct 7, 2016

bors-servo commented Oct 10, 2016

bors-servo commented Oct 13, 2016

asajeffrey commented Oct 13, 2016

nox left a comment

This comment has been minimized.

nox Oct 17, 2016

This comment has been minimized.

asajeffrey Oct 17, 2016

asajeffrey commented Oct 21, 2016

bors-servo commented Oct 21, 2016

jdm commented Oct 24, 2016

asajeffrey commented Oct 24, 2016

asajeffrey commented Oct 28, 2016

asajeffrey commented Nov 2, 2016

bors-servo commented Nov 2, 2016

bors-servo commented Nov 2, 2016

asajeffrey commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 3, 2016

bors-servo commented Nov 4, 2016

asajeffrey commented Oct 7, 2016 •

edited by larsbergstrom