1266101 - Remove various bits of dead code related to painting SVG text

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Spinning this off from bug 1248708 as per discussion with jeremychen on IRC.

Comment 1

[Jonathan Watt [:jwatt]]

One issue is nsSVGUtils::PaintSVGGlyph. Even at the time this method was introduced (at the time nsContentUtils::PaintSVGGlyph) the DrawMode argument was unused:

  http://hg.mozilla.org/mozilla-central/rev/a0c93525cfba

It actually makes sense that it would be unused, since when painting SVG glyphs whether we paint the fill, stroke or both is controlled by the paint servers provided by the gfxTextContextPaint argument. In addition to the DrawMode argument being removable, there is only one linear call path into this function:

  nsSVGUtils::PaintSVGGlyph
  gfxSVGGlyphs::RenderGlyph
  gfxFontEntry::RenderSVGGlyph
  gfxFont::RenderSVGGlyph
  gfxFont::RenderSVGGlyph
  gfxFont::DrawOneGlyph

In DrawOneGlyph the ForcePaintingDrawMode call ensures that we never pass GLYPH_PATH in, which makes the check for that in gfxSVGGlyphs::RenderGlyph useless. We can therefore remove the DrawMode arguments all the way up this call stack, and also remove ForcePaintingDrawMode.

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch to remove DrawMode propagation to nsSVGUtils::PaintSVGGlyph

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch to remove DrawMode propagation to nsSVGUtils::PaintSVGGlyph

Comment 4

[Jonathan Watt [:jwatt]]

I've modified the patch locally to add the following comment above the |runParams.drawMode != DrawMode::GLYPH_PATH| check:

        // It doesn't make sense to try and draw SVG glyphs using GLYPH_PATH
        // to extract a path that we can fill and stroke.  Despite containting
        // vector information, SVG glyphs are designed to be treated as an
        // image.  Only they know how to fill and stroke themselves.

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 8743309 [details] [diff] [review]
patch to remove DrawMode propagation to nsSVGUtils::PaintSVGGlyph

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

::: gfx/thebes/gfxFont.cpp
@@ +1785,5 @@
>          if (!runParams.paintSVGGlyphs) {
>              return;
>          }
> +        if (runParams.drawMode != DrawMode::GLYPH_PATH &&
> +            RenderSVGGlyph(runParams.context, devPt, mode,

This will lead to a change in behavior, won't it? In the case when runParams.drawMode is GLYPH_PATH, the existing code would change that to FILL|STROKE, call RenderSVGGlyph, and return; but with this change, it will do nothing, and fall through to the rest of the method.

I'm not sure that's desirable. If we're doing a background-clip:text in order to fill text with a gradient, image, etc., then I think we decided that colored glyphs (which would include SVG glyphs) should retain their normal rendering, not attempt to become part of the clip path. In which case we also don't want to fall through to getting the path of the "normal" glyph instead.

So I wonder if this should be more like

  if (drawMode == GLYPH_PATH) {
    if (GetFontEntry()->HasSVGGlyph(aGlyphID))
      return; // SVG glyph cannot contribute to a path
  } else {
    if (RenderSVGGlyph(...))
      return; // Found and rendered an SVG glyph
  }

But I may be confused.... WDYT?

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> This will lead to a change in behavior, won't it?

Err, yes, I screwed that up. I even said in comment 1 that ForcePaintingDrawMode was useless, then messed up the code.

> So I wonder if this should be more like
> 
>   if (drawMode == GLYPH_PATH) {
>     if (GetFontEntry()->HasSVGGlyph(aGlyphID))
>       return; // SVG glyph cannot contribute to a path
>   } else {
>     if (RenderSVGGlyph(...))
>       return; // Found and rendered an SVG glyph
>   }

We could do that, but to keep the current behavior unaltered I think I should just remove the |runParams.drawMode != DrawMode::GLYPH_PATH| check.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: patch to remove DrawMode propagation to nsSVGUtils::PaintSVGGlyph

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8743381 [details] [diff] [review]
patch to remove DrawMode propagation to nsSVGUtils::PaintSVGGlyph

(I'll punt this review to jfkthame since he's already started taking a look at this.)

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: part 1 - remove DrawMode propagation to nsSVGUtils::PaintSVGGlyph

Seems NS_WARN_IF_FALSE and NS_WARN_IF have different semantics and I need to use the former if I want to provide a message.

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 8743390 [details] [diff] [review]
part 1 - remove DrawMode propagation to nsSVGUtils::PaintSVGGlyph

Digging further into this code it seems heycam wrote some of the modifications that are dead (not removed in this patch) so probably best to get reviews from him on this bug.

Comment 11

[Jonathan Watt [:jwatt]]

In http://hg.mozilla.org/mozilla-central/rev/bfef9b308f92 heycam added GLYPH_STROKE_UNDERNEATH but there were no tests added that would test the related code paths as far as I can tell.

Comment 12

[Jonathan Watt [:jwatt]]

(As noted in bug 1248708 comment 58, adding support for '-webikit-text-stroke' will mean that 'paint-order' can usefully apply to HTML text and maybe some of this "dead code" will actually be useful. Unclear to me yet.)

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Remove most of the GLYPH_STROKE/GLYPH_STROKE_UNDERNEATH checking code

At any rate, _currently_ the only code setting DrawMode::GLYPH_STROKE/GLYPH_STROKE_UNDERNEATH is SVGTextFrame::SetupContextPaint, and that only passes it up to its single caller SVGTextFrame::PaintSVG where it propagates no further. So the rest of the DrawMode::GLYPH_STROKE/GLYPH_STROKE_UNDERNEATH checking code seems to be dead in which case we can remove this.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8743517 [details] [diff] [review]
part 2 - Remove most of the GLYPH_STROKE/GLYPH_STROKE_UNDERNEATH checking code

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

I guess I expected to make regular SVG stroked text use GLYPH_STROKE_UNDERNEATH if paint-order said so, but since we're going through the paint-as-path path whenever we have a stroke, it's not needed currently.

So one thing to test would be if we can start using GLYPH_STROKE when painting SVG text with solid, opaque colours.  Try changing the condition in SVGTextFrame::ShouldRenderAsPath not to return true if we have a stroke at all, but only if the stroke has a non-solid color paint and doesn't have dashing.  If so, then I think we should use gfx's handling of text stroking.  If that works, then I think we should keep GLYPH_STROKE_UNDERNEATH so that we wouldn't have to return true from SVGTextFrame::ShouldRenderAsPath if paint-order indicated strokes go on top.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/173a441d7bb1

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/173a441d7bb1

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8743517 [details] [diff] [review]
part 2 - Remove most of the GLYPH_STROKE/GLYPH_STROKE_UNDERNEATH checking code

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

Since this is complicating the patches in bug 1248708, let's just land this now, and if someone later wants to try to get the SVG text rendering code to use the gfxFont stroke painting functionality, we can do that later.  I'll file a followup for that.

Comment 18

[Cameron McCormack (:heycam)]

Filed bug 1266606 for that.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/642e7a61200e

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/642e7a61200e