Remove various bits of dead code related to painting SVG text

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Assignee

Description

3 years ago
Spinning this off from bug 1248708 as per discussion with jeremychen on IRC.
Assignee

Comment 1

3 years ago
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.
Assignee

Updated

3 years ago
Attachment #8743302 - Attachment is patch: true
Assignee

Comment 3

3 years ago
Attachment #8743302 - Attachment is obsolete: true
Attachment #8743302 - Flags: review?(dholbert)
Attachment #8743309 - Flags: review?(dholbert)
Assignee

Comment 4

3 years ago
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 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?
Assignee

Comment 6

3 years ago
(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.
Assignee

Updated

3 years ago
Attachment #8743309 - Attachment is obsolete: true
Attachment #8743309 - Flags: review?(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.)
Attachment #8743381 - Flags: review?(dholbert) → review?(jfkthame)
Assignee

Comment 9

3 years ago
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.
Attachment #8743381 - Attachment is obsolete: true
Attachment #8743381 - Flags: review?(jfkthame)
Attachment #8743390 - Flags: review?(jfkthame)
Assignee

Comment 10

3 years ago
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.
Attachment #8743390 - Attachment description: patch to remove DrawMode propagation to nsSVGUtils::PaintSVGGlyph → part 1 - remove DrawMode propagation to nsSVGUtils::PaintSVGGlyph
Attachment #8743390 - Flags: review?(jfkthame) → review?(cam)
Assignee

Comment 11

3 years ago
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.
Assignee

Comment 12

3 years ago
(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.)
Assignee

Comment 13

3 years ago
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.
Attachment #8743517 - Flags: review?(cam)
Attachment #8743390 - Flags: review?(cam) → review+
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.
Assignee

Updated

3 years ago
Keywords: leave-open
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.
Attachment #8743517 - Flags: review?(cam) → review+
Filed bug 1266606 for that.
Assignee

Updated

3 years ago
Keywords: leave-open

Comment 20

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/642e7a61200e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.