1357545 - Handle text decoration

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P3)

Description

[Jeff Muizelaar [:jrmuizel]]

Currently we fail to use text layers when text decoration happens. This causes noticeable fallback when hover over links.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Our current thoughts for the way forward here is to change nsDisplayText::RenderToContext to take a new TextPainter class instead of a gfxContext. The TextPainter would wrap a DrawTarget like gfxContext currently does and either forward commands through to the DrawTarget or record the needed information for use by WebRender.

A good first step would probably be to just replace the gfxContext with a TextPainter class that has a more DrawTarget like API. This way we avoid having to use the gfxContext wrapper in the first place.

Comment 2

[Aria Beingessner [:Gankra]]

Attachment: Bug 1357545 - handle text-shadows/decorations with webrender (layers-free)

This replaces our DrawTargetCapture hack with a similar but more powerful TextDrawTarget
hack. The old design had several limitations:

* It couldn't handle shadows
* It couldn't handle selections
* It couldn't handle font/color changes in a single text-run
* It couldn't handle decorations (underline, overline, line-through)

Mostly this was a consequence of the fact that it only modified the start and end
of the rendering algorithm, and therefore couldn't distinguish draw calls for different
parts of the text.

This new design is based on a similar principle as DrawTargetCapture, but also passes
down the TextDrawTarget in the drawing arguments, so that the drawing algorithm can
notify us of changes in phase (e.g. "now we're doing underlines"). This also lets us
directly pass data to TextDrawTarget when possible (as is done for shadows and selections).

In doing this, I also improved the logic copied from ContainsOnlyColoredGlyphs to handle
changes in font/color mid-text-run (which can happen because of font fallback).

The end result is:

* We handle all shadows natively
* We handle all selections natively
* We handle font/color changes in a single text-run
    * We still hackily intercept draw calls
    * But we don't need to buffer commands, reducing total memcopies
* We handle decorations (but only as blobs)

In addition, this changes integrates webrender's PushTextShadow API, which was designed for
this use case. This is only done in the layerless path; WebrenderTextLayer continues to be
semantically limited. In theory WebrenderTextLayer can/should be removed, but I didn't want
to leave TextLayer without any consumer, and I've been advised to keep TextLayer working.

This also doesn't modify TextLayers, to minimize churn. In theory they can be augmented to support
the richer semantics that TextDrawTarget has, but there's little motivation since the API is
largely unused with this change.

Review commit: https://reviewboard.mozilla.org/r/156382/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156382/

Comment 3

[Aria Beingessner [:Gankra]]

Attachment: WIP: update webrender

Review commit: https://reviewboard.mozilla.org/r/156384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156384/

Comment 4

[Aria Beingessner [:Gankra]]

Hey [:jrmuizel], here's the changes mostly cleaned up and ready to go. The first patch is the real work, the second patch is just a temporary thing to be replaced by a proper WR update. There's a few TODO's left in the patch that I'd like to address, but I'd rather get feedback on the design first before progressing. I expect a few of these might be left in the final submission as follow-up work.

Discussing with glennw, it looks like we're going to push forward with native text-decoration support as an immediate follow-up, so I'll be hacking away in this code some more after this patch.

Also: not sure if these changes are at all visible to any of the testing bots?

Comment 5

[Kartikaya Gupta (email:[e-mail])]

(In reply to Alexis Beingessner [:Gankro] from comment #4)
> Also: not sure if these changes are at all visible to any of the testing
> bots?

Not unless you push to try.

Comment 6

[Aria Beingessner [:Gankra]]

Comment on attachment 8885508 [details]
Bug 1357545 - handle text-shadows/decorations with webrender (layers-free)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156382/diff/1-2/

Comment 7

[Aria Beingessner [:Gankra]]

Comment on attachment 8885509 [details]
WIP: update webrender

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156384/diff/1-2/

Comment 8

[Aria Beingessner [:Gankra]]

Comment on attachment 8885508 [details]
Bug 1357545 - handle text-shadows/decorations with webrender (layers-free)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156382/diff/2-3/

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 8885508 [details]
Bug 1357545 - handle text-shadows/decorations with webrender (layers-free)

https://reviewboard.mozilla.org/r/156382/#review174880

This seems like it should be OK, but webrender and layers and all that is pretty much foreign territory to me. So consider this "feedback+", but Jeff should do the real review.

::: layout/generic/TextDrawTarget.h:76
(Diff revision 3)
> +  // Prevent this from being copied
> +  TextDrawTarget(const TextDrawTarget& src) = delete;
> +  TextDrawTarget& operator=(const TextDrawTarget&) = delete;
> +
> +  // Change the phase of text we're drawing.
> +  void StartDrawing(Phase aPhase) { mCurrentlyDrawing = aPhase; }

As a sanity-check, might be worth asserting here that we're not attempting to draw a phase out of order?

::: layout/generic/TextDrawTarget.h:94
(Diff revision 3)
> +
> +    // Make sure we're only given color patterns
> +    MOZ_RELEASE_ASSERT(aPattern.GetType() == PatternType::COLOR);
> +    const ColorPattern* colorPat = static_cast<const ColorPattern*>(&aPattern);
> +
> +    // Grab the font, make sure it's the same one we've always seen

The second part of this comment doesn't seem to be correct.

::: layout/generic/TextDrawTarget.h:231
(Diff revision 3)
> +  // We wrap a gfxContext now, but we aspire to only use DrawTarget.
> +  // gfxContext* mContext;

We wrap a commented-out gfxContext? :)

::: layout/generic/nsTextFrame.cpp:5184
(Diff revision 3)
> +      mTextDrawer->GetBeforeDecorations().Length() > 0 ||
> +      mTextDrawer->GetAfterDecorations().Length() > 0) {
> -  return mozilla::LAYER_NONE;
> +    return mozilla::LAYER_NONE;
> -}
> +  }
>  
> +  // Most only have one font (multiple colors is fine)

typo, s/Most/Must/

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8885508 [details]
Bug 1357545 - handle text-shadows/decorations with webrender (layers-free)

https://reviewboard.mozilla.org/r/156382/#review175042

Comment 11

[Aria Beingessner [:Gankra]]

Comment on attachment 8885508 [details]
Bug 1357545 - handle text-shadows/decorations with webrender (layers-free)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156382/diff/3-4/

Comment 12

[Aria Beingessner [:Gankra]]

Comment on attachment 8885508 [details]
Bug 1357545 - handle text-shadows/decorations with webrender (layers-free)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156382/diff/4-5/

Comment 13

[Aria Beingessner [:Gankra]]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=25537c6402bcb9bdfd58d41c518539fb0ca4056d&selectedJob=123971813

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/564959d26e8d
handle text-shadows/decorations with webrender (layers-free) r=jrmuizel

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/564959d26e8d