Skip to content

Script lookup iframes by frameid#13646

Merged
bors-servo merged 1 commit intoservo:masterfrom
asajeffrey:script-lookup-iframes-by-frameid
Nov 4, 2016
Merged

Script lookup iframes by frameid#13646
bors-servo merged 1 commit intoservo:masterfrom
asajeffrey:script-lookup-iframes-by-frameid

Conversation

@asajeffrey
Copy link
Copy Markdown
Contributor

@asajeffrey asajeffrey commented Oct 7, 2016 •
edited by larsbergstrom
Loading

Lookup iframes by FrameId rather than PipelineId. This should make lookup much more reliable, since the frame id doesn't change.

cc @ConnorGBrewster @aneeshusa

  • ./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 Reviewable

@asajeffrey 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 Oct 7, 2016
@highfive
Copy link
Copy Markdown

highfive commented Oct 7, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/htmliframeelement.rs, components/script/dom/document.rs, components/script_traits/script_msg.rs, components/script_traits/script_msg.rs, components/script/script_thread.rs, components/script_traits/lib.rs, components/script_traits/lib.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 7, 2016
@highfive
Copy link
Copy Markdown

highfive commented Oct 7, 2016

warning Warning warning

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

@asajeffrey
Copy link
Copy Markdown
Contributor Author

asajeffrey commented Oct 7, 2016

The first commit in this PR is actually #13627, we shouldn't merge this one until it lands.

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Oct 10, 2016

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Oct 10, 2016
@asajeffrey asajeffrey force-pushed the script-lookup-iframes-by-frameid branch from b1d34f7 to 7948fd5 Compare October 11, 2016 17:48
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Oct 13, 2016

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Oct 13, 2016
@asajeffrey asajeffrey force-pushed the script-lookup-iframes-by-frameid branch from 7948fd5 to 94c7391 Compare October 13, 2016 22:32
@asajeffrey
Copy link
Copy Markdown
Contributor Author

asajeffrey commented Oct 13, 2016

Rebased.

@KiChjang KiChjang removed the S-needs-rebase There are merge conflict errors. label Oct 13, 2016
nox
nox reviewed Oct 17, 2016
View reviewed changes
Copy link
Copy Markdown
Contributor

@nox nox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question, otherwise r=me.

components/script_traits/script_msg.rs Outdated
/// Dispatch a mozbrowser event to a given iframe,
/// or to the window if no subpage id is provided.
/// First PipelineId is for the parent, second PipelineId is for the actual pipeline.
/// First PipelineId is for the parent, second PipelineId is in the frame.
Copy link
Copy Markdown
Contributor

@nox nox Oct 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean that it is "in the frame"? It still lives here so it's a but confusing.

Copy link
Copy Markdown
Contributor Author

@asajeffrey asajeffrey Oct 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, that second pipeline id should be a frame id.

@nox nox 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 17, 2016
@asajeffrey asajeffrey force-pushed the script-lookup-iframes-by-frameid branch from 94c7391 to bc290df Compare October 21, 2016 01:48
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 21, 2016
@asajeffrey
Copy link
Copy Markdown
Contributor Author

asajeffrey commented Oct 21, 2016

Working on this, turns out there were bugs, sigh.

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Oct 21, 2016

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

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Oct 21, 2016
@jdm
Copy link
Copy Markdown
Member

jdm commented Oct 24, 2016

I haven't done more than skim recent changes in this PR, but I'm concerned about the changes to how the document load event is initiated. The old design was that it was fine to optimistically enqueue a "I think the document is finished loading everything" event, because when being run it would first check if that were actually the case. Some of the changes and comments around them make me worried that this is no longer true.

@asajeffrey
Copy link
Copy Markdown
Contributor Author

asajeffrey commented Oct 24, 2016

@jdm yes, this is what I've been spending my time debugging. I now understand better what the document loading code is doing, I even wrote some comments so that future me might remember this. At least the wpt test suite caught this.

@asajeffrey asajeffrey force-pushed the script-lookup-iframes-by-frameid branch 3 times, most recently from 5129976 to 9631237 Compare October 28, 2016 17:18
@asajeffrey
Copy link
Copy Markdown
Contributor Author

asajeffrey commented Oct 28, 2016

This PR triggered issue #13964, fixed by #13965, which should land before this does.

bors-servo pushed a commit that referenced this pull request Nov 2, 2016
Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=<…
2a6c3bb 
…try>

Script thread with no root document

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

This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633.

The last commit is the one that matters, the ones before are #13646.

