Pipeline always stores frame by asajeffrey · Pull Request #13627 · servo/servo

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:23:53Z

components/constellation/constellation.rs

        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:12:39Z

components/constellation/constellation.rs

        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...

…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

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...

…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 No impact; the issue is one of maintainability or tidiness. Proposed solution requires refactoring. A-constellation Involves the constellation labels Oct 6, 2016

highfive assigned larsbergstrom Oct 6, 2016

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

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

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

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

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

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 tasks

cbrewster reviewed Oct 9, 2016

View reviewed changes

cbrewster added S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 9, 2016

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

cbrewster approved these changes Oct 10, 2016

View reviewed changes

cbrewster added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-answer Someone asked a question that requires an answer. S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Oct 10, 2016

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

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

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Oct 10, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Oct 10, 2016

Made pipeline always store the frame_id.

eef02dd

asajeffrey force-pushed the pipeline-always-stores-frame-id branch from c7c45ce to eef02dd Compare October 10, 2016 18:59

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Oct 10, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 10, 2016

bors-servo merged commit eef02dd into servo:master Oct 10, 2016

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

Uh oh!

Conversation

asajeffrey commented Oct 6, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Oct 6, 2016

Uh oh!

highfive commented Oct 6, 2016

Uh oh!

asajeffrey commented Oct 6, 2016

Uh oh!

cbrewster commented Oct 7, 2016

Uh oh!

paulrouget commented Oct 7, 2016

Uh oh!

bors-servo commented Oct 7, 2016

Uh oh!

bors-servo commented Oct 7, 2016

Uh oh!

asajeffrey commented Oct 7, 2016

Uh oh!

cbrewster left a comment

Choose a reason for hiding this comment

Uh oh!

cbrewster Oct 9, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

cbrewster Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

cbrewster left a comment

Choose a reason for hiding this comment

Uh oh!

cbrewster Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey commented Oct 10, 2016

Uh oh!

bors-servo commented Oct 10, 2016

Uh oh!

bors-servo commented Oct 10, 2016

Uh oh!

bors-servo commented Oct 10, 2016

Uh oh!

highfive commented Oct 10, 2016

Uh oh!

asajeffrey commented Oct 10, 2016

Uh oh!

bors-servo commented Oct 10, 2016

Uh oh!

bors-servo commented Oct 10, 2016

Uh oh!

bors-servo commented Oct 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

asajeffrey commented Oct 6, 2016 •

edited by larsbergstrom

Loading