1364824 - stylo: Implement PostRebuildAllStyleDataEvent

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+10]

@counter-style needs a mechanism to invalidate all style data (recompute all data in style structs) after rule changes. It currently uses PostRebuildAllStyleDataEvent in Gecko to do so asynchronously from nsPresContext::FlushCounterStyles. We probably need something similar in Stylo.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

emilio, could you provide some suggestion on how should we do this?

It seems to me RebuildAllStyleData in ServoRestyleManager is actually async somehow. I wonder whether we can just duplicate some of its code to Post*Event one...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

You mean async in the sense that the restyle is posted asynchronously? If so, yes.

If you want it synchronously you should be able to do:

    RebuildAllStyleData();
    ProcessPendingRestyles();

Would that work for you?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1364824: Implement ServoRestyleManager::PostRebuildAllStyleDataEvent.

Signed-off-by: Emilio Cobos Álvarez <[e-mail]>

Review commit: https://reviewboard.mozilla.org/r/139188/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139188/

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8867617 [details]
Bug 1364824: Implement ServoRestyleManager::PostRebuildAllStyleDataEvent.

https://reviewboard.mozilla.org/r/139188/#review142416

::: commit-message-72dbf:4
(Diff revision 1)
> +Bug 1364824: Implement ServoRestyleManager::PostRebuildAllStyleDataEvent. r?heycam
> +
> +MozReview-Commit-ID: I9HmM9ZMB2c
> +Signed-off-by: Emilio Cobos Álvarez <[e-mail]>

Probably don't need this line.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1364824: Make RebuildAllStyleData's flush async too.

If this turns out to be problematic, I won't land it, but I think it's worth
trying.

Review commit: https://reviewboard.mozilla.org/r/139190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139190/

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8867624 [details]
Bug 1364824: Make RebuildAllStyleData's flush async too.

https://reviewboard.mozilla.org/r/139190/#review142418

OK, let's try.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8867624 [details]
Bug 1364824: Make RebuildAllStyleData's flush async too.

https://reviewboard.mozilla.org/r/139190/#review142468

::: servo/components/style/gecko/data.rs:92
(Diff revision 1)
>  
>  impl PerDocumentStyleDataImpl {
>      /// Reset the device state because it may have changed.
>      ///
>      /// Implies also a stylesheet flush.
>      pub fn reset_device(&mut self, guard: &SharedRwLockReadGuard) {

> error: unused variable: `guard`

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

It seems somehow the patch doesn't quite work :/

The first patch doesn't fix the issue of bug 1328319, and the second patch has startup crash.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

The second patch doesn't work because I forgot to force_dirty the stylesheets, and because we definitely rely on it being sync (some font-face tests assert without it).

I'll land the first patch because it fixes some font-face tests:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1f30074da4dec0e5f0cdf6fd10d36064d750440

Then file a bug to make the flush async, which requires a few more changes.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8867617 [details]
Bug 1364824: Implement ServoRestyleManager::PostRebuildAllStyleDataEvent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139188/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa66df85a979
Implement ServoRestyleManager::PostRebuildAllStyleDataEvent. r=heycam

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fa66df85a979