Combine Page into BrowsingContext #11044

cbrewster · 2016-05-06T04:52:42Z

Fixes #11031.

Page and BrowsingContext have similar use cases and we decided it would be best to join the two.

This is the ground work for actually using session history in the BrowsingContext to implement the History API.

r? @jdm

This change is

highfive · 2016-05-06T04:52:46Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/htmliframeelement.rs, components/script/lib.rs, components/script/webdriver_handlers.rs, components/script/dom/storage.rs, components/script/dom/browsingcontext.rs, components/script/page.rs, components/script/script_thread.rs, components/script/dom/window.rs, components/script/devtools.rs

highfive · 2016-05-06T04:52:47Z

Warning

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

cbrewster · 2016-05-06T04:53:03Z

@bors-servo try


        Auto merge of #11044 - ConnorGBrewster:page_to_browsing_context, r=<try>

@jdm

Combine Page into BrowsingContext Fixes #11031. `Page` and `BrowsingContext` have similar use cases and we decided it would be best to join the two. This is the ground work for actually using session history in the `BrowsingContext` to implement the History API. r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11044)

bors-servo · 2016-05-06T04:53:06Z

⌛ Trying commit 7165d3a with merge b7bcc25...

bors-servo · 2016-05-06T05:53:19Z

💔 Test failed - mac-rel-css

highfive · 2016-05-06T05:53:22Z

  ▶ TIMEOUT [expected FAIL] /css-transforms-1_dev/html/iframe-001.htm
  │ 
  └ Shutting down the Constellation after generating an output file or exit flag specified

  ▶ TIMEOUT [expected PASS] /css-transforms-1_dev/html/transform-iframe-001.htm
  │ 
  └ Shutting down the Constellation after generating an output file or exit flag specified

  ▶ CRASH [expected FAIL] /css21_dev/html4/font-size-121.htm
  │ 
  └ Shutting down the Constellation after generating an output file or exit flag specified

  ▶ TIMEOUT [expected FAIL] /css21_dev/html4/min-width-tables-001.htm
  │ 
  └ Shutting down the Constellation after generating an output file or exit flag specified

highfive · 2016-05-06T21:29:21Z

New code was committed to pull request.

highfive · 2016-05-06T21:30:34Z

New code was committed to pull request.

cbrewster · 2016-05-06T22:26:35Z

@bors-servo try

bors-servo · 2016-05-06T22:26:37Z

⌛ Trying commit 4cddf88 with merge fa7fd35...


        Auto merge of #11044 - ConnorGBrewster:page_to_browsing_context, r=<try>

@jdm

Combine Page into BrowsingContext Fixes #11031. `Page` and `BrowsingContext` have similar use cases and we decided it would be best to join the two. This is the ground work for actually using session history in the `BrowsingContext` to implement the History API. r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11044)

bors-servo · 2016-05-06T22:32:13Z

💔 Test failed - windows

highfive · 2016-05-06T23:54:30Z

New code was committed to pull request.

bors-servo · 2016-05-06T23:54:43Z

⌛ Trying commit 4e98f43 with merge 834c782...


        Auto merge of #11044 - ConnorGBrewster:page_to_browsing_context, r=<try>

@jdm

Combine Page into BrowsingContext Fixes #11031. `Page` and `BrowsingContext` have similar use cases and we decided it would be best to join the two. This is the ground work for actually using session history in the `BrowsingContext` to implement the History API. r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11044)

bors-servo · 2016-05-07T00:33:03Z

💔 Test failed - mac-rel-css

highfive · 2016-05-07T00:33:06Z

  ▶ TIMEOUT [expected FAIL] /css-transforms-1_dev/html/iframe-001.htm
  │ 
  └ Shutting down the Constellation after generating an output file or exit flag specified

  ▶ CRASH [expected FAIL] /css21_dev/html4/c5510-ipadn-000.htm
  │ 
  └ Shutting down the Constellation after generating an output file or exit flag specified

  ▶ CRASH [expected PASS] /css21_dev/html4/font-size-081.htm
  │ 
  └ Shutting down the Constellation after generating an output file or exit flag specified

  ▶ CRASH [expected PASS] /css21_dev/html4/padding-bottom-applies-to-002.htm
  │ 
  └ Shutting down the Constellation after generating an output file or exit flag specified

  ▶ CRASH [expected FAIL] /css21_dev/html4/replaced-intrinsic-003.htm
  │ 
  │ Shutting down the Constellation after generating an output file or exit flag specified
  └ Shutting down the Constellation after generating an output file or exit flag specified

  ▶ CRASH [expected PASS] /css21_dev/html4/width-045.htm
  │ 
  └ Shutting down the Constellation after generating an output file or exit flag specified

