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

style: Replicate the list of stylesheets on the layout thread. #18124

Merged
merged 1 commit into from Aug 18, 2017

Conversation

@emilio
Copy link
Member

emilio commented Aug 17, 2017

This is a patch that unifies a bit how Gecko and Stylo stylesheets work, in
order to be able to eventually move the stylesheets into the stylist, and be
able to incrementally update the invalidation map.

This is on top of #18113


This change is Reviewable

@highfive
Copy link

highfive commented Aug 17, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/invalidation/stylesheets.rs, components/style/stylesheet_set.rs, components/style/stylist.rs
  • @canaltinova: components/style/invalidation/stylesheets.rs, components/style/stylesheet_set.rs, components/style/stylist.rs
  • @fitzgen: components/script/dom/document.rs, components/script/dom/htmllinkelement.rs, components/script/dom/node.rs, components/script/dom/htmlstyleelement.rs, components/script/dom/bindings/trace.rs and 4 more
  • @KiChjang: components/script/dom/document.rs, components/script/dom/htmllinkelement.rs, components/script/dom/node.rs, components/script/dom/htmlstyleelement.rs, components/script/dom/bindings/trace.rs and 4 more
@highfive
Copy link

highfive commented Aug 17, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify style and script code, but no tests are modified. Please consider adding a test!
@emilio
Copy link
Member Author

emilio commented Aug 17, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Aug 17, 2017

Trying commit e34c51d with merge 73d1357...

bors-servo added a commit that referenced this pull request Aug 17, 2017
style: Replicate the list of stylesheets on the layout thread.

This is a patch that unifies a bit how Gecko and Stylo stylesheets work, in
order to be able to eventually move the stylesheets into the stylist, and be
able to incrementally update the invalidation map.

This is on top of #18113

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

bors-servo commented Aug 17, 2017

💔 Test failed - android

@emilio
Copy link
Member Author

emilio commented Aug 17, 2017

@highfive highfive assigned SimonSapin and unassigned metajack Aug 17, 2017
@emilio emilio force-pushed the emilio:stylist-stylesheets branch from e34c51d to 9dcb7d1 Aug 17, 2017
@SimonSapin
Copy link
Member

SimonSapin commented Aug 17, 2017

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 13 of 13 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


components/script/dom/document.rs, line 2399 at r4 (raw file):

        let mut stylesheets = self.stylesheets.borrow_mut();
        let insertion_point = stylesheets.iter().find(|sheet_in_doc| {
            sheet_in_doc.owner.upcast::<Node>().is_before(owner.upcast())

Shouldn’t this be owner.upcast::<Node>().is_before(sheet_in_doc.owner.upcast())?


components/script/dom/node.rs, line 2395 at r2 (raw file):

        }

        let mut parent = self_and_ancestors.pop().unwrap();

Nit: parent is not the parent of any particular node we’ve been manipulating so far. Maybe call it shared_ancestor?


Comments from Reviewable

@emilio emilio force-pushed the emilio:stylist-stylesheets branch from 9dcb7d1 to 1394a0e Aug 17, 2017
@SimonSapin
Copy link
Member

SimonSapin commented Aug 17, 2017

Shouldn’t this be owner.upcast::().is_before(sheet_in_doc.owner.upcast())?

r=me with that fixed.

@emilio
Copy link
Member Author

emilio commented Aug 17, 2017

Great catch, Simon :)

@bors-servo try

@bors-servo
Copy link
Contributor

bors-servo commented Aug 17, 2017

Trying commit 1394a0e with merge 9a5b0a4...

bors-servo added a commit that referenced this pull request Aug 17, 2017
style: Replicate the list of stylesheets on the layout thread.

This is a patch that unifies a bit how Gecko and Stylo stylesheets work, in
order to be able to eventually move the stylesheets into the stylist, and be
able to incrementally update the invalidation map.

This is on top of #18113

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

bors-servo commented Aug 17, 2017

💔 Test failed - linux-dev

@SimonSapin
Copy link
Member

SimonSapin commented Aug 17, 2017

Checking files for tidiness...

