1434474 - nsIPresShell::RestyleForCSSRuleChanges probably doesn't need to trigger separate font flushes

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Bobby Holley (:bholley)]

This seems to be the cause of the first large stylist update in bug 1434145. Emilio and I discussed, and it's not obvious why we need to do this work, since we could otherwise just let it happen during the eventual style flush.

I'm going to push an experimental patch and see what breaks.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #1)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=dd1973aaaf5bd176b7a5e828683c7dfb260757c3

There's some orange on that push. I'm out of time for the evening, but can look tomorrow if emilio doesn't get to it first.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Bobby Holley (:bholley) from comment #2)
> (In reply to Bobby Holley (:bholley) from comment #1)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=dd1973aaaf5bd176b7a5e828683c7dfb260757c3
> 
> There's some orange on that push. I'm out of time for the evening, but can
> look tomorrow if emilio doesn't get to it first.

You definitely need to mark it as dirty somehow, it's just a matter of it not needing to be off a runnable. I'm running a patch through try right now.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

This is my try, let's see... https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f4a8992704dd0f75754973fef6e800ba1c8295f

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> This is my try, let's see...
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6f4a8992704dd0f75754973fef6e800ba1c8295f

Looks good! Will clean it up.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1434474: There's no need to rebuild font / counter styles / font feature values off a runnable.

Everything that needs them up-to-date will call flush appropriately, there
should be no need to do it manually.

This way we coalesce all the stylist updates until the next style flush in the
best case, or until one of the consumers actually needs them.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8947049 [details]
Bug 1434474: There's no need to rebuild font / counter styles / font feature values off a runnable.

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Here's a try run without the other patches causing the WPT passes.

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

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8947049 [details]
Bug 1434474: There's no need to rebuild font / counter styles / font feature values off a runnable.

Per IRC discussion, my intuition says that we want EnsureStyleFlush here. If we do that, we should make sure that the EnsureStyleFlush path will bail appropriately for !mDidInitialize presshells, or else find some other way to fix the cnn issue.

Redirecting to Boris, who's going to have a look.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8947049 [details]
Bug 1434474: There's no need to rebuild font / counter styles / font feature values off a runnable.

So, summarizing the discussion with bz:

* We need EnsureStyleFlush rather than SetNeedsStyleFlush.
* The Ensure should be in the appropriate callers, rather than in the Mark functions, since not all callers need it.
* We should add comments CreateShell/DeleteShell indicating that we need to mark the fonts as dirty because changing the shell can change which rules apply. But we don't need to do any Ensuring at those sites.

I can stamp the result.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8947049 [details]
Bug 1434474: There's no need to rebuild font / counter styles / font feature values off a runnable.

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

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8947049 [details]
Bug 1434474: There's no need to rebuild font / counter styles / font feature values off a runnable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216850/diff/3-4/

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8947049 [details]
Bug 1434474: There's no need to rebuild font / counter styles / font feature values off a runnable.

https://reviewboard.mozilla.org/r/216850/#review222892

::: layout/style/FontFaceSet.cpp:1986
(Diff revision 4)
>  FontFaceSet::UserFontSet::DoRebuildUserFontSet()
>  {
>    if (!mFontFaceSet) {
>      return;
>    }
> -  mFontFaceSet->RebuildUserFontSet();
> +  mFontFaceSet->MarkUserFontSetDirty();

I don't have time to check, but are you sure we don't need to ensure a flush here?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8947049 [details]
Bug 1434474: There's no need to rebuild font / counter styles / font feature values off a runnable.

https://reviewboard.mozilla.org/r/216850/#review222892

> I don't have time to check, but are you sure we don't need to ensure a flush here?

It goes through FontFaceSet, so it does call EnsureStyleFlush.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8947049 [details]
Bug 1434474: There's no need to rebuild font / counter styles / font feature values off a runnable.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216850/diff/4-5/

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6d21bdf7eab3
There's no need to rebuild font / counter styles / font feature values off a runnable. r=bholley

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6d21bdf7eab3