Closed
Bug 1435214
Opened 7 years ago
Closed 7 years ago
Make stylesheet insertions that have @keyframes and @font-face rules optimizable
Categories
(Core :: CSS Parsing and Computation, enhancement)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: emilio, Assigned: emilio)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8947769 [details]
Bug 1435214: Optimize @keyframes rule insertions.
https://reviewboard.mozilla.org/r/217486/#review223280
Looks pretty good! One thing I am wondering is that we can clear the hash table when we really need to reduild whole style data? I mean RebuildAllStyleData().
Attachment #8947769 -
Flags: review?(hikezoe) → review+
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8947768 [details]
style: Make insertion of @font-face rules not necessarily restyle the whole document.
https://reviewboard.mozilla.org/r/217484/#review223286
Thinking a bit more about it, this is actually not correct.
Firstly, if we do not restyle the document, how would we kick off the font downloading if there is any matching in the document? IIRC, font is loaded lazily when characters are actually requested.
In addition, when a font becomes listed in `@font-face` rule, we usually need to draw invisible text instead of going to fallback directly (the `font-display: block` behavior by default), and if we do not restyle, we would not be able to do so. That would also make the page rendering more racy (font rendering would further depend on when the stylesheet is loaded).
So I think we probably cannot take this optimization, and we should have test against it (maybe it already fails some test?)
Attachment #8947768 -
Flags: review?(xidorn+moz) → review-
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8947769 [details]
Bug 1435214: Optimize @keyframes rule insertions.
https://reviewboard.mozilla.org/r/217486/#review223288
It's getting too late here now... I'll probably review it tomorrow, but definitely not today... some comments first.
::: layout/style/nsAnimationManager.h:350
(Diff revision 1)
> return false;
> }
>
> + bool AnimationMayBeReferenced(nsAtom* aName) const
> + {
> + return mMaybeReferencedAnimations.GetEntry(aName);
Using `Contains` instead should make it clearer here (although internally `Contains` just calls into `GetEntry`).
::: layout/style/nsAnimationManager.h:359
(Diff revision 1)
> + // This includes names for both animations that are running and @keyframes
> + // which have not been found, and is used for style invalidation (so it's not
> + // needed to keep it always up-to-date).
Maybe something like this is clearer:
> This includes all animaton names referenced regardless of whether a corresponding `@keyframes` rule is available.
>
> It may contain names which is no longer referenced, but it should always contain names which is currently referenced, so that it is usable for style invalidation.
Assignee | ||
Comment 6•7 years ago
|
||
Comment on attachment 8947768 [details]
style: Make insertion of @font-face rules not necessarily restyle the whole document.
Per IRC discussion, I think this patch is right.
Font loads are triggered from layout, and[1] is what makes sure that we reflow the page when @font-face rules are added (and recascade).
There's no need to do a full doc restyle, and there'd be no need to recascade if there wasn't any Ex / Ch unit, but that's another matter.
[1]: https://searchfox.org/mozilla-central/rev/eeb7190f9ad6f1a846cd6df09986325b3f2c3117/layout/base/nsPresContext.cpp#2357
Attachment #8947768 -
Flags: review- → review?(xidorn+moz)
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Comment on attachment 8947769 [details]
> Bug 1435214: Optimize @keyframes rule insertions.
>
> https://reviewboard.mozilla.org/r/217486/#review223280
>
> Looks pretty good! One thing I am wondering is that we can clear the hash
> table when we really need to reduild whole style data? I mean
> RebuildAllStyleData().
Does RebuildAllStyleData (with a non-zero restyle-hint) guarantee that we'll rebuild all the animations? If not I doubt I can do this.
Flags: needinfo?(hikezoe)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8947769 [details]
Bug 1435214: Optimize @keyframes rule insertions.
https://reviewboard.mozilla.org/r/217486/#review223288
> Using `Contains` instead should make it clearer here (although internally `Contains` just calls into `GetEntry`).
Done, agreed :)
> Maybe something like this is clearer:
> > This includes all animaton names referenced regardless of whether a corresponding `@keyframes` rule is available.
> >
> > It may contain names which is no longer referenced, but it should always contain names which is currently referenced, so that it is usable for style invalidation.
I like it! I added that comment :)
Assignee | ||
Comment 11•7 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> Comment on attachment 8947769 [details]
> Bug 1435214: Optimize @keyframes rule insertions.
>
> https://reviewboard.mozilla.org/r/217486/#review223280
>
> Looks pretty good! One thing I am wondering is that we can clear the hash
> table when we really need to reduild whole style data? I mean
> RebuildAllStyleData().
We can probably do something like:
diff --git a/layout/base/ServoRestyleManager.cpp b/layout/base/ServoRestyleManager.cpp
index fab5f4d12d09..6d846781f9af 100644
--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@@ -1136,6 +1136,15 @@ ServoRestyleManager::DoProcessPendingRestyles(ServoTraversalFlags aFlags)
}
if (mRestyleForCSSRuleChanges) {
+ styleSet->UpdateStylist();
+ nsINode* restyleRoot = mDocument->GetServoRestyleRoot();
+ if (root == mDocument || root == mDocument->GetRootElement()) {
+ // We're going to restyle the whole document and all the animations will
+ // be rebuilt, so now we can clear the animation manager's record of which
+ // animations are referenced.
+ presContext->AnimationManager()->ClearMaybeReferencedAnimations();
+ }
+
aFlags |= ServoTraversalFlags::ForCSSRuleChanges;
}
But it's not clear to me it's worth
Comment 12•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #7)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #3)
> > Comment on attachment 8947769 [details]
> > Bug 1435214: Optimize @keyframes rule insertions.
> >
> > https://reviewboard.mozilla.org/r/217486/#review223280
> >
> > Looks pretty good! One thing I am wondering is that we can clear the hash
> > table when we really need to reduild whole style data? I mean
> > RebuildAllStyleData().
>
> Does RebuildAllStyleData (with a non-zero restyle-hint) guarantee that we'll
> rebuild all the animations? If not I doubt I can do this.
We try to rebuild all animations (i.e. UpdateAnimations is called) when we meet TraversalFlags::ForCSSRuleChanges during traversal, IIRC.
Flags: needinfo?(hikezoe)
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8947768 [details]
style: Make insertion of @font-face rules not necessarily restyle the whole document.
https://reviewboard.mozilla.org/r/217484/#review223512
OK... So we don't need to restyle, because we don't store any actual reference to font faces in the computed style. We need a reflow to kick off the necessary download, which will be triggered by nsPresContext::UserFontSetUpdated (which will also trigger a restyle anyway).
Attachment #8947768 -
Flags: review?(xidorn+moz) → review+
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8947769 [details]
Bug 1435214: Optimize @keyframes rule insertions.
https://reviewboard.mozilla.org/r/217486/#review223514
Looks good, thanks.
::: layout/style/nsAnimationManager.h:363
(Diff revision 3)
> + // corresponding `@keyframes` rule is available.
> + //
> + // It may contain names which are no longer referenced, but it should always
> + // contain names which are currently referenced, so that it is usable for
> + // style invalidation.
> + nsTHashtable<nsRefPtrHashKey<nsAtom>> mMaybeReferencedAnimations;
Hmmm, so it looks like we actually never clear this set in the lifetime of the document... which is probably fine.
Attachment #8947769 -
Flags: review?(xidorn+moz) → review+
Comment 15•7 years ago
|
||
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4fef754d2761
Optimize @keyframes rule insertions. r=xidorn,hiro
Comment 16•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•