Pipeline always stores frame #13627

asajeffrey · 2016-10-06T23:17:08Z

This change makes the pipeline always store the frame id, not just optionally. This is the first part of a long slog to use FrameIds rather than PipelineIds to identify frames. cc @ConnorGBrewster

./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-06T23:17:11Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/bindings/trace.rs, components/script_traits/lib.rs, components/script_traits/lib.rs

highfive · 2016-10-06T23:17:12Z

Warning

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

asajeffrey · 2016-10-06T23:17:57Z

The first commit is from #13498, and isn't really part of this PR.

cbrewster · 2016-10-07T01:03:26Z

cc @paulrouget were you still interested in having pipelines that are detached from Frames?

paulrouget · 2016-10-07T08:09:11Z

@paulrouget were you still interested in having pipelines that are detached from Frames?

That used to be a thing, but no, not anymore.

I see no problem with this PR.

bors-servo · 2016-10-07T14:10:19Z

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

bors-servo · 2016-10-07T20:58:41Z

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

asajeffrey · 2016-10-07T21:51:38Z

#13498 has now landed, so this can now merge. r? @ConnorGBrewster

cbrewster

Looks good, except my one comment. Not 100% sure what to do in that case right now.

cbrewster · 2016-10-09T02:26:08Z

components/constellation/constellation.rs

