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

Take all the snapshots into account in the style system #16778

Merged
merged 4 commits into from May 10, 2017

Conversation

emilio
Copy link
Member

@emilio emilio commented May 9, 2017

See bug 1355343.

The servo part of this patch presumably needs some polishing, let's see.


This change is Reviewable

@highfive
Copy link

highfive commented May 9, 2017

Heads up! This PR modifies the following files:

  • @bholley: components/style/Cargo.toml, components/style/build_gecko.rs, components/style/data.rs, components/style/gecko/selector_parser.rs, components/style/servo/selector_parser.rs and 9 more
  • @fitzgen: components/script/dom/node.rs, components/script/layout_wrapper.rs
  • @KiChjang: components/script/dom/node.rs, components/script/layout_wrapper.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 9, 2017
@highfive
Copy link

highfive commented May 9, 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 May 9, 2017

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit 6615974 with merge 8c3b0c3...

bors-servo pushed a commit that referenced this pull request May 9, 2017
Take all the snapshots into account in the style system

See [bug 1355343](https://bugzilla.mozilla.org/show_bug.cgi?id=1355343).

The servo part of this patch presumably needs some polishing, let's see.
@bors-servo
Copy link
Contributor

💔 Test failed - android

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label May 9, 2017
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label May 9, 2017
@emilio
Copy link
Member Author

emilio commented May 9, 2017

@bors-servo try

  • lockfile

@bors-servo
Copy link
Contributor

⌛ Trying commit 327d275 with merge 1d5a88f...

bors-servo pushed a commit that referenced this pull request May 9, 2017
Take all the snapshots into account in the style system

See [bug 1355343](https://bugzilla.mozilla.org/show_bug.cgi?id=1355343).

The servo part of this patch presumably needs some polishing, let's see.

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

💔 Test failed - windows-msvc-dev

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label May 9, 2017
@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label May 9, 2017
@emilio
Copy link
Member Author

emilio commented May 9, 2017

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit a1aa368 with merge 87ef22b...

bors-servo pushed a commit that referenced this pull request May 9, 2017
Take all the snapshots into account in the style system

See [bug 1355343](https://bugzilla.mozilla.org/show_bug.cgi?id=1355343).

The servo part of this patch presumably needs some polishing, let's see.

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

💔 Test failed - android

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label May 9, 2017
@emilio
Copy link
Member Author

emilio commented May 9, 2017

@bors-servo retry

@bors-servo
Copy link
Contributor

⌛ Trying commit a1aa368 with merge eba3525...

bors-servo pushed a commit that referenced this pull request May 9, 2017
Take all the snapshots into account in the style system

See [bug 1355343](https://bugzilla.mozilla.org/show_bug.cgi?id=1355343).

The servo part of this patch presumably needs some polishing, let's see.

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

💔 Test failed - linux-dev

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label May 9, 2017
@highfive 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 May 10, 2017
…shot handling. r=bholley

MozReview-Commit-ID: 6OrUKX5RcBq
Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>
I've chosen this approach mainly because there's no other good way to guarantee
the model is correct than holding the snapshots alive until a style refresh.

What I tried before this (storing them in a sort of "immutable element data") is
a pain, since we call into style from the frame constructor and other content
notifications, which makes keeping track of which snapshots should be cleared an
which shouldn't an insane task.

Ideally we'd have a single entry-point for style, but that's not the case right
now, and changing that requires pretty non-trivial changes to the frame
constructor.

MozReview-Commit-ID: FF1KWZv2iBM
Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>
This is likely to be important at least for the initial element.

For the rest of them this is likely useless because we create them in a
throwaway fashion.

MozReview-Commit-ID: EFz9WUdB8S0
Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>
MozReview-Commit-ID: Dyyvo6YMTcL
Signed-off-by: Emilio Cobos Álvarez <emilio@crisal.io>
@emilio
Copy link
Member Author

emilio commented May 10, 2017

@bors-servo r=bholley

  • Bogus flags were changing unexpected things under my feet.

@bors-servo
Copy link
Contributor

📌 Commit e6fc0aa has been approved by bholley

@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-rebase There are merge conflict errors. labels May 10, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit e6fc0aa with merge 9bc0f24...

bors-servo pushed a commit that referenced this pull request May 10, 2017
Take all the snapshots into account in the style system

See [bug 1355343](https://bugzilla.mozilla.org/show_bug.cgi?id=1355343).

The servo part of this patch presumably needs some polishing, let's see.

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

💔 Test failed - mac-dev-unit

@highfive 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 May 10, 2017
@jdm
Copy link
Member

jdm commented May 10, 2017

@bholley
Copy link
Contributor

bholley commented May 10, 2017

@bors-servo r-

We shouldn't land this until the VCS sync service is restored.

@emilio
Copy link
Member Author

emilio commented May 10, 2017

@bors-servo r=bholley p=10

@bors-servo
Copy link
Contributor

📌 Commit e6fc0aa has been approved by bholley

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels May 10, 2017
@bors-servo
Copy link
Contributor

⌛ Testing commit e6fc0aa with merge c8171ed...

bors-servo pushed a commit that referenced this pull request May 10, 2017
Take all the snapshots into account in the style system

See [bug 1355343](https://bugzilla.mozilla.org/show_bug.cgi?id=1355343).

The servo part of this patch presumably needs some polishing, let's see.

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

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: bholley
Pushing c8171ed to master...

@bors-servo bors-servo merged commit e6fc0aa into servo:master May 10, 2017
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label May 10, 2017
@emilio emilio deleted the snapshots branch May 17, 2017 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants