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

bors-servo commented May 9, 2017

Trying commit 6615974 with merge 8c3b0c3...

bors-servo added 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

bors-servo commented May 9, 2017

💔 Test failed - android

@emilio emilio force-pushed the emilio:snapshots branch from 6615974 to 327d275 May 9, 2017
@highfive highfive removed the S-tests-failed label May 9, 2017
@emilio
Copy link
Member Author

emilio commented May 9, 2017

@bors-servo try

  • lockfile
@bors-servo
Copy link
Contributor

bors-servo commented May 9, 2017

Trying commit 327d275 with merge 1d5a88f...

bors-servo added 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

bors-servo commented May 9, 2017

💔 Test failed - windows-msvc-dev

@emilio emilio force-pushed the emilio:snapshots branch from 327d275 to a1aa368 May 9, 2017
@highfive highfive removed the S-tests-failed label May 9, 2017
@emilio
Copy link
Member Author

emilio commented May 9, 2017

@bors-servo
Copy link
Contributor

bors-servo commented May 9, 2017

Trying commit a1aa368 with merge 87ef22b...

bors-servo added 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

bors-servo commented May 9, 2017

💔 Test failed - android

@emilio
Copy link
Member Author

emilio commented May 9, 2017

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented May 9, 2017

Trying commit a1aa368 with merge eba3525...

bors-servo added 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

bors-servo commented May 9, 2017

💔 Test failed - linux-dev

@emilio emilio force-pushed the emilio:snapshots branch from a1aa368 to d4c8b25 May 9, 2017
@emilio emilio force-pushed the emilio:snapshots branch from 08376f0 to 5b7fa76 May 10, 2017
emilio added 4 commits Apr 16, 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 emilio force-pushed the emilio:snapshots branch from 5b7fa76 to e6fc0aa May 10, 2017
@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

bors-servo commented May 10, 2017

📌 Commit e6fc0aa has been approved by bholley

@bors-servo
Copy link
Contributor

bors-servo commented May 10, 2017

Testing commit e6fc0aa with merge 9bc0f24...

bors-servo added 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

bors-servo commented May 10, 2017

💔 Test failed - mac-dev-unit

@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

bors-servo commented May 10, 2017

📌 Commit e6fc0aa has been approved by bholley

@bors-servo
Copy link
Contributor

bors-servo commented May 10, 2017

Testing commit e6fc0aa with merge c8171ed...

bors-servo added 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

bors-servo commented May 10, 2017

☀️ 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
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@emilio emilio deleted the emilio:snapshots branch May 17, 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.