Excise SubpageId and use only PipelineIds by aneeshusa · Pull Request #11698 · servo/servo

aneeshusa · 2016-06-10T07:33:03Z

SubpageId was originally introduced in 2013 to help iframes keep track of
their associated (children) pipelines. However, since each pipeline
already has a PipelineId, and those are unique, those are sufficient
to keep track of children.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Remove SubpageId #11694 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because refactoring

This change is

highfive · 2016-06-10T07:33:06Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/pipeline.rs, components/webdriver_server/lib.rs, components/constellation/constellation.rs
@jgraham: components/webdriver_server/lib.rs
@KiChjang: components/script/dom/bindings/global.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/servohtmlparser.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/document.rs, components/script/dom/browsingcontext.rs, components/script/dom/workerglobalscope.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/dom/worker.rs, components/script/webdriver_handlers.rs, components/script/dom/serviceworker.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/script_thread.rs, components/script/dom/htmliframeelement.rs, components/script/dom/console.rs, components/script/dom/storage.rs, components/script/dom/htmlformelement.rs, components/script/dom/bindings/trace.rs, components/script/dom/serviceworkerglobalscope.rs

highfive · 2016-06-10T07:33:07Z

Warning

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

aneeshusa · 2016-06-10T07:35:28Z

BTW, this isn't a rebase of #7792; I learned a lot about how Servo works by doing this myself :)

highfive · 2016-06-10T11:37:17Z

New code was committed to pull request.

bors-servo · 2016-06-10T16:58:14Z

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

cbrewster · 2016-06-10T23:33:56Z

Would it be possible to get rid of the (PipelineId, PipelineId)s?

aneeshusa · 2016-06-11T02:29:54Z

Rebased (not sure why highfive didn't leave a comment).

@ConnorGBrewster in general, the second PipelineId is necessary to identify which pipeline the message is about, and the first PipelineId is for the parent, which is needed to find the relevant iframe via get_browsing_context() -> active_document() -> find_iframe().

It's possible that in some cases this may be able to be reduced to just the child PipelineId, but it's not obvious to me that that holds and I'd like to leave that for a follow-up.

cbrewster · 2016-06-11T02:51:58Z

@aneeshusa makes sense, I was thinking you could just do the child and do the lookup in the constellation. But yeah that seems like it would be out of scope for this PR.

bors-servo · 2016-06-12T11:48:41Z

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

aneeshusa · 2016-06-26T15:21:10Z

components/script/script_thread.rs

-                    None => return warn!("Message sent to closed pipeline {}.", pipeline_id),
+                    None => return warn!("Message sent to closed pipeline {}.", parent_pipeline_id),
                };
+                let document = document.r();


Note: @asajeffrey removed this line in #11695, but this seemed inadvertent to me so I brought it back. Not sure about this change.

Please remove it again.

cbrewster · 2016-06-26T21:33:21Z

@bors-servo try

Excise SubpageId and use only PipelineIds  SubpageId was originally introduced in 2013 to help iframes keep track of their associated (children) pipelines. However, since each pipeline already has a PipelineId, and those are unique, those are sufficient to keep track of children. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11694 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11698)

bors-servo · 2016-06-26T21:33:24Z

⌛ Trying commit 19cf404 with merge c36df77...

bors-servo · 2016-06-26T21:45:49Z

💔 Test failed - mac-dev-unit

cbrewster · 2016-06-26T22:59:03Z

This is looking awesome! I have a couple things for you to look at, but other than that, it looks good. I especially like the move to more consistent naming.

I would like to see if there is a way to avoid passing parent and child pipelines everywhere and I would like to have FrameType moved to the Pipeline itself, but I think both those can be done in separate PRs.

After the fixes, it would probably be good to have someone else do a look over the changes.

Reviewed 7 of 17 files at r1, 1 of 13 files at r7, 4 of 16 files at r10, 11 of 12 files at r12, 1 of 1 files at r13.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

components/constellation/constellation.rs, line 1363 [r13] (raw file):

        // This makes things like contentDocument work correctly.
        if let Some((parent_pipeline_id, _)) = pipeline_info {
            match self.pipelines.get(&next_pipeline_id) {

This match can go away.

components/script/script_thread.rs, line 1197 [r13] (raw file):

        // Kick off the fetch for the new resource.
        let new_load = InProgressLoad::new(new_pipeline_id, Some((parent_pipeline_id, frame_type)),

Should this be the child pipeline id instead of the parent?

components/script/script_thread.rs, line 1427 [r13] (raw file):

    fn handle_page_headers_available(&self, id: &PipelineId,
                                     metadata: Option<Metadata>) -> Option<ParserRoot> {
        let idx = self.incomplete_loads.borrow().iter().position(|load| { load.pipeline_id == *id });

See above about child pipeline instead of parent.

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

    }

    pub fn pipeline(&self) -> Option<PipelineId> {

That's weird that we had both pipeline() and pipeline_id()...

Comments from Reviewable

bors-servo · 2016-07-03T16:58:08Z

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

bors-servo · 2016-09-13T19:49:53Z

⌛ Trying commit 56fbfd4 with merge 93414ce...

Excise SubpageId and use only PipelineIds  SubpageId was originally introduced in 2013 to help iframes keep track of their associated (children) pipelines. However, since each pipeline already has a PipelineId, and those are unique, those are sufficient to keep track of children. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11694 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11698)

bors-servo · 2016-09-13T20:46:20Z

💔 Test failed - mac-rel-wpt

aneeshusa · 2016-09-13T21:17:06Z

Failures look like intermittents and linux-rel passed wpt ok. r? @asajeffrey

asajeffrey · 2016-09-13T22:28:32Z

@bors-sevo: r+

asajeffrey · 2016-09-13T22:29:23Z

@bors-servo: r+

bors-servo · 2016-09-13T22:29:24Z

📌 Commit 56fbfd4 has been approved by asajeffrey

cbrewster · 2016-09-13T22:53:44Z

@bors-servo retry

bors-servo · 2016-09-13T22:53:46Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, windows-dev are reusable. Rebuilding only mac-rel-wpt...

notriddle · 2016-09-14T06:45:54Z

@bors-servo clean force retry r+

bors-servo · 2016-09-14T06:45:55Z

💡 This pull request was already approved, no need to approve it again.

This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.

bors-servo · 2016-09-14T06:45:55Z

📌 Commit 56fbfd4 has been approved by notriddle

bors-servo · 2016-09-14T06:45:57Z

⌛ Testing commit 56fbfd4 with merge 161e074...

Excise SubpageId and use only PipelineIds  SubpageId was originally introduced in 2013 to help iframes keep track of their associated (children) pipelines. However, since each pipeline already has a PipelineId, and those are unique, those are sufficient to keep track of children. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #11694 (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because refactoring   --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11698)

bors-servo · 2016-09-14T07:42:15Z

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

highfive assigned metajack Jun 10, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 10, 2016

aneeshusa force-pushed the remove-subpage-id branch from 9bd1139 to 775786d Compare June 10, 2016 11:37

highfive added the S-needs-rebase There are merge conflict errors. label Jun 10, 2016

aneeshusa force-pushed the remove-subpage-id branch from 775786d to 3492085 Compare June 11, 2016 01:44

highfive added the S-needs-rebase There are merge conflict errors. label Jun 12, 2016

aneeshusa force-pushed the remove-subpage-id branch from 3492085 to 23ce28c Compare June 26, 2016 15:04

aneeshusa reviewed Jun 26, 2016
View reviewed changes

aneeshusa force-pushed the remove-subpage-id branch from 23ce28c to 19cf404 Compare June 26, 2016 19:17

highfive added the S-tests-failed The changes caused existing tests to fail. label Jun 26, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Jul 3, 2016

highfive added the S-tests-failed The changes caused existing tests to fail. label Sep 13, 2016

aneeshusa removed the S-awaiting-answer Someone asked a question that requires an answer. label Sep 13, 2016

highfive assigned notriddle and unassigned asajeffrey Sep 14, 2016

bors-servo merged commit 56fbfd4 into servo:master Sep 14, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Sep 14, 2016

Uh oh!

Conversation

aneeshusa commented Jun 10, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Jun 10, 2016

Uh oh!

highfive commented Jun 10, 2016

Uh oh!

aneeshusa commented Jun 10, 2016

Uh oh!

highfive commented Jun 10, 2016

Uh oh!

bors-servo commented Jun 10, 2016

Uh oh!

cbrewster commented Jun 10, 2016

Uh oh!

aneeshusa commented Jun 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbrewster commented Jun 11, 2016

Uh oh!

bors-servo commented Jun 12, 2016

Uh oh!

aneeshusa Jun 26, 2016

Choose a reason for hiding this comment

Uh oh!

Ms2ger Jun 26, 2016

Choose a reason for hiding this comment

Uh oh!

cbrewster commented Jun 26, 2016

Uh oh!

bors-servo commented Jun 26, 2016

Uh oh!

bors-servo commented Jun 26, 2016

Uh oh!

cbrewster commented Jun 26, 2016

Uh oh!

bors-servo commented Jul 3, 2016

Uh oh!

bors-servo commented Sep 13, 2016

Uh oh!

bors-servo commented Sep 13, 2016

Uh oh!

aneeshusa commented Sep 13, 2016

Uh oh!

asajeffrey commented Sep 13, 2016

Uh oh!

asajeffrey commented Sep 13, 2016

Uh oh!

bors-servo commented Sep 13, 2016

Uh oh!

cbrewster commented Sep 13, 2016

Uh oh!

bors-servo commented Sep 13, 2016

Uh oh!

notriddle commented Sep 14, 2016

Uh oh!

bors-servo commented Sep 14, 2016

Uh oh!

bors-servo commented Sep 14, 2016

Uh oh!

bors-servo commented Sep 14, 2016

Uh oh!

bors-servo commented Sep 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

aneeshusa commented Jun 10, 2016 •

edited by larsbergstrom

Loading

aneeshusa commented Jun 11, 2016 •

edited

Loading