Closed Bug 935862 Opened 6 years ago Closed 5 years ago

inefficient textrun construction on pages with downloadable fonts

Categories

(Core :: Layout: Text and Fonts, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jtd, Assigned: jtd)

References

(Depends on 2 open bugs, )

Details

Attachments

(8 files, 3 obsolete files)

2.89 KB, text/plain
Details
4.11 KB, patch
heycam
: review+
Details | Diff | Splinter Review
13.36 KB, patch
heycam
: review+
Details | Diff | Splinter Review
57.17 KB, patch
heycam
: review+
Details | Diff | Splinter Review
4.25 KB, patch
jtd
: review+
Details | Diff | Splinter Review
9.08 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.08 KB, patch
heycam
: review+
Details | Diff | Splinter Review
1.56 KB, patch
heycam
: review+
Details | Diff | Splinter Review
When loading the Wikipedia page for Ben Franklin, we appear to end up creating a huge number of textruns three times.  

Dumping out the textruns, which includes font and style plus the actual textrun text:

16632 runs (16167 using font-family: sans-serif)
2364 unique runs (!)
144 same textrun constructed 63 times (!?!)
1327 same textrun constructed 3 times
294 textruns affected by downloadable font

There are three big reflows, and the word cache hit ratio reflects the pattern: 67% on the first pass, 99-100% on the subsequent runs.  At the end of the third large reflow 12,263 textruns have been dumped (i.e. explicitly dumped, not expired).

The page contains a downloadable font that is used for a small selection of all textruns on the page.  I'm guessing one factor may be the PostRebuildAllStyleDataEvent(NS_STYLE_HINT_REFLOW) call in nsPresContext::UserFontSetUpdated().  The userfont set change will only affect textruns that reference @font-face fonts (i.e. *not* the sans-serif runs) but I suspect we're dumping more than just those runs.
Here's an approach:

* Have a generation counter (do we have one already, actually?) on the user font set, which gets incremented whenever a font is added or finished downloading.  Additionally, store what the generation counter value is when a given font starts/finishes downloading.

* Store on nsStyleFont the value of the user font set generation counter at the time the nsStyleFont was created.

* Add a new restyle hint, eRestyleHint_UserFontSetChange, which means "restyle under the assumption that the user font set has changed".

* Change nsPresContext::UserFontSetUpdated to pass in nsChangeHint(0) and (eRestyle_UserFontSetChange | eRestyle_ForceDescendants).

* Make nsStyleFont::CalcDifference return nsChangeHint_NeutralChange for generation counter differences.

* Add handling for eRestyleHint_UserFontSetChange in ElementRestyler::RestyleSelf that queries the user font set to ask "given the font I've got in my new nsStyleFont, is it affected by any downloadable font changes in the user font set after generation N"?  If true, we add NS_CHANGE_HINT_RESTYLE.

John/David, does this sound like it could work?
Flags: needinfo?(dbaron)
How does comment 1 differ from what we do now?  (i.e., what's today's path that leads to text runs being dumped?)

