Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IFrame elements now manage FrameIds rather than the constellation. #13498

Merged
merged 1 commit into from Oct 7, 2016

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Sep 29, 2016

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 Reviewable

@highfive
Copy link

highfive commented Sep 29, 2016

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
Copy link

highfive commented Sep 29, 2016

warning Warning warning

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

glennw left a comment

Looks good - just one comment request :)

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

This comment has been minimized.

Copy link
@glennw

glennw Oct 6, 2016

Member

Add a comment explaining why this starts at 1?

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Oct 6, 2016

Author Member

Done.

@glennw
Copy link
Member

glennw commented Oct 6, 2016

@asajeffrey Nice! Squash and r=me

@jdm jdm added S-needs-squash and removed S-awaiting-review labels Oct 6, 2016
@asajeffrey asajeffrey force-pushed the asajeffrey:script-iframe-stores-frameid branch from 3a06846 to b400461 Oct 6, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Oct 6, 2016

Squashed. Thanks! @bors-servo r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Oct 6, 2016

📌 Commit b400461 has been approved by glennw

@asajeffrey asajeffrey mentioned this pull request Oct 6, 2016
3 of 3 tasks complete
@bors-servo
Copy link
Contributor

bors-servo commented Oct 6, 2016

Testing commit b400461 with merge 12973e5...

bors-servo added a commit that referenced this pull request Oct 6, 2016
IFrame elements now manage FrameIds rather than the constellation.

<!-- Please describe your changes on the following line: -->

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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13498)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2016

💔 Test failed - linux-rel-wpt

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2016

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

@asajeffrey asajeffrey force-pushed the asajeffrey:script-iframe-stores-frameid branch from b400461 to 466dcd8 Oct 7, 2016
@asajeffrey asajeffrey force-pushed the asajeffrey:script-iframe-stores-frameid branch from 466dcd8 to 8ecb13b Oct 7, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Oct 7, 2016

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

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2016

📌 Commit 8ecb13b has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2016

Testing commit 8ecb13b with merge faecd9d...

bors-servo added a commit that referenced this pull request Oct 7, 2016
IFrame elements now manage FrameIds rather than the constellation.

<!-- Please describe your changes on the following line: -->

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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13498)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2016

💔 Test failed - mac-dev-unit

@asajeffrey asajeffrey force-pushed the asajeffrey:script-iframe-stores-frameid branch from d89d2f3 to f53408d Oct 7, 2016
@asajeffrey
Copy link
Member Author

asajeffrey commented Oct 7, 2016

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

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2016

📌 Commit f53408d has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2016

Testing commit f53408d with merge d01a866...

bors-servo added a commit that referenced this pull request Oct 7, 2016
IFrame elements now manage FrameIds rather than the constellation.

<!-- Please describe your changes on the following line: -->

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.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [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.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13498)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 7, 2016

@bors-servo bors-servo merged commit f53408d into servo:master Oct 7, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.