./components/script/dom/document.rs:142: use statement is not in alphabetical order
	expected: style::shared_lock::SharedRwLockReadGuard
	found: style::shared_lock::SharedRwLock as StyleSharedRwLock

./components/script/dom/document.rs:145: use statement is not in alphabetical order
	expected: style::stylesheets::{Stylesheet, StylesheetContents, OriginSet}
	found: style::stylesheet_set::StylesheetSet

./components/layout_thread/lib.rs:135: use statement is not in alphabetical order
	expected: style::properties::PropertyId
	found: style::invalidation::media_queries::{MediaListKey, ToMediaListKey}

./components/layout_thread/lib.rs:140: use statement is not in alphabetical order
	expected: style::stylesheets::{Origin, OriginSet, Stylesheet, StylesheetContents, StylesheetInDocument, UserAgentStylesheets}
	found: style::stylesheet_set::StylesheetSet
@emilio emilio force-pushed the emilio:stylist-stylesheets branch from 1394a0e to 3446372 Aug 17, 2017
@emilio
Copy link
Member Author

emilio commented Aug 17, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Aug 17, 2017

Trying commit 3446372 with merge 307478b...

bors-servo added a commit that referenced this pull request Aug 17, 2017
style: Replicate the list of stylesheets on the layout thread.

This is a patch that unifies a bit how Gecko and Stylo stylesheets work, in
order to be able to eventually move the stylesheets into the stylist, and be
able to incrementally update the invalidation map.

This is on top of #18113

<!-- 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/18124)
<!-- Reviewable:end -->
@emilio
Copy link
Member Author

emilio commented Aug 17, 2017

@bors-servo r=SimonSapin p=1

  • It's been 2 hours on try :(
@bors-servo
Copy link
Contributor

bors-servo commented Aug 17, 2017

📌 Commit 3446372 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Aug 18, 2017

📌 Commit 1ddef08 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Aug 18, 2017

Testing commit 1ddef08 with merge 495a083...

bors-servo added a commit that referenced this pull request Aug 18, 2017
style: Replicate the list of stylesheets on the layout thread.

This is a patch that unifies a bit how Gecko and Stylo stylesheets work, in
order to be able to eventually move the stylesheets into the stylist, and be
able to incrementally update the invalidation map.

This is on top of #18113

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

bors-servo commented Aug 18, 2017

💔 Test failed - mac-rel-wpt3

@emilio
Copy link
Member Author

emilio commented Aug 18, 2017

@bors-servo retry

  • Generic exception?
@bors-servo
Copy link
Contributor

bors-servo commented Aug 18, 2017

Testing commit 1ddef08 with merge bc4b67d...

bors-servo added a commit that referenced this pull request Aug 18, 2017
style: Replicate the list of stylesheets on the layout thread.

This is a patch that unifies a bit how Gecko and Stylo stylesheets work, in
order to be able to eventually move the stylesheets into the stylist, and be
able to incrementally update the invalidation map.

This is on top of #18113

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

bors-servo commented Aug 18, 2017

💔 Test failed - mac-rel-wpt4

This is a patch that unifies a bit how Gecko and Stylo stylesheets work, in
order to be able to eventually move the stylesheets into the stylist, and be
able to incrementally update the invalidation map.
@emilio emilio force-pushed the emilio:stylist-stylesheets branch from 1ddef08 to d1725b1 Aug 18, 2017
@emilio
Copy link
Member Author

emilio commented Aug 18, 2017

@bors-servo r=SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Aug 18, 2017

📌 Commit d1725b1 has been approved by SimonSapin

@bors-servo
Copy link
Contributor

bors-servo commented Aug 18, 2017

Testing commit d1725b1 with merge 81a382c...

bors-servo added a commit that referenced this pull request Aug 18, 2017
style: Replicate the list of stylesheets on the layout thread.

This is a patch that unifies a bit how Gecko and Stylo stylesheets work, in
order to be able to eventually move the stylesheets into the stylist, and be
able to incrementally update the invalidation map.

This is on top of #18113

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

bors-servo commented Aug 18, 2017

@bors-servo bors-servo merged commit d1725b1 into servo:master Aug 18, 2017
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
@emilio emilio deleted the emilio:stylist-stylesheets branch Aug 18, 2017
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

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