Skip to content

/ servo

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

Implement browsing context group and set #23507

Merged
merged 1 commit into from Jun 7, 2019
Merged

Implement browsing context group and set #23507

merged 1 commit into from Jun 7, 2019

Conversation

@gterzian
Copy link
Member

gterzian commented Jun 4, 2019 •
edited by SimonSapin

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #23307 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___

This change is Reviewable
@gterzian gterzian requested a review from asajeffrey Jun 4, 2019
@highfive
Copy link

highfive commented Jun 4, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/constellation.rs
  • @cbrewster: components/constellation/constellation.rs
  • @paulrouget: components/constellation/constellation.rs
@gterzian gterzian force-pushed the gterzian:introduce_browsing_context_groups branch 2 times, most recently from 29b464e to 65ae91e Jun 4, 2019
@gterzian
Copy link
Member Author

gterzian commented Jun 4, 2019

@bors-servo try=wpt
bors-servo added a commit that referenced this pull request Jun 4, 2019
@bors-servo
Auto merge of #23507 - gterzian:introduce_browsing_context_groups, r=…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
53c4343 
…<try>

Implement browsing context group and set

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

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #23307 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/23507)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2019

⌛ Trying commit 65ae91e with merge 53c4343...
@gterzian gterzian force-pushed the gterzian:introduce_browsing_context_groups branch 6 times, most recently from a6c7687 to b287c4c Jun 4, 2019
@gterzian
Copy link
Member Author

gterzian commented Jun 4, 2019

@bors-servo try=wpt (false start)
bors-servo added a commit that referenced this pull request Jun 4, 2019
@bors-servo
Auto merge of #23507 - gterzian:introduce_browsing_context_groups, r=…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
f74ac81 
…<try>

Implement browsing context group and set

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

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #23307 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/23507)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2019

⌛ Trying commit b287c4c with merge f74ac81...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2019

💔 Test failed - linux-rel-css
@gterzian
Copy link
Member Author

gterzian commented Jun 4, 2019

@bors-servo try=wpt
bors-servo added a commit that referenced this pull request Jun 4, 2019
@bors-servo
Auto merge of #23507 - gterzian:introduce_browsing_context_groups, r=…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
b0b9eb2 
…<try>

Implement browsing context group and set

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

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #23307 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/23507)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2019

⌛ Trying commit a1df62d with merge b0b9eb2...
@asajeffrey
asajeffrey requested changes Jun 4, 2019
View changes
Copy link
Member

asajeffrey left a comment

