I was writing a testcase for bug 1098495 but noticed that I was hitting all the above slow path. Basically the chain of slow path happens because we allocate a display item for the markup indentation (I know it add spacing): Text p=0x10e3bedb0 f=0x109cfa618(Text(2)"\n ") bounds(6450,5730,300,1020) visible(0,0,53580,62100) componentAlpha(6450,5730,300,1020) clip() layer=0x1174ecc00 When I change my testcase to remove the whitespace the thebes layer goes away, the component alpha flag goes away and my intermediate surface no longer requires a copy.

ni?roc since mstange told me you worked on avoiding whitespace text display item?

I think it's a good idea to skip creating display items for text whose textrun contains only spaces (which would cover most forms of whitespace since CSS normally converts them to spaces). The difficulty is that certain very rare fonts have a space glyph that actually draws something. We've discussed this problem before but I don't remember the outcome. Jonathan, do you? It seems likely to me we could analyze the font data to determine whether the glyph returned by gfxFont::GetSpaceGlyph draws anything. Not sure what the overhead of that would be since we might have to load additional tables.

Actually we can probably just extend gfxFont::CheckForFeaturesInvolvingSpace to request the exact ink bounds of the space glyph. If they're empty, we know nothing is drawn.

I haven't looked closely at the patches here yet, but one quick question -- maybe the answer is obvious -- will this have any impact on our ability to highlight or hit-test such text?

No, part 3 takes care of that by always creating the display item if the text is selected or we're building the display list for some reason other than painting.

Comment on attachment 8523616 [details] [diff] [review] Part 1: Make gfxDwriteFont cache GetSpaceGlyph Review of attachment 8523616 [details] [diff] [review]: ----------------------------------------------------------------- Yes, we should cache it; but I think you can do this a bit differently. Note that gfxDWriteFont::ComputeMetrics() always looks up the space glyph anyway, in order to set mMetrics->spaceWidth. Change that to use mFontFace->GetGlyphIndices directly, rather than calling GetSpaceGlyph(), and store the resulting glyphID. Then GetSpaceGlyph() won't need to check anything, it can just return the saved value.

Comment on attachment 8524512 [details] [diff] [review] part 1 v2 Review of attachment 8524512 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxDWriteFonts.cpp @@ +77,5 @@ > AntialiasOption anAAOption) > : gfxFont(aFontEntry, aFontStyle, anAAOption) > , mCairoFontFace(nullptr) > , mMetrics(nullptr) > + , mSpaceGlyph(-1) Let's make this a uint32_t (for consistency with other places) and initialize it to zero, now that you don't need -1 as a "not yet initialized" indicator. @@ +231,5 @@ > > mMetrics->internalLeading = std::max(mMetrics->maxHeight - mMetrics->emHeight, 0.0); > mMetrics->externalLeading = ceil(fontMetrics.lineGap * mFUnitsConvFactor); > > + UINT32 ucs = L' '; You'd better remove the (re)declaration of ucs a bit later in this method.

There's also a weird Mac reftest failure that I need to figure out: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/rocallahan@mozilla.com-731125b76d43/try-macosx64-debug/try_snowleopard-debug_test-reftest-bm108-tests1-macosx-build3034.txt.gz

Still getting weird failures on Mac: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c5160f64ef5&filter-searchStr=Rev4%20MacOSX%20Snow%20Leopard%2010.6%20try%20debug%20test%20reftest and Windows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c5160f64ef5&filter-searchStr=Windows%207%2032-bit%20try%20opt%20test%20reftest We're failing to draw text decorations on some space-only textruns. I can't get this to reproduce locally on Mac or Windows, despite the fact it shows up 100% reliably on try on both platforms.

Jonathan, could you try reproducing these failures on Mac and/or Windows?

The patches here didn't apply cleanly on my tree; do they need some rebasing/updating? I might be able to do that locally if necessary, but maybe you have newer ones on hand already... Looking at the logs, I notice that the bits of text-decoration/complex-decoration-style-*.html that fail are the lines that use text-shadow. Is this because nsContextBoxBlur::Init doesn't set up a gfxContext at all if it's passed an empty rect (which the patches here will give for space-only textruns)? That would cause nsTextFrame::PaintOneShadow to take an early return, too, and not paint anything.

I bet you're right!!! Thanks!!! Actually it looks like a more general problem that shadowGfxRect/shadowRect doesn't include the area occupied by text decorations, if they extend out of the LOOSE_INK_EXTENTS text area...

Fixing the shadowRect didn't work :-(

This is why I was not seeing failures on Linux.

Updated with some refactoring of nsTextFrame to share more shadow-drawing code, and adding the shadowRect extension in there. We really should call something like nsTextFrame::UnionAdditionalOverflow here to handle cases where the decoration lines extend outside the font-box. But that's a separate bug.

