IFrame elements now manage FrameIds rather than the constellation. by asajeffrey · Pull Request #13498 · servo/servo

asajeffrey · 2016-09-29T03:16:48Z

This PR stores the FrameId as well as the PipelineId in an html iframe. The iframes are now responsible for creating frame ids, not the constellation.

This is the first step in fixing #633, because it means we know the frame id of each script thread when it is created. It also means we can share the frame id, for example using it in the debugger.

cc @jdm, @ConnorGBrewster and @ejpbruel.

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

This change is

highfive · 2016-09-29T03:16:52Z

Heads up! This PR modifies the following files:

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

highfive · 2016-09-29T03:16:53Z

Warning

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

glennw

Looks good - just one comment request :)

glennw · 2016-10-06T20:28:05Z

components/constellation/constellation.rs

-                next_pipeline_namespace_id: PipelineNamespaceId(0),
-                root_frame_id: None,
-                next_frame_id: FrameId(0),
+                next_pipeline_namespace_id: PipelineNamespaceId(1),


Add a comment explaining why this starts at 1?

glennw · 2016-10-06T21:40:11Z

@asajeffrey Nice! Squash and r=me

asajeffrey · 2016-10-06T23:02:32Z

Squashed. Thanks! @bors-servo r=glennw

bors-servo · 2016-10-06T23:02:33Z

📌 Commit b400461 has been approved by glennw

bors-servo · 2016-10-06T23:46:48Z

⌛ Testing commit b400461 with merge 12973e5...

@jdm

IFrame elements now manage FrameIds rather than the constellation.  This PR stores the FrameId as well as the PipelineId in an html iframe. The iframes are now responsible for creating frame ids, not the constellation. This is the first step in fixing #633, because it means we know the frame id of each script thread when it is created. It also means we can share the frame id, for example using it in the debugger. cc @jdm, @ConnorGBrewster and @ejpbruel. ---  - [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 it's a refactoring.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13498)

bors-servo · 2016-10-07T01:08:55Z

💔 Test failed - linux-rel-wpt

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

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

asajeffrey · 2016-10-07T16:57:47Z

Rebased, and fixed test failure due to left-over fake root pipeline. @bors-servo r=glennw

bors-servo · 2016-10-07T16:57:48Z

📌 Commit 8ecb13b has been approved by glennw

bors-servo · 2016-10-07T17:31:38Z

⌛ Testing commit 8ecb13b with merge faecd9d...

@jdm

IFrame elements now manage FrameIds rather than the constellation.  This PR stores the FrameId as well as the PipelineId in an html iframe. The iframes are now responsible for creating frame ids, not the constellation. This is the first step in fixing #633, because it means we know the frame id of each script thread when it is created. It also means we can share the frame id, for example using it in the debugger. cc @jdm, @ConnorGBrewster and @ejpbruel. ---  - [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 it's a refactoring.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13498)

bors-servo · 2016-10-07T17:49:30Z

💔 Test failed - mac-dev-unit

asajeffrey · 2016-10-07T19:13:59Z

Fixed failing unit tests by adding a TEST_PIPELINE_ID. @bors-servo r=glennw (on IRC).

bors-servo · 2016-10-07T19:14:00Z

📌 Commit f53408d has been approved by glennw

bors-servo · 2016-10-07T19:54:32Z

⌛ Testing commit f53408d with merge d01a866...

@jdm

IFrame elements now manage FrameIds rather than the constellation.  This PR stores the FrameId as well as the PipelineId in an html iframe. The iframes are now responsible for creating frame ids, not the constellation. This is the first step in fixing #633, because it means we know the frame id of each script thread when it is created. It also means we can share the frame id, for example using it in the debugger. cc @jdm, @ConnorGBrewster and @ejpbruel. ---  - [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 it's a refactoring.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13498)

bors-servo · 2016-10-07T20:57:43Z

☀️ 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-content/script Related to the script thread A-constellation Involves the constellation labels Sep 29, 2016

highfive assigned glennw Sep 29, 2016

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

glennw reviewed Oct 6, 2016

View reviewed changes

jdm added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 6, 2016

asajeffrey force-pushed the script-iframe-stores-frameid branch from 3a06846 to b400461 Compare October 6, 2016 23:01

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 6, 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 6, 2016

asajeffrey mentioned this pull request Oct 6, 2016

Pipeline always stores frame #13627

Merged

3 tasks

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 7, 2016

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

asajeffrey force-pushed the script-iframe-stores-frameid branch from b400461 to 466dcd8 Compare October 7, 2016 16:50

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 7, 2016

asajeffrey force-pushed the script-iframe-stores-frameid branch from 466dcd8 to 8ecb13b Compare October 7, 2016 16:56

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-rebase There are merge conflict errors. labels Oct 7, 2016

IFrame elements now manage FrameIds rather than the constellation.

f53408d

asajeffrey force-pushed the script-iframe-stores-frameid branch from d89d2f3 to f53408d Compare October 7, 2016 19:11

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 7, 2016

bors-servo merged commit f53408d into servo:master Oct 7, 2016

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

This was referenced Oct 7, 2016

Purge pipelines from distant history. #11893

Closed

Maintain a list of script threads in the constellation (DO NOT MERGE) #13484

Closed

Uh oh!

Conversation

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

Uh oh!

Uh oh!

highfive commented Sep 29, 2016

Uh oh!

highfive commented Sep 29, 2016

Uh oh!

glennw left a comment

Choose a reason for hiding this comment

Uh oh!

glennw Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

asajeffrey Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

glennw commented Oct 6, 2016

Uh oh!

asajeffrey commented Oct 6, 2016

Uh oh!

bors-servo commented Oct 6, 2016

Uh oh!

bors-servo commented Oct 6, 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!

bors-servo 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!

bors-servo commented Oct 7, 2016

Uh oh!

bors-servo commented Oct 7, 2016

Uh oh!

bors-servo commented Oct 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

asajeffrey commented Sep 29, 2016 •

edited by larsbergstrom

Loading