Mostly looks good! I'm glad to see the constellation getting freshened with new bits of the spec. The main change is to avoid panic in the constellation.
components/constellation/constellation.rs Outdated Show resolved Hide resolved
components/constellation/constellation.rs Outdated Show resolved Hide resolved
components/constellation/constellation.rs Outdated Show resolved Hide resolved
components/constellation/constellation.rs Outdated Show resolved Hide resolved
components/constellation/constellation.rs Outdated
host: &Host,
top_level_browsing_context_id: &TopLevelBrowsingContextId,
opener: &Option<BrowsingContextId>,
) -> Option<Weak<EventLoop>> {

This comment has been minimized.

Sign in to view
Copy link
@asajeffrey

asajeffrey Jun 4, 2019

Member

If this returned a Result then you could use ? notation.

This comment has been minimized.

Sign in to view
Copy link
@gterzian

gterzian Jun 4, 2019 •
edited

Author Member

I don't know if that would be useful, since I added a few warn statements in the None cases...

This comment has been minimized.

Sign in to view
Copy link
@asajeffrey

asajeffrey Jun 4, 2019

Member

Well you could make the return type Result<Weak<EventLoop>,&'static str> and then replace the match foo { ... } by foo.ok_or("a warning message")? Then put the logic to warn! in the caller of get_event_loop. Not a huge deal, just might make the code a bit shorter and easier to read.

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Jun 4, 2019

Member

? works with Option as well, unless I'm misunderstanding the suggestion here.

This comment has been minimized.

Sign in to view
Copy link
@asajeffrey

asajeffrey Jun 4, 2019

Member

True, but then you couldn't return a warning message.

This comment has been minimized.

Sign in to view
Copy link
@gterzian

gterzian Jun 6, 2019

Author Member

Ok, I'll look into it, sorry I missed those responses earlier...
components/constellation/constellation.rs Outdated Show resolved Hide resolved
components/constellation/constellation.rs Outdated Show resolved Hide resolved
components/constellation/constellation.rs Show resolved Hide resolved
@asajeffrey
Copy link
Member

asajeffrey commented Jun 4, 2019

@paulrouget does this impact the work on multiple windows?
@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True
@gterzian gterzian force-pushed the gterzian:introduce_browsing_context_groups branch from 7e65b30 to 69145af Jun 4, 2019
@gterzian
Copy link
Member Author

gterzian commented Jun 4, 2019

@bors-servo try=wpt (should still be ok)
bors-servo added a commit that referenced this pull request Jun 4, 2019
@bors-servo
Auto merge of #23507 - gterzian:introduce_browsing_context_groups, r=…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
bc58564 
…<try>

Implement browsing context group and set

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

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #23307 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/23507)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2019

⌛ Trying commit 15a623d with merge a54fd6d...
@gterzian
Copy link
Member Author

gterzian commented Jun 4, 2019

By the way this also addresses @nox concern at the time of implementing window.open, since these changes would restrict sharing an event-loop to same-origin opener-openee type of relationship(essentially sharing a similar-origin-window-agent same-agent-window-objects), as opposed to running all same origin pages on the same event-loop...
@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True
@gterzian gterzian force-pushed the gterzian:introduce_browsing_context_groups branch from 15a623d to a981010 Jun 5, 2019
@gterzian
Copy link
Member Author

gterzian commented Jun 5, 2019

Ok, squashed and ready for another review...
@gterzian
Copy link
Member Author

gterzian commented Jun 5, 2019

The failure on CI appears unrelated(download error for a crate)
@asajeffrey
Copy link
Member

asajeffrey commented Jun 5, 2019

Yay, no panic! The only remaining question is whether to return a Result<..., &'static str> and use ? notation. Personally I would, but it's up to you. You can r=me.
@jdm
Copy link
Member

jdm commented Jun 5, 2019

@bors-servo delegate=gterzian
@bors-servo
Copy link
Contributor

bors-servo commented Jun 5, 2019

✌️ @gterzian can now approve this pull request
@gterzian
gterzian reviewed Jun 6, 2019
View changes
components/constellation/constellation.rs Outdated
let _ = self
.event_loops
.insert(host, Rc::downgrade(&pipeline.pipeline.event_loop));
let _ = self.set_event_loop(

This comment has been minimized.

Sign in to view
Copy link
@gterzian

gterzian Jun 6, 2019

Author Member

note: the let is unnecessary here
@paulrouget
Copy link
Contributor

paulrouget commented Jun 6, 2019

does this impact the work on multiple windows?

No.
@gterzian
Copy link
Member Author

gterzian commented Jun 6, 2019

@asajeffrey Could you take a look at the last commit please?
@asajeffrey
Copy link
Member

asajeffrey commented Jun 6, 2019

LGTM!
@gterzian
implement browsing context group and set
Loading status checks…
8c28852
@gterzian gterzian force-pushed the gterzian:introduce_browsing_context_groups branch from 9a178e9 to 8c28852 Jun 7, 2019
@gterzian
Copy link
Member Author

gterzian commented Jun 7, 2019

@bors-servo r=asajeffrey
@bors-servo
Copy link
Contributor

bors-servo commented Jun 7, 2019

📌 Commit 8c28852 has been approved by asajeffrey
@bors-servo
Copy link
Contributor

bors-servo commented Jun 7, 2019

⌛ Testing commit 8c28852 with merge 525d515...
bors-servo added a commit that referenced this pull request Jun 7, 2019
@bors-servo
Auto merge of #23507 - gterzian:introduce_browsing_context_groups, r=…
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
525d515 
…asajeffrey

Implement browsing context group and set

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

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./mach test-tidy` does not report any errors
- [ ] These changes fix #23307 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/23507)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 7, 2019

☀️ Test successful - arm64, linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: asajeffrey
Pushing 525d515 to master...
@bors-servo bors-servo merged commit 8c28852 into servo:master Jun 7, 2019
3 of 4 checks passed
3 of 4 checks passed
Travis CI - Pull Request Build Failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@gterzian gterzian deleted the gterzian:introduce_browsing_context_groups branch Jun 7, 2019
@gterzian gterzian mentioned this pull request Sep 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdm jdm

@asajeffrey asajeffrey

Assignees

@asajeffrey asajeffrey

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Make groupings of browsing context comply with spec changes
7 participants
@gterzian @highfive @bors-servo @asajeffrey @jdm @paulrouget @ferjm
You can’t perform that action at this time.