Closed Bug 906643 Opened 6 years ago Closed 6 years ago

Support animation in SVG glyphs

Categories

(Core :: Graphics: Text, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: roc, Assigned: roc)

References

Details

Attachments

(7 files, 9 obsolete files)

3.60 KB, patch
mats
: review+
Details | Diff | Splinter Review
6.00 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.13 KB, patch
mats
: review+
Details | Diff | Splinter Review
18.64 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
4.30 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
5.13 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
7.06 KB, patch
jfkthame
: review+
Details | Diff | Splinter Review
No description provided.
The basic plan here:
-- Any kind of update to the rendering of an SVG glyphs document causes its refresh driver to tick.
-- We add a new hook to the refresh driver that runs a callback after each tick.
-- gfxSVGGlyphsDocument uses this hook; this is how we detect that the document has changed. (SVG images should use this too; the current way SVG images detect animation misses CSS animations, animated images and maybe other things.)
-- Every time the refresh driver ticks, gfxSVGGlyphs notifies its gfxFontEntry that glyphs have changed rendering/geometry.
-- The gfxFontEntry notifies any of its gfxFonts that have used SVG glyphs that glyphs have changed.
-- Each such gfxFont that's a user font (which will be all of them, until platforms support SVG fonts) gets the gfxUserFontSet from each of its gfxFontGroups and notifies it.
-- The gfxUserFontSet (which is actually an nsFontFaceLoader) tells its prescontext to reflow and invalidate everything.

We should probably remove the user-font restriction at some point, especially for FirefoxOS.  However this is not easy to do since other than the gfxUserFontSet we don't have a route to the prescontexts using the fonts. We'll have to move that to gfxFontGroup probably.

The last step is pretty expensive. It could be reduced to at least a simple traversal of the frame tree looking for text frames that are using the relevant gfxFontGroup. However we'd want to coalesce updates for the case where a single SVG-glyphs font is used with multiple gfxFontGroups in the same document.

It's important that every prescontext using text with animated glyphs is invalidated and has its reflow dirtied before the original glyphs document refresh driver tick returns to the event loop. Otherwise the document rendering could get out of sync with the glyphs (e.g. the overflow areas of frames might not match the rendering of their text).
Note that we could avoid involving gfxFont here by having user-font gfxFontEntry be responsible for tracking its gfxFontGroups. I'll attach the patches for the plan above, but if going directly from gfxFontEntry to gfxFontGroups is better I can change to that.
Using nsIRunnable here is a bit clunky, but it lets NS_NewRunnableMethod be used. Let me know if you want a dedicated base class instead.
Attachment #792179 - Flags: review?(dbaron)
A try push is building this code (before I restructured the patch queue) here:
https://tbpl.mozilla.org/?tree=Try&rev=3354d4190694
Comment on attachment 792179 [details] [diff] [review]
Part 1: Add nsRefreshDriver::SetPostRefreshObserver so we can receive notifications when a refresh has occurred

Could you explain:

 * why this is a generic mechanism (taking nsIRunnable), yet

 * it only allows a single observer?