cc @jdm @Ms2ger @ConnorGBrewster
---
<!-- 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 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/14013)
<!-- Reviewable:end -->
@asajeffrey asajeffrey force-pushed the script-lookup-iframes-by-frameid branch from 7ecee09 to e886199 Compare November 2, 2016 20:53
@asajeffrey
Copy link
Copy Markdown
Contributor Author

asajeffrey commented Nov 2, 2016

Rebased. @bors-servo try

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 2, 2016

⌛ Trying commit e886199 with merge ac21b50...

bors-servo pushed a commit that referenced this pull request Nov 2, 2016
Auto merge of #13646 - asajeffrey:script-lookup-iframes-by-frameid, r…
ac21b50 
…=<try>

Script lookup iframes by frameid

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

Lookup iframes by `FrameId` rather than `PipelineId`. This should make lookup much more reliable, since the frame id doesn't change.

cc @ConnorGBrewster @aneeshusa
---

<!-- 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 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/13646)

<!-- Reviewable:end -->
@asajeffrey asajeffrey added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-needs-rebase There are merge conflict errors. labels Nov 2, 2016
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 2, 2016

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

bors-servo pushed a commit that referenced this pull request Nov 3, 2016
Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=<…
4fe3e6f 
…try>

Script thread with no root document

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

This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633.

The last commit is the one that matters, the ones before are #13646.

cc @jdm @Ms2ger @ConnorGBrewster
---
<!-- 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 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/14013)
<!-- Reviewable:end -->
@asajeffrey asajeffrey removed the S-awaiting-review There is new code that needs to be reviewed. label Nov 3, 2016
@asajeffrey asajeffrey force-pushed the script-lookup-iframes-by-frameid branch from e886199 to f3993d9 Compare November 3, 2016 20:40
@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Nov 3, 2016
@asajeffrey
Copy link
Copy Markdown
Contributor Author

asajeffrey commented Nov 3, 2016

Squashed and rebased @bors-servo r=ConnorGBrewster

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 3, 2016

📌 Commit f3993d9 has been approved by ConnorGBrewster

@highfive highfive assigned cbrewster and unassigned nox Nov 3, 2016
@highfive 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 Nov 3, 2016
bors-servo pushed a commit that referenced this pull request Nov 3, 2016
Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=<…
4aa4b02 
…try>

Script thread with no root document

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

This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633.

The last commit is the one that matters, the ones before are #13646.

cc @jdm @Ms2ger @ConnorGBrewster
---
<!-- 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 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/14013)
<!-- Reviewable:end -->
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 3, 2016

⌛ Testing commit f3993d9 with merge ce725c9...

bors-servo pushed a commit that referenced this pull request Nov 3, 2016
Auto merge of #13646 - asajeffrey:script-lookup-iframes-by-frameid, r…
ce725c9 
…=ConnorGBrewster

Script lookup iframes by frameid

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

Lookup iframes by `FrameId` rather than `PipelineId`. This should make lookup much more reliable, since the frame id doesn't change.

cc @ConnorGBrewster @aneeshusa
---

<!-- 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 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/13646)

<!-- Reviewable:end -->
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Nov 4, 2016

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

@bors-servo bors-servo merged commit f3993d9 into servo:master Nov 4, 2016
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Nov 4, 2016
This was referenced Nov 4, 2016
Merged
Closed
bors-servo pushed a commit that referenced this pull request Nov 8, 2016
Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=jdm
8236d19 
Script thread with no root document

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

This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633.

The last commit is the one that matters, the ones before are #13646.

cc @jdm @Ms2ger @ConnorGBrewster
---
<!-- 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 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/14013)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this pull request Nov 8, 2016
Auto merge of #14013 - asajeffrey:script-thread-no-root-document, r=jdm
d1b7f19 
Script thread with no root document

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

This PR removes the single root document from the script thread, and replaces it by a lookup table from `PipelineId`s to `Document`s. This is needed if we're going to share script threads, as per #633.

The last commit is the one that matters, the ones before are #13646.

cc @jdm @Ms2ger @ConnorGBrewster
---
<!-- 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 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/14013)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@nox nox nox left review comments

@cbrewster cbrewster cbrewster requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@cbrewster cbrewster

Labels

A-constellation Involves the constellation A-content/script Related to the script thread I-refactor No impact; the issue is one of maintainability or tidiness. Proposed solution requires refactoring.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@asajeffrey @highfive @bors-servo @jdm @nox @cbrewster @KiChjang