935862 - inefficient textrun construction on pages with downloadable fonts

Status: RESOLVED FIXED

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

Description

[John Daggett (:jtd)]

Attachment: textperf metrics when loading wikipedia ben franklin page

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.

Comment 1

[Cameron McCormack (:heycam)]

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?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 3

[Cameron McCormack (:heycam)]

(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.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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...)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 7

[John Daggett (:jtd)]

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.

Comment 8

[John Daggett (:jtd)]

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.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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?

Comment 10

[John Daggett (:jtd)]

Attachment: patch, part 1 - add a flag for ex/ch unit usage

Comment 11

[John Daggett (:jtd)]

Attachment: patch, part 2 - add userfont lookup methods

Comment 12

[John Daggett (:jtd)]

Attachment: patch, part 3 - do selective reflows when downloadable fonts load

Comment 13

[John Daggett (:jtd)]

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.

Comment 14

[John Daggett (:jtd)]

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

Comment 15

[John Daggett (:jtd)]

Hmmm, maybe MarkDescendantsDirty isn't quite correct in this situation...

Comment 16

[Cameron McCormack (:heycam)]

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.

Comment 17

[Cameron McCormack (:heycam)]

If you pass eStyleChange to FrameNeedsReflow, then it will call MarkIntrinsicISizesDirty on ancestors and descendants, which I think is what you want.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yes, you want nsIPresShell::FrameNeedsReflow with eStyleChange, rather than using nsLayoutUtils::MarkDescendantsDirty.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(and FrameNeedsReflow will schedule the reflow)

Comment 20

[John Daggett (:jtd)]

Attachment: patch, part 3 - do selective reflows when downloadable fonts load

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.

Comment 21

[Cameron McCormack (:heycam)]

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 22

[Cameron McCormack (:heycam)]

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().

Comment 23

[Cameron McCormack (:heycam)]

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.

Comment 24

[John Daggett (:jtd)]

Attachment: patch, part 3 v3 - do selective reflows when downloadable fonts load

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

Comment 25

[John Daggett (:jtd)]

Attachment: patch, part 4 - basic reflow sanity tests

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.

Comment 26

[Cameron McCormack (:heycam)]

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. :-)

Comment 27

[Cameron McCormack (:heycam)]

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?

Comment 28

[Cameron McCormack (:heycam)]

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.

Comment 29

[Cameron McCormack (:heycam)]

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.

Comment 30

[John Daggett (:jtd)]

(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.

Comment 31

[John Daggett (:jtd)]

Attachment: patch, part 5 - handle svgtextframe's correctly

Cam's patch to fixup the handling of SVGTextFrame's.

r=jdaggett

Comment 32

[John Daggett (:jtd)]

Attachment: patch, part 2 - add userfont lookup methods

Revised based on review comments.

Comment 33

[John Daggett (:jtd)]

Attachment: patch, part 6 - workaround various reftest failures

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.

Comment 34

[John Daggett (:jtd)]

Attachment: patch, part 3a - adjust the code a bit for landing

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.

Comment 35

[Cameron McCormack (:heycam)]

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.

Comment 36

[John Daggett (:jtd)]

(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.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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...)

Comment 38

[John Daggett (:jtd)]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeea4c65c725
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d45de55087d
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4d3f86cfff
https://hg.mozilla.org/integration/mozilla-inbound/rev/cab9f41173d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/b692ef1c19c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/70371a23e8dc

Comment 39

[John Daggett (:jtd)]

(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.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eeea4c65c725
https://hg.mozilla.org/mozilla-central/rev/5d45de55087d
https://hg.mozilla.org/mozilla-central/rev/aa4d3f86cfff
https://hg.mozilla.org/mozilla-central/rev/cab9f41173d9
https://hg.mozilla.org/mozilla-central/rev/b692ef1c19c4
https://hg.mozilla.org/mozilla-central/rev/70371a23e8dc