(I'd probably go for less-generic.)
Attachment #792179 - Flags: review?(dbaron) → review-
Comment on attachment 792183 [details] [diff] [review]
Part 5: Make nsPresContext::RefreshDriver() accessible outside of layout

Is exposing nsRefreshDriver outside layout really what we want long term?
It seems like a heavy price to pay...  If it's only for the one call to
SetPostRefreshObserver in part 7 then maybe we could add a
SetPostRefreshObserver method on nsIPresShell instead that proxies the
call to the RefreshDriver?  (like we do for AddRefreshObserver)

Or could we instead allow just gfx/ to use MOZILLA_INTERNAL_API somehow?
(given the increasingly tighter bonds between layout and gfx code)

I wouldn't feel comfortable with other modules using nsRefreshDriver
methods directly.
Attachment #792183 - Flags: review?(matspal) → review-
Comment on attachment 792180 [details] [diff] [review]
Part 2: Add gfxUserFontSet::NotifyGlyphsChanged callback that we can use to force reflow/repainting in response to glyph changes

r=mats  (I filed bug 906852 on some existing style issues)
Attachment #792180 - Flags: review?(matspal) → review+
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #11)
> Could you explain:
> 
>  * why this is a generic mechanism (taking nsIRunnable), yet
> 
>  * it only allows a single observer?
> 
> (I'd probably go for less-generic.)

Do you want a less-generic name, or just a non-generic callback base class?

Can I call it nsAPostRefreshObserver, and call the method SetPostRefreshObserver?
Flags: needinfo?(dbaron)
(In reply to Mats Palmgren (:mats) from comment #12)
> Is exposing nsRefreshDriver outside layout really what we want long term?
> It seems like a heavy price to pay...  If it's only for the one call to
> SetPostRefreshObserver in part 7 then maybe we could add a
> SetPostRefreshObserver method on nsIPresShell instead that proxies the
> call to the RefreshDriver?  (like we do for AddRefreshObserver)

Let's do that.
No, my concern was that you're adding a generic-sounding mechanism (post-refresh observer) yet on the assumption that there's only going to be one of them, ever.  Then, I presume, you're adding one somewhere.  (That said, I didn't find which patch calls SetPostRefreshObserver.)  So there's an implicit assumption that the post-refresh observer is the one thing that you're making it -- if somebody else added another one, they'd either overwrite yours or yours would overwrite theirs, perhaps conditionally on the two features both being used in the same page.  So I'd rather have that assumption explicit.
Flags: needinfo?(dbaron)
The two usages I have in mind are observing changes to an SVG document used as an image and observing changes to an SVG document used in a font. They can't happen at the same time for the same document. I'm not sure what to call the abstraction they have in common. Should I just allow multiple callbacks? I think that'd be absolutely fine.
Flags: needinfo?(dbaron)
I'd be fine with multiple callbacks; I'd also be find with calling it something like the "container observer" that says it's only for the thing that loaded the document.

(Speaking of which:  is SVG-as-image or -as-font driving its own refresh cycle, or is (I hope) its parent be driving that cycle?)
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #18)
> (Speaking of which:  is SVG-as-image or -as-font driving its own refresh
> cycle, or is (I hope) its parent be driving that cycle?)

It has its own refresh driver cycle, because these resources can be shared by more than one parent.
Comment on attachment 792183 [details] [diff] [review]
Part 5: Make nsPresContext::RefreshDriver() accessible outside of layout

We won't need this.
Attachment #792183 - Attachment is obsolete: true
Attached patch Part 1 v2Splinter Review
Attachment #792179 - Attachment is obsolete: true
Attachment #794606 - Flags: review?(dbaron)
Attached patch Part 7 v2Splinter Review
Attachment #792186 - Attachment is obsolete: true
Attachment #792186 - Flags: review?(jfkthame)
Attachment #794608 - Flags: review?(jfkthame)
Comment on attachment 794607 [details] [diff] [review]
Part 5: Expose Add/RemovePostRefreshObserver through nsIPresShell

r=mats
Attachment #794607 - Flags: review?(matspal) → review+
Comment on attachment 792181 [details] [diff] [review]
Part 3: Make each user-font gfxFont track which gfxFontGroups it belongs to

Review of attachment 792181 [details] [diff] [review]:
-----------------------------------------------------------------

I've suggested some renaming that to my mind makes things a bit clearer here. I felt the current naming was a bit confusing, with a persistent implication of a distinct "user font group" object that doesn't actually exist. But see what you think - if you really don't like it, I'm not going to try to insist.

Either way, r=me.

::: gfx/thebes/gfxFont.cpp
@@ +2929,5 @@
> +gfxFont::AddUserFontGroup(gfxFontGroup* aFontGroup)
> +{
> +    if (!mFontEntry->IsUserFont()) {
> +        return;
> +    }

I'm not really happy with this function name (see comment in gfxFont.h). How about making it simply AddFontGroup(), as suggested there, and doing the IsUserFont check at the call site?

@@ +2948,5 @@
> +    }
> +
> +    NS_ASSERTION(mUserFontGroups, "Unbalanced RemoveUserFontSet");
> +    NS_ASSERTION(mUserFontGroups->GetEntry(aFontGroup),
> +                 "Unbalanced RemoveUserFontSet");

Combine these into a single NS_ASSERTION(mUserFontGroups && ....)

@@ +3956,5 @@
> +    }
> +    for (uint32_t i = 0; i < mFonts.Length(); ++i) {
> +        // gfxPangoFonts is responsible for this null check!!!
> +        gfxFont* font = mFonts[i].Font();
> +        if (font) {

if (font && font->GetFontEntry()->IsUserFont()) ....

@@ +3957,5 @@
> +    for (uint32_t i = 0; i < mFonts.Length(); ++i) {
> +        // gfxPangoFonts is responsible for this null check!!!
> +        gfxFont* font = mFonts[i].Font();
> +        if (font) {
> +            mFonts[i].Font()->RemoveUserFontGroup(this);

Re-use your 'font' variable, don't re-fetch it from mFonts here.

@@ +3972,5 @@
> +    for (uint32_t i = 0; i < mFonts.Length(); ++i) {
> +        // gfxPangoFonts is responsible for this null check!!!
> +        gfxFont* font = mFonts[i].Font();
> +        if (font) {
> +            mFonts[i].Font()->AddUserFontGroup(this);

Ditto.

::: gfx/thebes/gfxFont.h
@@ +1713,5 @@
> +    /**
> +     * For user fonts, we track the set of gfxFontGroups that reference this font.
> +     */
> +    void AddUserFontGroup(gfxFontGroup* aFontGroup);
> +    void RemoveUserFontGroup(gfxFontGroup* aFontGroup);

These names make it sound (to me) as though there's some kind of special gfxUserFontGroup object that's being added/removed, but in fact it's just a standard gfxFontGroup.

I'd be inclined to drop "User" from these names, and just let the comment say something like "Currently, only user fonts keep track of the gfxFontGroups that reference them". WDYT?

@@ +1922,5 @@
>  
>      nsExpirationState          mExpirationState;
>      gfxFontStyle               mStyle;
>      nsAutoTArray<gfxGlyphExtents*,1> mGlyphExtentsArray;
> +    nsAutoPtr<nsTHashtable<nsPtrHashKey<gfxFontGroup> > > mUserFontGroups;

As above - there's no such thing as a "user font group". It's just a font group. I think you should call this mFontGroups, and add a comment noting that currently, only user fonts actually create and maintain this table.

@@ +3489,5 @@
> +    // Call RemoveUserFontGroup on all fonts in mFonts
> +    void RemoveFontGroupFromUserFonts();
> +
> +    // Call AddUserFontGroup on all fonts in mFonts
> +    void AddFontGroupToUserFonts();

Drop "User" from these names too, as we'll probably need to track this (or some variation of it) for all fonts eventually.
Attachment #792181 - Flags: review?(jfkthame) → review+
Comment on attachment 792182 [details] [diff] [review]
Part 4: Make each gfxFontEntry track its gfxFonts with SVG glyphs

Review of attachment 792182 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.h
@@ +461,5 @@
>      nsAutoArrayPtr<uint8_t> mUVSData;
>      gfxUserFontData* mUserFontData;
>      gfxSVGGlyphs    *mSVGGlyphs;
> +    // list of gfxFonts that are using SVG glyphs
> +    nsTArray<gfxFont*> mFontListForSVGFonts;

I think I'd prefer mFontsUsingSVGGlyphs, if you're OK with that.
Attachment #792182 - Flags: review?(jfkthame) → review+
Comment on attachment 792184 [details] [diff] [review]
Part 6: Add gfxFontGEntry::NotifyGlyphsChanged, which calls new gfxFont::NotifyGlyphsChanged, which calls new gfxGlyphExtents::NotifyGlyphsChanged

Review of attachment 792184 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +284,5 @@
>  
> +void
> +gfxFontEntry::NotifyGlyphsChanged()
> +{
> +    for (uint32_t i = 0; i < mFontListForSVGFonts.Length(); ++i) {

Put Length() into a local 'count' variable for the loop.

@@ +2951,5 @@
> +                                 void* aClosure)
> +{
> +    gfxFontGroup* fg = aKey->GetKey();
> +    // A gfxFontGroup which doesn't have a gfxUserFontSet can't have this
> +    // user font!

I think it'd be worth making that an assertion rather than merely a comment.
Attachment #792184 - Flags: review?(jfkthame) → review+
John Daggett had some comments about the whole approach that we discussed on IRC today but he didn't get around to putting in the bug.

Basically, he prefers an approach where instead of keeping references from gfxFonts to their owning gfxFontGroups and then user-font-sets and hence prescontexts, each gfxFont would have a set of some kind of "glyphs changed" callback objects attached. These objects would be attached and removed by each nsTextFrame that uses the font in a gfxTextRun during Reflow (I think ... I'm extrapolating a bit here).

This would allow more fine-grained invalidation/reflow, would probably add less complexity to the font system (but more to layout), and would work with system SVG fonts when there's no gfxUserFontSet.

What do you think?
Note, we'd still need most of parts 4 and 6 since we need a way for a gfxFontEntry with SVG glyphs to find all its gfxFonts and invalidate their gfxTextExtents. So basically we'd modify part 3 to map to callback objects instead of gfxFontGroups and remove the gfxFontGroup changes. Then we'd add layout code to manage those callback objects.
Comment on attachment 794608 [details] [diff] [review]
Part 7 v2

Review of attachment 794608 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.h
@@ +434,5 @@
>      nsRefPtr<gfxCharacterMap> mCharacterMap;
>      uint32_t         mUVSOffset;
>      nsAutoArrayPtr<uint8_t> mUVSData;
>      gfxUserFontData* mUserFontData;
> +    nsAutoPtr<gfxSVGGlyphs> mSVGGlyphs;

Hmm....it looks to me like mUserFontData could also be changed to an nsAutoPtr - maybe fix that too, while you're here?

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +77,5 @@
>  
> +void
> +gfxSVGGlyphs::DidRefresh()
> +{
> +    if (mFontEntry) {

Remove the null check here; there's no way we can construct a gfxSVGGlyphs without passing it a gfxFontEntry. (Maybe assert that it's non-null, for debug purposes.)

::: gfx/thebes/gfxSVGGlyphs.h
@@ +35,3 @@
>      typedef mozilla::dom::Element Element;
>      typedef gfxFont::DrawMode DrawMode;
> +    typedef mozilla::TimeStamp TimeStamp;

What do we need TimeStamp for here? I'm not seeing it offhand...

@@ +38,3 @@
>  
> +    // We don't need to actually refcount this since this won't go away during
> +    // WillRefresh.

Don't you mean DidRefresh here?
Attachment #794608 - Flags: review?(jfkthame) → review+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #29)
> Note, we'd still need most of parts 4 and 6 since we need a way for a
> gfxFontEntry with SVG glyphs to find all its gfxFonts and invalidate their
> gfxTextExtents. So basically we'd modify part 3 to map to callback objects
> instead of gfxFontGroups and remove the gfxFontGroup changes. Then we'd add
> layout code to manage those callback objects.

Hmm, just saw these comments, but I'm on my way to bed. Will think about it tomorrow.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> John Daggett had some comments about the whole approach that we discussed on
> IRC today but he didn't get around to putting in the bug.
> 
> Basically, he prefers an approach where instead of keeping references from
> gfxFonts to their owning gfxFontGroups and then user-font-sets and hence
> prescontexts, each gfxFont would have a set of some kind of "glyphs changed"
> callback objects attached. These objects would be attached and removed by
> each nsTextFrame that uses the font in a gfxTextRun during Reflow (I think
> ... I'm extrapolating a bit here).
> 
> This would allow more fine-grained invalidation/reflow, would probably add
> less complexity to the font system (but more to layout), and would work with
> system SVG fonts when there's no gfxUserFontSet.
> 
> What do you think?

I don't think I'm entirely clear on what these "callback objects" are expected to be. Would it be sufficient for the gfxFont to keep a list of the nsTextFrames that have used it, and then its NotifyGlyphsChanged callback would tell each of these frames to reflow/invalidate? Or is the idea that we'll keep track of the individual animated glyphs, so that we can invalidate ONLY those that have changed?

Anyhow, in principle this sounds like it could be a good approach. Is there a possibility the more fine-grained tracking would prove significantly more expensive, though? The number of nsTextFrames that use a given font might be much larger than the number of gfxFontGroups in some cases, I'd guess.

Maybe we should try coding it this way and see how it works out - both in terms of the complexity of the changes and any detectable difference in performance. I do like the fact that it would no longer be limited to user fonts; that always looked like an interim situation that we'd need to revisit at some point. Are you up for writing the alternate version of the patches?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> Basically, he prefers an approach where instead of keeping references from
> gfxFonts to their owning gfxFontGroups and then user-font-sets and hence
> prescontexts, each gfxFont would have a set of some kind of "glyphs changed"
> callback objects attached. These objects would be attached and removed by
> each nsTextFrame that uses the font in a gfxTextRun during Reflow (I think
> ... I'm extrapolating a bit here).
> 
> This would allow more fine-grained invalidation/reflow, would probably add
> less complexity to the font system (but more to layout), and would work with
> system SVG fonts when there's no gfxUserFontSet.

Yes, this is basically what I was thinking.  

Looking over the patches, I would sketch it out something like:

- add two new flags to gfxFontEntry, mHasDynamicGlyphs and mDynamicGlyphsDirty
- for SVG glyphs mHasDynamicGlyphs is set to SVGDocumentWrapper::IsAnimated
- DidRefresh sets mDynamicGlyphsDirty to true
- clear mTightGlyphExtents when mDynamicGlyphsDirty is set (within
  GetTightGlyphExtentsAppUnits?)
- add text run flag if any glyph run has dynamic glyphs and mark text frames
- layout code has observer to handle the logic now in
  nsUserFontSet::NotifyGlyphsChanged, it invalidates all text frames
  containing dynamic glyphs

Hopefully this will be easier to understand in the code and it'll be easy to support other dynamic bitmap formats (e.g. Google and/or MS proposals) later.
(In reply to Jonathan Kew (:jfkthame) from comment #32)
> I don't think I'm entirely clear on what these "callback objects" are
> expected to be. Would it be sufficient for the gfxFont to keep a list of the
> nsTextFrames that have used it, and then its NotifyGlyphsChanged callback
> would tell each of these frames to reflow/invalidate? Or is the idea that
> we'll keep track of the individual animated glyphs, so that we can
> invalidate ONLY those that have changed?

We don't want gfxFont to refer to nsTextFrames directly. So these would be abstract gfx-defined wrapper objects around nsTextFrames. I don't think tracking individual animated glyphs is necessary at this stage.

> Anyhow, in principle this sounds like it could be a good approach. Is there
> a possibility the more fine-grained tracking would prove significantly more
> expensive, though? The number of nsTextFrames that use a given font might be
> much larger than the number of gfxFontGroups in some cases, I'd guess.

Yes, but hopefully you don't use one of these fonts for all your text on many pages.

> Maybe we should try coding it this way and see how it works out - both in
> terms of the complexity of the changes and any detectable difference in
> performance. I do like the fact that it would no longer be limited to user
> fonts; that always looked like an interim situation that we'd need to
> revisit at some point. Are you up for writing the alternate version of the
> patches?

Yes.
(In reply to John Daggett (:jtd) from comment #33)
> - add two new flags to gfxFontEntry, mHasDynamicGlyphs and
> mDynamicGlyphsDirty
> - for SVG glyphs mHasDynamicGlyphs is set to SVGDocumentWrapper::IsAnimated

We can't use SVGDocumentWrapper::IsAnimated, it's broken for animations other than SMIL-based animation (e.g. CSS animations and animated raster images). It's difficult to know whether there's animation other than just by waiting to see if there's a refresh driver tick, like the current patches do.
This is good. Thanks John for pointing this out.
Attachment #792181 - Attachment is obsolete: true
Attachment #792182 - Attachment is obsolete: true
Attachment #796464 - Flags: review?(jfkthame)
Attached patch Part 6 v2Splinter Review
Updated to notify through the GlyphChangeObserver list.
Attachment #792184 - Attachment is obsolete: true
Attachment #796466 - Flags: review?(jfkthame)
Attached patch Part 5.2 v2Splinter Review
There is actually no need for GlyphChangeObserver to keep the gfxFont alive, we can easily make that reference weak.
Attachment #796464 - Attachment is obsolete: true
Attachment #796464 - Flags: review?(jfkthame)
Attachment #797075 - Flags: review?(jfkthame)
Attached patch Part 5.6 v2 (obsolete) — Splinter Review
The previous patch had a problem with transformed text runs. Basically we have to delay the animated-glyphs check until we've updated the glyphruns for an nsTransformedTextRun.
Attachment #796465 - Attachment is obsolete: true
Attachment #796465 - Flags: review?(jfkthame)
Attachment #797076 - Flags: review?(jfkthame)
Attachment #796466 - Attachment is patch: true
Comment on attachment 797075 [details] [diff] [review]
Part 5.2 v2

Review of attachment 797075 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxFont.cpp
@@ +1717,5 @@
>      mKerningSet = HasFeatureSet(HB_TAG('k','e','r','n'), mKerningEnabled);
>  }
>  
> +static PLDHashOperator
> +NotifyGlyphChangeObserversOfFontDestruction(nsPtrHashKey<gfxFont::GlyphChangeObserver>* aKey,

Would you be OK with making this slightly less verbose? Maybe just NotifyFontDestroyed(...)? ISTM that would be sufficiently clear, given its usage.

@@ +3745,5 @@
> +{
> +  NS_ASSERTION(mGlyphChangeObservers, "No observers registered");
> +  NS_ASSERTION(mGlyphChangeObservers->Contains(aObserver), "Observer not registered");
> +  mGlyphChangeObservers->RemoveEntry(aObserver);
> +}

nit for both these functions: local style expects a 4-space indent.
Attachment #797075 - Flags: review?(jfkthame) → review+
Comment on attachment 797076 [details] [diff] [review]
Part 5.6 v2

Review of attachment 797076 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should probably collect the fontsWithAnimatedGlyphs using a hashset or something like that, rather than appending to an array when iterating over the glyph runs.

The case where the same (animated) font gets used for a number of separate glyph runs may be quite common - think of a simple paragraph of text with a bunch of emoji scattered through it. As it stands, this patch looks like it could end up putting the emoji font into the list multiple times, and then creating multiple observers for the same font and textframe.
Attachment #796466 - Flags: review?(jfkthame) → review+
(In reply to Jonathan Kew (:jfkthame) from comment #42)
> Comment on attachment 797076 [details] [diff] [review]
> Part 5.6 v2
> 
> Review of attachment 797076 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think we should probably collect the fontsWithAnimatedGlyphs using a
> hashset or something like that, rather than appending to an array when
> iterating over the glyph runs.
> 
> The case where the same (animated) font gets used for a number of separate
> glyph runs may be quite common - think of a simple paragraph of text with a
> bunch of emoji scattered through it. As it stands, this patch looks like it
> could end up putting the emoji font into the list multiple times, and then
> creating multiple observers for the same font and textframe.

Or maybe just check whether the font is already in the array before appending it. I'd guess the number of *distinct* font instances that are using animated glyphs is likely to be very small, so just linear-searching the array shouldn't be expensive.
Attached patch Part 5.6 v3Splinter Review
Attachment #797076 - Attachment is obsolete: true
Attachment #797076 - Flags: review?(jfkthame)
Attachment #800593 - Flags: review?(jfkthame)
Comment on attachment 800593 [details] [diff] [review]
Part 5.6 v3

Review of attachment 800593 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, modulo a couple of nits.

::: layout/generic/nsTextFrame.cpp
@@ +530,5 @@
>  
> +void
> +GlyphObserver::NotifyGlyphsChanged()
> +{
> +  nsIPresShell* shell = mFrame->PresContext()->PresShell();

Should there be null-checks here? ISTM I've seen other code that checks these and bails early if they're null, implying that we think (or thought) there was a possibility they could be null in some situations. Or do you know better?

@@ +831,5 @@
> +  // We are lazy and don't try to remove a property value that might be
> +  // obsolete due to style changes or font selection changes. That is
> +  // likely to be rarely needed, and we don't want to eat the overhead of
> +  // doing it for the overwhelmingly common case of no property existing.
> +  // (And we're out of state bits to conviently use for a fast property

s/conviently/conveniently/

@@ +841,5 @@
> +static void
> +CreateObserversForAnimatedGlyphs(gfxTextRun* aTextRun)
> +{
> +  if (!aTextRun->GetUserData())
> +    return;

{braces}

@@ +844,5 @@
> +  if (!aTextRun->GetUserData())
> +    return;
> +
> +  nsTArray<gfxFont*> fontsWithAnimatedGlyphs;
> +  gfxTextRun::GlyphRunIterator glyphRuns(aTextRun, 0, aTextRun->GetLength());

Given that we're going to iterate over the whole textrun here, creating a GlyphRunIterator (with its support for partial-textrun ranges) is overkill; it would be slightly faster to simply call aTextRun->GetGlyphRuns() and use the pointer and count it returns.
Attachment #800593 - Flags: review?(jfkthame) → review+
BTW, did you perhaps obsolete the wrong patch here when updating to the new implementation? It looks to me like you probably meant to obsolete part 2, but not part 4.
(In reply to Jonathan Kew (:jfkthame) from comment #45)
> > +  nsIPresShell* shell = mFrame->PresContext()->PresShell();
> 
> Should there be null-checks here? ISTM I've seen other code that checks
> these and bails early if they're null, implying that we think (or thought)
> there was a possibility they could be null in some situations. Or do you
> know better?

No need to null-check those when you get them through a frame.
Comment on attachment 794606 [details] [diff] [review]
Part 1 v2

r=dbaron
Attachment #794606 - Flags: review?(dbaron) → review+
Depends on: 1276829
You need to log in before you can comment on or make changes to this bug.