cbrewster · 2016-05-07T02:53:00Z

@bors-servo retry


        Auto merge of #11113 - jdm:trace_browsingcontext, r=Ms2ger

@Ms2ger

Trace and finalize BrowsingContext This is a prerequisite for merging #11044, and is an important correctness fix on its own. r? @Ms2ger  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11113)

bors-servo · 2016-05-11T13:58:45Z

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


        remove page and move functionality to browing context

Allow for adding history items Fixed nested iframe test failure Cleanup and small refactors fixup

cbrewster · 2016-05-11T18:48:15Z

This should now be ready for review.


        removed instances of &Root<BrowsingContext>

highfive · 2016-05-11T19:21:24Z

New code was committed to pull request.

jdm · 2016-05-11T19:54:17Z

This is great! I'm so please to see Page totally gone :)
-S-awaiting-review +S-needs-code-changes

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 9 of 9 files at r1, 14 of 14 files at r2, 12 of 12 files at r3, 1 of 1 files at r4, 3 of 3 files at r5, 1 of 1 files at r6, 3 of 3 files at r7, 2 of 2 files at r8, 3 of 3 files at r9, 18 of 18 files at r10, 13 of 13 files at r11, 3 of 3 files at r12, 3 of 3 files at r13.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

components/script/script_thread.rs, line 627 [r1] (raw file):

        let mut resizes = vec!();

        {

We can get rid of the extra scope now that there's no borrowing.

components/script/script_thread.rs, line 1398 [r1] (raw file):

            chan.send(ConstellationMsg::SetFinalUrl(incomplete.pipeline_id, final_url.clone())).unwrap();
        }
        debug!("ScriptThread: loading {} on context {:?}", incomplete.url, incomplete.pipeline_id);

I think pipeline would make more sense here.

components/script/dom/browsingcontext.rs, line 190 [r1] (raw file):

            self.stack.extend(context.children.borrow()
                                              .iter()
                                              .cloned()

Why is this cloned necessary?

components/script/dom/browsingcontext.rs, line 104 [r10] (raw file):

        history.push(SessionHistoryEntry::new(document, document.url().clone(), document.Title()));
        self.active_index.set(self.active_index.get() + 1);
    }

Let's add assert_eq!(self.active_index.get(), history.len() - 1).