(In reply to Cameron McCormack (:heycam) from comment #1)
> * Add handling for eRestyleHint_UserFontSetChange in
> ElementRestyler::RestyleSelf that queries the user font set to ask "given
> the font I've got in my new nsStyleFont, is it affected by any downloadable
> font changes in the user font set after generation N"?  If true, we add
> NS_CHANGE_HINT_RESTYLE.

What was NS_CHANGE_HINT_RESTYLE supposed to be here?
Flags: needinfo?(dbaron) → needinfo?(cam)
(In reply to David Baron [:dbaron] (UTC+11) (needinfo? for questions) from comment #2)
> How does comment 1 differ from what we do now?  (i.e., what's today's path
> that leads to text runs being dumped?)

Currently, in nsPresContext::UserFontSetUpdated we call PostRebuildAllStyleData with NS_STYLE_HINT_REFLOW to reflow the entire document.  Reflowing the text frame is what causes the text runs to be dumped, IIRC.  John correct me if I'm wrong...

> What was NS_CHANGE_HINT_RESTYLE supposed to be here?

I meant to type NS_STYLE_HINT_REFLOW.
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #3)
> Currently, in nsPresContext::UserFontSetUpdated we call
> PostRebuildAllStyleData with NS_STYLE_HINT_REFLOW to reflow the entire
> document.  Reflowing the text frame is what causes the text runs to be
> dumped, IIRC.  John correct me if I'm wrong...

No, I *think* that is caused by the nsTextFrame::MarkIntrinsicISizesDirty call resulting from the nsChangeHint_ClearDescendantIntrinsics hint within NS_STYLE_HINT_REFLOW.

(More thoughts shortly, I hope...)
(In reply to Cameron McCormack (:heycam) from comment #1)
> * Have a generation counter (do we have one already, actually?) on the user
> font set, which gets incremented whenever a font is added or finished
> downloading.  Additionally, store what the generation counter value is when
> a given font starts/finishes downloading.

gfxUserFontSet::mGeneration / GetGeneration may well already do what you want, but you should check.

> * Add a new restyle hint, eRestyleHint_UserFontSetChange, which means
> "restyle under the assumption that the user font set has changed".

This feels more like a change hint than a restyle hint.  (Or, an
alternative to a change hint is a DidSetStyleContext override on
a frame class, which might actually be worth considering in this case.)

> * Change nsPresContext::UserFontSetUpdated to pass in nsChangeHint(0) and
> (eRestyle_UserFontSetChange | eRestyle_ForceDescendants).

Fine, modulo previous comment.

> * Store on nsStyleFont the value of the user font set generation counter at
> the time the nsStyleFont was created.

Seems useful with all 3 approaches I'm discussing here.

> * Make nsStyleFont::CalcDifference return nsChangeHint_NeutralChange for
> generation counter differences.

Yeah, it would be this way with a DidSetStyleContext approach, although
different (but I don't quite see how) with a change hint.

> * Add handling for eRestyleHint_UserFontSetChange in
> ElementRestyler::RestyleSelf that queries the user font set to ask "given
> the font I've got in my new nsStyleFont, is it affected by any downloadable
> font changes in the user font set after generation N"?  If true, we add
> NS_STYLE_HINT_REFLOW.

The DidSetStyleContext could call ClearTextRuns, although with my
DidSetStyleContext proposal you would still need something to trigger
the reflow.  So I guess my DidSetStyleContext idea doesn't quite work,
but the change hint approach also doesn't quite work since you might not
have the necessary user font set (or text -- do you need that)
information in nsStyleFont::CalcDifference.

Then again, maybe the change hint thing would work; you'd just need
to add something to give nsStyleFont::CalcDifference access to a
pres context so it can get the user font set.  I don't think that's
harmful; it's just something we've never done.
(In reply to David Baron [:dbaron] (UTC+11) (needinfo? for questions) from comment #5)
> This feels more like a change hint than a restyle hint.  (Or, an
> alternative to a change hint is a DidSetStyleContext override on
> a frame class, which might actually be worth considering in this case.)

BTW, part of my intuition here is that change hints are the general mechanism for adding more specific (i.e., more optimized) change handling to layout or any other consumers of style data; adding more of them doesn't really add complexity worse than linearly, so it's relatively cheap.  Restyle hints exist to solve a specific problem within the style system and have already gotten more complicated than I'd like them to be.
The original testcase has been modified so I set up two testpages that mimic page performance with and without webfont usage:

http://people.mozilla.org/~jdaggett/fontreflow/wikipedia-benfranklin-normal.html
http://people.mozilla.org/~jdaggett/fontreflow/wikipedia-benfranklin-webfonts.html

For the most part I simply substituted Fira Sans for sans-serif and Noto Serif for serif. I also tweaked the font weight for certain elements to bump up the total number of fonts loaded to 8 fonts.

I logged textruns for both cases and then analyzed the number of times the same textrun was constructed in each case. The bottom three entries of the distribution are included below.

command line: 
grep textrun textrun-benfranklin-webfonts.out | sort | uniq -c | sort -r | awk '{print $1}' | uniq -c

Normal wikipedia page for Ben Franklin

#textrun numtimes
  27 3
 262 2
1332 1

Webfonts wikipedia page for Ben Franklin (8 webfonts loaded)

#textrun numtimes
   3 7
 238 6
1332 5

example textrun built 5 times in the webfonts case:

5 [fira sans] default: serif lang: en script: 25 len 111 weight: 400 width: 0 style: normal size:  28.00 1-byte TEXTRUN [ That same year, he edited and published the first Masonic book in the Americas, a reprint of James Anderson's ] ENDTEXTRUN

So the majority of textruns were built only once without webfonts and built five times with. Ideally in the webfonts case individual textruns should only be built twice, before and after the font for a given textrun loads. But because the restyle operation forces all frames to be reconstructed, the same textrun gets built several times more than is necessary.
So I think there are a couple separate problems here.

Current behavior is that when userfont set updates occur, nsPresContext::UserFontSetUpdated method is called which, in turn, calls PostRebuildAllStyleDataEvent to restyle everything. The restyle code calls FrameNeedsReflow which in turn calls MarkIntrinsicISizesDirty, which for text frames dumps the text runs:

nsTextFrame::ClearTextRuns (this=0x12f7b9790) at nsTextFrame.h:502
nsTextFrame::MarkIntrinsicISizesDirty (this=0x12f7b9790) at nsTextFrame.cpp:7371
PresShell::FrameNeedsReflow (this=0x129e8b800, aFrame=0x12bbef458, aIntrinsicDirty=nsIPresShell::eStyleChange, aBitToAdd=NS_FRAME_IS_DIRTY) at nsPresShell.cpp:2893
mozilla::RestyleManager::StyleChangeReflow (this=0x129d61400, aFrame=0x12bbef458, aHint=524351) at RestyleManager.cpp:582

The UserFontSetUpdated method is called in three situations:

1. fontset has been updated, if script adds/deletes a font for example
2. font load has completed (data urls, cached faces avoid this)
3. font load is running slow and fallback fonts need to be used

The only place where a restyle event should occur I think is (1), since for (2) and (3) there's nothing really that changes with respect to style data. For cases (2) and (3) we should be using a separate method that only initiates reflow as needed.

The "as needed" part is what's tricky here. Generation numbers don't really help, when the fontset is updated those will change so any stored values will always be stale. The real question is whether a particular frame uses line metrics or textruns and, if it does, whether those metrics and/or textruns were affected by the font load. A first approximation is whether the nsStyleFont fontlist contains the family name matching that of the font loaded. A more optimal heuristic would be whether the *face* loaded is one that would be chosen given the font properties for a given frame but I suspect that's simply too complex to handle efficiently.

So I think I will try to code up:

1. a new nsPresContext::UserFontLoaded method that is called instead of UserFontSetUpdated when a font load completes

2. within UserFontLoaded, traverse the frame tree and mark dirty frames that contain the family name of the loaded font in the fontlist

3. within nsTextFrame::MarkIntrinsicISizesDirty, instead of simply dumping textruns, look a little more carefully at whether to dump or not

Of these, I think (3) is the part that looks tricky. It's not clear to me how to tell *what* caused something to be marked dirty or what the heuristics are for determining whether textruns need to be rebuilt or not. Ultimately we need to avoid dumping textruns whenever possible but for this bug hopefully I can isolate the "font loaded" case and determine whether the fonts in the gfxFontGroup include the loaded font or not, then blow away textruns only if it does.
(In reply to John Daggett (:jtd) from comment #8)
> The UserFontSetUpdated method is called in three situations:
> 
> 1. fontset has been updated, if script adds/deletes a font for example
> 2. font load has completed (data urls, cached faces avoid this)
> 3. font load is running slow and fallback fonts need to be used
> 
> The only place where a restyle event should occur I think is (1), since for
> (2) and (3) there's nothing really that changes with respect to style data.
> For cases (2) and (3) we should be using a separate method that only
> initiates reflow as needed.

Can't this affect which font is chosen and measured for 'ex' and 'ch' units?  (Is that the only reason we need to restyle?)

Would it help to maintain global per-page booleans for whether the page uses ex and ch units so that we only do the restyle when they're present?  Or are there other dependencies?
This isn't quite right, the attached patches cause the correct loaded fonts to display without multiple rebuilds for each font but somehow the metrics for the fallback fonts aren't getting removed, which leads to the line spacing being wrong and clipping to occur at the right edge of some text.
Simple testcase:
  http://people.mozilla.org/~jdaggett/fontreflow/simplefontreflow.html

Current behavior: all textruns are built 4 times
Desired behavior: 3 textruns are built twice, 1 textrun is once
Hmmm, maybe MarkDescendantsDirty isn't quite correct in this situation...
You at least need something to kick off a reflow after setting dirty bits.  I think David mentioned that you want to call nsIPresShell::FrameNeedsReflow (which will do something like MarkDescendantsDirty) and then schedule a reflow.
If you pass eStyleChange to FrameNeedsReflow, then it will call MarkIntrinsicISizesDirty on ancestors and descendants, which I think is what you want.
Yes, you want nsIPresShell::FrameNeedsReflow with eStyleChange, rather than using nsLayoutUtils::MarkDescendantsDirty.
(and FrameNeedsReflow will schedule the reflow)
Updated to use FrameNeedsReflow. The code in MarkDirtyForFontChange will handle a null font argument but this is currently unused. The initial construction of the userfont set has some tricky requirements since it may occur within reflow, such that calling MarkDirtyForFontChange isn't appropriate. So within nsPresContext::UserFontSetUpdated, a null font calls through to the old call to PostRebuildAllStyleDataEvent.

Note that this patch is dependent on the linux platform font list changes, although if need be I could adjust to function correctly on top of existing trunk code.
Attachment #8565859 - Attachment is obsolete: true
Attachment #8566357 - Flags: review?(cam)
Attachment #8565858 - Flags: review?(cam)
Attachment #8565857 - Flags: review?(cam)
Depends on: 1056479
Attachment #8565857 - Flags: review?(cam) → review+
Comment on attachment 8565858 [details] [diff] [review]
patch, part 2 - add userfont lookup methods

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

::: gfx/thebes/gfxTextRun.cpp
@@ +3058,5 @@
>  
> +bool
> +gfxFontGroup::ContainsUserFont(const gfxUserFontEntry* aUserFont)
> +{
> +    UpdateUserFonts();

Why do we need to call this here?

::: gfx/thebes/gfxUserFontSet.cpp
@@ +924,5 @@
> +{
> +    const nsTArray<FontFamilyName>& fontnames = aFontList.GetFontlist();
> +    uint32_t n = fontnames.Length();
> +    for (uint32_t i = 0; i < n; i++) {
> +        const FontFamilyName& name = fontnames[i];

If you want to use the new ranged for loops, you could replace these four lines with:

  for (const FamilyName& name : aFontList.GetFontlist()) {

:)
Comment on attachment 8566357 [details] [diff] [review]
patch, part 3 - do selective reflows when downloadable fonts load

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

::: layout/base/nsPresContext.cpp
@@ +2152,5 @@
>  {
>    if (!mShell)
>      return;
>  
> +  bool usePlatformFontList = true;

May as well drop the "= true".

@@ +2161,5 @@
> +#endif
> +
> +  // xxx - until the Linux platform font list is always used, use full
> +  // restyle to force updates with gfxPangoFontGroup usage
> +  if (!usePlatformFontList || !aUpdatedFont) {

Can you add a comment in here about why we're doing this when aUpdatedFont is null?

@@ +2181,5 @@
> +  // downloadable font family in question. If a frame's nsStyleFont has
> +  // the name, check the font group associated with the metrics to see if
> +  // it contains that specific font (i.e. the one chosen within the family
> +  // given the weight, width, and slant from the nsStyleFont). If it does,
> +  // mark that frame dirty and skip inspecting it's descendents.

s/it's descendents/its descendants/

::: layout/style/nsFontFaceUtils.cpp
@@ +12,5 @@
> +#include "nsPlaceholderFrame.h"
> +#include "nsTArray.h"
> +
> +static bool
> +FrameUsesFont(nsIFrame* aFrame, const gfxUserFontEntry* aFont)

To be safe, we should not only check the main style context of the frame, but also the frame's additional style contexts.  Could you make this function loop over the frame's additional style contexts and check their mFont.fontlist values too?

@@ +44,5 @@
> +  return false;
> +}
> +
> +/* static */ void
> +nsFontFaceUtils::MarkDirtyForFontChange(nsIFrame* aSubtreeRoot,

It would be kind of nice to avoid the duplication this function has with MarkDescendantsDirty, but don't worry about it for now.

@@ +55,5 @@
> +
> +  // check descendants, iterating over subtrees that may include
> +  // additional subtrees associated with placeholders
> +  do {
> +    nsIFrame *subtreeRoot = subtrees.ElementAt(subtrees.Length() - 1);

"*" next to type.

@@ +63,5 @@
> +    nsAutoTArray<nsIFrame*, 32> stack;
> +    stack.AppendElement(subtreeRoot);
> +
> +    do {
> +      nsIFrame *f = stack.ElementAt(stack.Length() - 1);

And here.

@@ +66,5 @@
> +    do {
> +      nsIFrame *f = stack.ElementAt(stack.Length() - 1);
> +      stack.RemoveElementAt(stack.Length() - 1);
> +
> +      // if this frame uses the font, mark it's descendants dirty

its

@@ +72,5 @@
> +      if (FrameUsesFont(f, aFont)) {
> +        ps->FrameNeedsReflow(f, nsIPresShell::eStyleChange, NS_FRAME_IS_DIRTY);
> +      } else {
> +        if (f->GetType() == nsGkAtoms::placeholderFrame) {
> +          nsIFrame *oof = nsPlaceholderFrame::GetRealFrameForPlaceholder(f);

And here.

@@ +89,5 @@
> +          }
> +        }
> +      }
> +    } while (stack.Length() != 0);
> +  } while (subtrees.Length() != 0);

Better to use !list.IsEmpty().
Attachment #8566357 - Flags: review?(cam) → review+
Comment on attachment 8566357 [details] [diff] [review]
patch, part 3 - do selective reflows when downloadable fonts load

Would like to see an updated version of this patch.
Attachment #8566357 - Flags: review+
Depends on: 1135556
Depends on: 1135329
Depends on: 1135567
Updated based on review comments.

(In reply to Cameron McCormack (:heycam) from comment #22)
> Comment on attachment 8566357 [details] [diff] [review]
> patch, part 3 - do selective reflows when downloadable fonts load
> 
> Review of attachment 8566357 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsPresContext.cpp
> @@ +2152,5 @@
> >  {
> >    if (!mShell)
> >      return;
> >  
> > +  bool usePlatformFontList = true;
> 
> May as well drop the "= true".

Nope, the conditionals below it only apply to Linux builds! For non-Linux builds this code will simply be optimized out.

> > +  // xxx - until the Linux platform font list is always used, use full
> > +  // restyle to force updates with gfxPangoFontGroup usage
> > +  if (!usePlatformFontList || !aUpdatedFont) {
> 
> Can you add a comment in here about why we're doing this when aUpdatedFont
> is null?

Added. The code isn't ideal now, it's tangled up in the lazy initialization of the userfont set which can occur during reflow. But the main perf win here is to avoid repeated reflows when a page contains a number of fonts, so punt on that for now... :P
Attachment #8566357 - Attachment is obsolete: true
Attachment #8568967 - Flags: review?(cam)
Basic reflow tests to confirm that pages are consistently displaying @font-face content the same way regardless of reflow behavior. Tested using fonts loaded in various ways, including with data urls and with differing load times.

Having the .sjs file for loading fonts with a delay will be helpful for implementing tests that explicitly test multiple reflows as each individual font loads.
Attachment #8569015 - Flags: review?(cam)
Comment on attachment 8568967 [details] [diff] [review]
patch, part 3 v3 - do selective reflows when downloadable fonts load

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

::: layout/base/nsPresContext.cpp
@@ +2183,5 @@
> +
> +  if (UsesExChUnits()) {
> +    // xxx - dbaron said this should work but get ex/ch related reftest failures
> +    // PostRebuildAllStyleDataEvent(nsChangeHint(0), eRestyle_ForceDescendants);
> +    PostRebuildAllStyleDataEvent(NS_STYLE_HINT_REFLOW, eRestyle_ForceDescendants);

File a follow-up to handle this please.

I wonder too whether we should handle this in MarkDirtyForFontChange?  We could call nsLayoutUtils::PostRestyleEvent on the element rather than FrameNeedsReflow, if UsesExChUnits() is true.

::: layout/style/nsFontFaceUtils.cpp
@@ +11,5 @@
> +#include "nsLayoutUtils.h"
> +#include "nsPlaceholderFrame.h"
> +#include "nsTArray.h"
> +
> +static bool StyleContextContainsFont(nsStyleContext* aStyleContext,

Newline after "bool", to be fully style guide compliant. :-)
Attachment #8568967 - Flags: review?(cam) → review+
Comment on attachment 8569015 [details] [diff] [review]
patch, part 4 - basic reflow sanity tests

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

::: layout/reftests/font-face/reflow-sanity-delay-1a.html
@@ +7,5 @@
> +body { margin: 20px }
> +
> +@font-face {
> +  font-family: reflow1a;
> +  src: url(../fonts/markfonts-delay.sjs?font=markA&delay=100);

Like you do in reflow-sanity-delay-1c.html, should you be putting &test=delay-1a in here (and in reflow-sanity-delay-1b.html)?  Presumably that is to avoid font cache hits?
Attachment #8569015 - Flags: review?(cam) → review+
If you can come up with some tests that use nsDOMWindowUtils::GetFramesReflowed that confirms that we are only reflowing the number of frames that we expect, after a given font is loaded, that'd be good too.
BTW: on IRC we were discussing whether the timers would still fire in the right order if their timeout period all elapsed at once (if the machine is running very slowly for example); I checked nsTimerImpl.cpp/TimerThread.cpp and it seems that they will.
(In reply to Cameron McCormack (:heycam) from comment #27)
> Like you do in reflow-sanity-delay-1c.html, should you be putting
> &test=delay-1a in here (and in reflow-sanity-delay-1b.html)?  Presumably
> that is to avoid font cache hits?

Makes sense. Yes, it's to avoid pulling fonts out of the font cache.
Cam's patch to fixup the handling of SVGTextFrame's.

r=jdaggett
Attachment #8570353 - Flags: review+
Revised based on review comments.
Attachment #8565858 - Attachment is obsolete: true
Attachment #8565858 - Flags: review?(cam)
Attachment #8571767 - Flags: review?(cam)
Attachment #8571767 - Flags: review?(cam) → review+
Depends on: 1139269
Workaround reftest failures caused by improper invalidation problems. For the all-upper.html and all-lower.html I have some tests that show the problem when restyling so I'm going to log a separate bug on that.
Attachment #8573723 - Flags: review?(cam)
Minor tweak to the part 3 patch to be able to land it before the linux fontconfig platform fontlist bug. Once this lands, I add the changes back to my patches for that bug.
Attachment #8573725 - Flags: review?(cam)
Comment on attachment 8573723 [details] [diff] [review]
patch, part 6 - workaround various reftest failures

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

::: layout/reftests/text-transform/reftest.list
@@ +17,5 @@
>  == uppercase-1.html uppercase-ref.html
>  == uppercase-szlig-1.html uppercase-szlig-ref.html
>  # these use DejaVu Sans via @font-face for consistency of results
> +skip-if(B2G) fuzzy-if(cocoaWidget,250,15) HTTP(..) == all-upper.html all-upper-ref.html # bug 773482
> +skip-if(B2G) fuzzy-if(cocoaWidget,250,15) HTTP(..) == all-lower.html all-lower-ref.html # bug 773482

Please add the number of the bug you're about to file here.
Attachment #8573723 - Flags: review?(cam) → review+
Attachment #8573725 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #26)

> ::: layout/base/nsPresContext.cpp
> @@ +2183,5 @@
> > +
> > +  if (UsesExChUnits()) {
> > +    // xxx - dbaron said this should work but get ex/ch related reftest failures
> > +    // PostRebuildAllStyleDataEvent(nsChangeHint(0), eRestyle_ForceDescendants);
> > +    PostRebuildAllStyleDataEvent(NS_STYLE_HINT_REFLOW, eRestyle_ForceDescendants);
> 
> File a follow-up to handle this please.

David, what I think you suggested, nsChangeHint(0), caused reftest failures. Should that have been sufficient? If you think it should be, I'll file a follow-up to track down the problem.
Flags: needinfo?(dbaron)
(In reply to John Daggett (:jtd) from comment #36)
> (In reply to Cameron McCormack (:heycam) from comment #26)
> 
> > ::: layout/base/nsPresContext.cpp
> > @@ +2183,5 @@
> > > +
> > > +  if (UsesExChUnits()) {
> > > +    // xxx - dbaron said this should work but get ex/ch related reftest failures
> > > +    // PostRebuildAllStyleDataEvent(nsChangeHint(0), eRestyle_ForceDescendants);
> > > +    PostRebuildAllStyleDataEvent(NS_STYLE_HINT_REFLOW, eRestyle_ForceDescendants);
> > 
> > File a follow-up to handle this please.
> 
> David, what I think you suggested, nsChangeHint(0), caused reftest failures.
> Should that have been sufficient? If you think it should be, I'll file a
> follow-up to track down the problem.

Hmmm.  It should be sufficient to handle the ex/ch case, although given that you're returning early, it shuoldn't be sufficient to handle the other things that you're supposed to be doing.

Do you still see the failures without the early return immediately after?

(It's not clear if it's actually worth bothering trying to optimize that case, though...)
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (UTC-8) from comment #37)

> Do you still see the failures without the early return immediately after?

I didn't try that! I'll push a patch with that and file a follow-up if it works.
Blocks: 1140946
You need to log in before you can comment on or make changes to this bug.