@@ -2007,7 +2008,7 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        // a dependent pipeline to be ready to paint.
        while let Some(valid_frame_change) = self.pending_frames.iter().rposition(|frame_change| {
            let waiting_on_dependency = frame_change.old_pipeline_id.map_or(false, |old_pipeline_id| {
-                self.pipelines.get(&old_pipeline_id).map(|pipeline| pipeline.frame).is_none()
+                self.pipelines.get(&old_pipeline_id).and_then(|pipeline| self.frames.get(&pipeline.frame_id)).is_none()


I don't believe this is equivalent to what we had before. Before, if frame is None, it means that the pipeline has not matured yet. In this change it is checking if the Frame exists. I think this will actually be ok, just because I am not sure if we ever have a scenario where one pending frame change is waiting on another.

I could add a is_mature flag to Pipeline, to capture the case where the frame id was None.

Let's do that!

cbrewster

Looks good to me except possibly 1 nit.

Squash and r=me

cbrewster · 2016-10-10T15:13:36Z

components/constellation/constellation.rs

@@ -663,6 +663,7 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
        assert!(self.pipelines.get(&pipeline_id).map(|pipeline| pipeline.frame_id == frame_id).unwrap_or(false));
        assert!(!self.frames.contains_key(&frame_id));

+        self.pipelines.get_mut(&pipeline_id).map(|pipeline| pipeline.is_mature = true);


nit: should we use if let here instead of map since we are changing state.

Fixed. I think we should also get rid of those assertions, as they are potential sources of panic in the constellation.

asajeffrey · 2016-10-10T17:11:04Z

Squashed, @bors-servo r=ConnorGBrewster

bors-servo · 2016-10-10T17:11:05Z

📌 Commit c7c45ce has been approved by ConnorGBrewster

bors-servo · 2016-10-10T17:39:05Z

⌛ Testing commit c7c45ce with merge d175d63...


        Auto merge of #13627 - asajeffrey:pipeline-always-stores-frame-id, r=…

…ConnorGBrewster Pipeline always stores frame  This change makes the pipeline always store the frame id, not just optionally. This is the first part of a long slog to use FrameIds rather than PipelineIds to identify frames. cc @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/13627)

bors-servo · 2016-10-10T18:12:40Z

💔 Test failed - linux-rel-wpt

highfive · 2016-10-10T18:12:43Z

  ▶ Unexpected subtest result in /html/browsers/history/joint-session-history/joint-session-history-only-fully-active.html:
  └ PASS [expected FAIL] Do only fully active documents count for session history?

  ▶ TIMEOUT [expected OK] /_mozilla/mozilla/mozbrowser/mozbrowserlocationchange_event.html
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  └ 3.3 (Core Profile) Mesa 12.0.1

  ▶ Unexpected subtest result in /_mozilla/mozilla/mozbrowser/mozbr</span><span class="stdout">owserlocationchange_event.html:
  │ TIMEOUT [expected PASS] Browser API; mozbrowserlocationchange event
  └   → Test timed out


        Made pipeline always store the frame_id.

asajeffrey · 2016-10-10T19:02:41Z

Fixed a case where is_mature wasn't being set properly. @bors-servo r=ConnorGBrewster via irc.

bors-servo · 2016-10-10T19:02:41Z

📌 Commit eef02dd has been approved by ConnorGBrewster

bors-servo · 2016-10-10T19:58:15Z

⌛ Testing commit eef02dd with merge 51b806f...


        Auto merge of #13627 - asajeffrey:pipeline-always-stores-frame-id, r=…

…ConnorGBrewster Pipeline always stores frame  This change makes the pipeline always store the frame id, not just optionally. This is the first part of a long slog to use FrameIds rather than PipelineIds to identify frames. cc @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/13627)

bors-servo · 2016-10-10T20:51:09Z

☀️ 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

asajeffrey added I-refactor A-constellation labels Oct 6, 2016

highfive assigned larsbergstrom Oct 6, 2016

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

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

asajeffrey force-pushed the asajeffrey:pipeline-always-stores-frame-id branch from 9d0ed77 to e47a2ff Oct 7, 2016

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

asajeffrey force-pushed the asajeffrey:pipeline-always-stores-frame-id branch from e47a2ff to 8ca1cec Oct 7, 2016

highfive assigned cbrewster and unassigned larsbergstrom Oct 7, 2016

asajeffrey mentioned this pull request Oct 7, 2016

Script lookup iframes by frameid #13646

Merged

3 of 3 tasks complete

cbrewster reviewed Oct 9, 2016

View changes

cbrewster added S-awaiting-answer and removed S-awaiting-review labels Oct 9, 2016

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

cbrewster approved these changes Oct 10, 2016

View changes

cbrewster added S-needs-squash and removed S-awaiting-answer S-awaiting-review S-needs-rebase labels Oct 10, 2016

asajeffrey force-pushed the asajeffrey:pipeline-always-stores-frame-id branch from 1da8b65 to c7c45ce Oct 10, 2016

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

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Oct 10, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 10, 2016

Made pipeline always store the frame_id.

Loading status checks…

eef02dd

asajeffrey force-pushed the asajeffrey:pipeline-always-stores-frame-id branch from c7c45ce to eef02dd Oct 10, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Oct 10, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 10, 2016

bors-servo merged commit eef02dd into servo:master Oct 10, 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 Oct 10, 2016

servo / servo

Pipeline always stores frame #13627

Pipeline always stores frame #13627

asajeffrey commented Oct 6, 2016 •

edited by larsbergstrom

highfive commented Oct 6, 2016

highfive commented Oct 6, 2016

asajeffrey commented Oct 6, 2016

cbrewster commented Oct 7, 2016

paulrouget commented Oct 7, 2016

bors-servo commented Oct 7, 2016

bors-servo commented Oct 7, 2016

asajeffrey commented Oct 7, 2016

cbrewster left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cbrewster left a comment

This comment has been minimized.

This comment has been minimized.

asajeffrey commented Oct 10, 2016

bors-servo commented Oct 10, 2016

bors-servo commented Oct 10, 2016

bors-servo commented Oct 10, 2016

highfive commented Oct 10, 2016

asajeffrey commented Oct 10, 2016

bors-servo commented Oct 10, 2016

bors-servo commented Oct 10, 2016

bors-servo commented Oct 10, 2016

servo / servo

Join GitHub today

Pipeline always stores frame #13627

Pipeline always stores frame #13627

Conversation

asajeffrey commented Oct 6, 2016 • edited by larsbergstrom

highfive commented Oct 6, 2016

highfive commented Oct 6, 2016

asajeffrey commented Oct 6, 2016

cbrewster commented Oct 7, 2016

paulrouget commented Oct 7, 2016

bors-servo commented Oct 7, 2016

bors-servo commented Oct 7, 2016

asajeffrey commented Oct 7, 2016

cbrewster left a comment

This comment has been minimized.

cbrewster Oct 9, 2016

This comment has been minimized.

asajeffrey Oct 10, 2016

This comment has been minimized.

cbrewster Oct 10, 2016

This comment has been minimized.

asajeffrey Oct 10, 2016

cbrewster left a comment

This comment has been minimized.

cbrewster Oct 10, 2016

This comment has been minimized.

asajeffrey Oct 10, 2016

asajeffrey commented Oct 10, 2016

bors-servo commented Oct 10, 2016

bors-servo commented Oct 10, 2016

bors-servo commented Oct 10, 2016

highfive commented Oct 10, 2016

asajeffrey commented Oct 10, 2016

bors-servo commented Oct 10, 2016

bors-servo commented Oct 10, 2016

bors-servo commented Oct 10, 2016

asajeffrey commented Oct 6, 2016 •

edited by larsbergstrom