components/script/dom/browsingcontext.rs, line 130 [r10] (raw file):

                             .position(|context| context.id == id);
        match remove_idx {
            Some(idx) => Some(Root::from_ref(&*self.children.borrow_mut().remove(idx))),

Is the Root::from_ref even necessary here?

Comments from Reviewable


        Addressed comment

highfive · 2016-05-11T21:01:55Z

New code was committed to pull request.

cbrewster · 2016-05-11T21:01:58Z

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

components/script/script_thread.rs, line 627 [r1] (raw file):

Previously, jdm (Josh Matthews) wrote…

We can get rid of the extra scope now that there's no borrowing.

Done.

components/script/script_thread.rs, line 1398 [r1] (raw file):

Previously, jdm (Josh Matthews) wrote…

I think pipeline would make more sense here.

Done.

components/script/dom/browsingcontext.rs, line 190 [r1] (raw file):

Previously, jdm (Josh Matthews) wrote…

Why is this cloned necessary?

Done.

components/script/dom/browsingcontext.rs, line 104 [r10] (raw file):

Previously, jdm (Josh Matthews) wrote…

Let's add assert_eq!(self.active_index.get(), history.len() - 1).

Done.

components/script/dom/browsingcontext.rs, line 130 [r10] (raw file):

Previously, jdm (Josh Matthews) wrote…

Is the Root::from_ref even necessary here?

The Vec stores JS<BrowsingContext> and the function returns a Option<Root<BrowsingContext>>. I have that to change the JS to Root. Is that ok? or is there a better way to handle this?

Comments from Reviewable

jdm · 2016-05-11T21:46:51Z

@bors-servo: r+
Yay!

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 2 of 2 files at r14.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

bors-servo · 2016-05-11T21:46:52Z

📌 Commit e50eb2a has been approved by jdm

bors-servo · 2016-05-12T03:53:03Z

⌛ Testing commit e50eb2a with merge 685dc99...


        Auto merge of #11044 - ConnorGBrewster:page_to_browsing_context, r=jdm

@jdm

Combine Page into BrowsingContext Fixes #11031. `Page` and `BrowsingContext` have similar use cases and we decided it would be best to join the two. This is the ground work for actually using session history in the `BrowsingContext` to implement the History API. r? @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11044)

bors-servo · 2016-05-12T05:03:35Z

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

highfive assigned jdm May 6, 2016

highfive added the S-awaiting-review label May 6, 2016

highfive added the S-tests-failed label May 6, 2016

cbrewster mentioned this pull request May 6, 2016

History API metabug #10992

Open

18 of 24 tasks complete

cbrewster force-pushed the cbrewster:page_to_browsing_context branch from 7165d3a to a2faace May 6, 2016

highfive removed the S-tests-failed label May 6, 2016

cbrewster force-pushed the cbrewster:page_to_browsing_context branch from a2faace to 4cddf88 May 6, 2016

highfive added the S-tests-failed label May 6, 2016

cbrewster force-pushed the cbrewster:page_to_browsing_context branch from 4cddf88 to 4e98f43 May 6, 2016

highfive removed the S-tests-failed label May 6, 2016

highfive added the S-tests-failed label May 7, 2016

highfive added the S-needs-rebase label May 11, 2016

jdm removed the S-blocked-on-external label May 11, 2016

remove page and move functionality to browing context

Loading status checks…

cbc5ca6

Allow for adding history items Fixed nested iframe test failure Cleanup and small refactors fixup

cbrewster force-pushed the cbrewster:page_to_browsing_context branch from d323481 to cbc5ca6 May 11, 2016

cbrewster removed the S-needs-rebase label May 11, 2016

removed instances of &Root<BrowsingContext>

Loading status checks…

9efd214

jdm added S-needs-code-changes and removed S-awaiting-review labels May 11, 2016

Addressed comment

Loading status checks…

e50eb2a

highfive removed the S-needs-code-changes label May 11, 2016

highfive added the S-awaiting-review label May 11, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels May 11, 2016

bors-servo merged commit e50eb2a into servo:master May 12, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label May 12, 2016

This was referenced May 12, 2016

Flesh out the iframe attribute processing steps. #10972

Closed

Implement non-visible pipeline and iframe visibility methods #10225

Merged

jdm mentioned this pull request May 20, 2016

Intermittent Mac script thread panic on shutdown: Option::unwrap in ScriptMemoryFailsafe::drop or Page::window #11059

Closed

servo / servo

Combine Page into BrowsingContext #11044

Combine Page into BrowsingContext #11044

cbrewster commented May 6, 2016 •

edited by larsbergstrom

highfive commented May 6, 2016

highfive commented May 6, 2016

cbrewster commented May 6, 2016

bors-servo commented May 6, 2016

bors-servo commented May 6, 2016

highfive commented May 6, 2016

highfive commented May 6, 2016

highfive commented May 6, 2016

cbrewster commented May 6, 2016

bors-servo commented May 6, 2016

bors-servo commented May 6, 2016

highfive commented May 6, 2016

bors-servo commented May 6, 2016

bors-servo commented May 7, 2016

highfive commented May 7, 2016

cbrewster commented May 7, 2016 •

edited

bors-servo commented May 11, 2016

cbrewster commented May 11, 2016

highfive commented May 11, 2016

jdm commented May 11, 2016

highfive commented May 11, 2016

cbrewster commented May 11, 2016

jdm commented May 11, 2016

bors-servo commented May 11, 2016

bors-servo commented May 12, 2016

bors-servo commented May 12, 2016

servo / servo

Join GitHub today

Combine Page into BrowsingContext #11044

Combine Page into BrowsingContext #11044

Conversation

cbrewster commented May 6, 2016 • edited by larsbergstrom

highfive commented May 6, 2016

highfive commented May 6, 2016

cbrewster commented May 6, 2016

bors-servo commented May 6, 2016

bors-servo commented May 6, 2016

highfive commented May 6, 2016

highfive commented May 6, 2016

highfive commented May 6, 2016

cbrewster commented May 6, 2016

bors-servo commented May 6, 2016

bors-servo commented May 6, 2016

highfive commented May 6, 2016

bors-servo commented May 6, 2016

bors-servo commented May 7, 2016

highfive commented May 7, 2016

cbrewster commented May 7, 2016 • edited

bors-servo commented May 11, 2016

cbrewster commented May 11, 2016

highfive commented May 11, 2016

jdm commented May 11, 2016

highfive commented May 11, 2016

cbrewster commented May 11, 2016

jdm commented May 11, 2016

bors-servo commented May 11, 2016

bors-servo commented May 12, 2016

bors-servo commented May 12, 2016

cbrewster commented May 6, 2016 •

edited by larsbergstrom

cbrewster commented May 7, 2016 •

edited