Comment on attachment 8547328 [details] [diff] [review] Part 1.6: Make gfxFT2FontBase set mAdjustedSize Review of attachment 8547328 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFT2FontBase.cpp @@ +119,5 @@ > } else { > gfxFT2LockedFace face(this); > mFUnitsConvFactor = face.XScale(); > face.GetMetrics(&mMetrics, &mSpaceGlyph); > + mAdjustedSize = mStyle.size; I'm not sure this is right, in the case where font-size-adjust is in effect and an adjustment has been applied to the font pattern by gfxPangoFonts code, but won't be reflected in mStyle.size. I will try to look into this a bit more .... unless you want to convince me it's legitimate in the meantime. (It looks like the non-Pango FT2 backend, as used on mobile, doesn't implement font-size-adjust at all, so no discrepancy will show up in that case - but that's a separate bug we should also fix.)

Oops, we still have an SVG issue. Fix: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a24d78d40d3a

(In reply to Jonathan Kew (:jfkthame) from comment #25) > Comment on attachment 8547328 [details] [diff] [review] > Part 1.6: Make gfxFT2FontBase set mAdjustedSize > > Review of attachment 8547328 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/thebes/gfxFT2FontBase.cpp > @@ +119,5 @@ > > } else { > > gfxFT2LockedFace face(this); > > mFUnitsConvFactor = face.XScale(); > > face.GetMetrics(&mMetrics, &mSpaceGlyph); > > + mAdjustedSize = mStyle.size; > > I'm not sure this is right, in the case where font-size-adjust is in effect > and an adjustment has been applied to the font pattern by gfxPangoFonts > code, but won't be reflected in mStyle.size. I will try to look into this a > bit more .... unless you want to convince me it's legitimate in the meantime. > > (It looks like the non-Pango FT2 backend, as used on mobile, doesn't > implement font-size-adjust at all, so no discrepancy will show up in that > case - but that's a separate bug we should also fix.) I don't know how to proceed here. Does this mean we don't have anywhere in gfxFont a cross-platform way to check the size of the font after font-size-adjust? Is there any way we can initialize mAdjustedSize so it's at least as valid for gfxFT2FontBase as the current value (0.0)? Would it be OK for gfxFont::IsSpaceGlyphInvisible to check mStyle.size > 0 instead of mAdjustedSize >= 1.0? All I was trying to do there was avoid spurious "true" returns for zero-sized fonts.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27) > Is there any way we can initialize mAdjustedSize so it's at least as valid > for gfxFT2FontBase as the current value (0.0)? I'd really like to do something about this here because I got burned in this bug by mAdjustedSize being valid on all platforms except those using gfxFT2FontBase.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27) > (In reply to Jonathan Kew (:jfkthame) from comment #25) > > Comment on attachment 8547328 [details] [diff] [review] > > Part 1.6: Make gfxFT2FontBase set mAdjustedSize > > > > Review of attachment 8547328 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: gfx/thebes/gfxFT2FontBase.cpp > > @@ +119,5 @@ > > > } else { > > > gfxFT2LockedFace face(this); > > > mFUnitsConvFactor = face.XScale(); > > > face.GetMetrics(&mMetrics, &mSpaceGlyph); > > > + mAdjustedSize = mStyle.size; > > > > I'm not sure this is right, in the case where font-size-adjust is in effect > > and an adjustment has been applied to the font pattern by gfxPangoFonts > > code, but won't be reflected in mStyle.size. I will try to look into this a > > bit more .... unless you want to convince me it's legitimate in the meantime. > > > > (It looks like the non-Pango FT2 backend, as used on mobile, doesn't > > implement font-size-adjust at all, so no discrepancy will show up in that > > case - but that's a separate bug we should also fix.) > > I don't know how to proceed here. Does this mean we don't have anywhere in > gfxFont a cross-platform way to check the size of the font after > font-size-adjust? In theory, that's what gfxFont::GetAdjustedSize() ought to be; in practice, I think it's probably wrong on Linux. > Would it be OK for gfxFont::IsSpaceGlyphInvisible to check mStyle.size > 0 > instead of mAdjustedSize >= 1.0? All I was trying to do there was avoid > spurious "true" returns for zero-sized fonts. AFAICS, GetAdjustedSize() should work for that, even if the value it returns may not be accurate in the gfxPangoFonts case (as I suspect). (I also think we should fix the FT2 backends -- both gfxPangoFonts, unless John gets rid of it first, and gfxFT2Fonts -- so that they *do* set mAdjustedSize correctly, and then GetAdjustedSize can be optimized to be a simple, non-virtual accessor for that field. But we should take that to a separate bug.)

Thanks, this sounds good.

Comment on attachment 8547532 [details] [diff] [review] part 2 v3 Review of attachment 8547532 [details] [diff] [review]: ----------------------------------------------------------------- LGTM - I didn't actually test it, but if you're happy and tryserver is happy, then so am I. :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/02f84b2d07ba https://hg.mozilla.org/integration/mozilla-inbound/rev/953dbdb984be https://hg.mozilla.org/integration/mozilla-inbound/rev/5af10836905a https://hg.mozilla.org/integration/mozilla-inbound/rev/de30d7df906f

https://hg.mozilla.org/mozilla-central/rev/02f84b2d07ba https://hg.mozilla.org/mozilla-central/rev/953dbdb984be https://hg.mozilla.org/mozilla-central/rev/5af10836905a https://hg.mozilla.org/mozilla-central/rev/de30d7df906f