Bug 26618 – [Chromium] Gradient and pattern text failing on Chromium/Linux

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Stephen White]

The following layout tests are failing on Chromium/Linux:

LayoutTests/svg/custom/js-late-gradient-creation.svg (bad baseline PNG)
LayoutTests/svg/custom/js-late-gradient-and-object.creation.svg
LayoutTests/svg/custom/js-late-pattern-creation.svg (bad baseline PNG)
LayoutTests/svg/custom/js-late-pattern-and-object-creation.svg

[Stephen White]

Created attachment 31662 [details]
Patterns/Gradients refactoring for Skia impl.

[Brett Wilson (Google)]

Comment on attachment 31662 [details]
Patterns/Gradients refactoring for Skia impl.

I think this definitely looks better than the old code.

> @@ -945,6 +930,25 @@ void GraphicsContext::setPlatformFillCol
>      if (paintingDisabled())
>          return;
>      platformContext()->setFillColor(color.rgb());
> +    platformContext()->setFillShader(NULL);

I'm not clear why this new call was necessary. Is this something setFillColor should be doing internally because fill colors and fill shaders are mutually exclusive?

>      platformContext()->setStrokeColor(strokecolor.rgb());
> +    platformContext()->setStrokeShader(NULL);

Same here.

[Stephen White]

(In reply to comment #2)
> (From update of attachment 31662 [details] [review])
> I think this definitely looks better than the old code.
> 
> > @@ -945,6 +930,25 @@ void GraphicsContext::setPlatformFillCol
> >      if (paintingDisabled())
> >          return;
> >      platformContext()->setFillColor(color.rgb());
> > +    platformContext()->setFillShader(NULL);
> 
> I'm not clear why this new call was necessary. Is this something setFillColor
> should be doing internally because fill colors and fill shaders are mutually
> exclusive?

Yes, they're mutually exclusive.  Each shader (fill and stroke) can be either NULL (for solid color), gradient shader, or pattern shader.  Basically I'm using the shader to indicate the same thing as state.fillColorSpace and state.strokeColorSpace (which we'd otherwise have to forward down into the platform layer as well).  It seemed redundant, so I didn't bother.

> 
> >      platformContext()->setStrokeColor(strokecolor.rgb());
> > +    platformContext()->setStrokeShader(NULL);
> 
> Same here.
> 

[Darin Fisher (:fishd, Google)]

Stylistically, this change looks good to me, but I'm not expert on the Skia related changes, so I need Brett to say "LGTM" before I'll R+ the patch.

[Brett Wilson (Google)]

(In reply to comment #3)
> > >      platformContext()->setFillColor(color.rgb());
> > > +    platformContext()->setFillShader(NULL);
> > 
> > I'm not clear why this new call was necessary. Is this something setFillColor
> > should be doing internally because fill colors and fill shaders are mutually
> > exclusive?
> 
> Yes, they're mutually exclusive.  Each shader (fill and stroke) can be either
> NULL (for solid color), gradient shader, or pattern shader.  Basically I'm
> using the shader to indicate the same thing as state.fillColorSpace and
> state.strokeColorSpace (which we'd otherwise have to forward down into the
> platform layer as well).  It seemed redundant, so I didn't bother.

I was suggesting that PlatformContextSkia::setFollColor (and the stroke equivalent) should internally clear the shader so callers don't have to remember to do both, assuming they're mutually exclusive. What do you think?

[Eric Seidel (no email)]

Comment on attachment 31662 [details]
Patterns/Gradients refactoring for Skia impl.

Won't this doe the wrong thing if the ctm is changed after the pattern is set on the context?

void GraphicsContext::setPlatformFillPattern(Pattern* pattern)
 945 {
 946     if (paintingDisabled())
 947         return;
 948 
 949     SkShader* pat = pattern->createPlatformPattern(getCTM());
 950     platformContext()->setFillShader(pat);
 951     pat->safeUnref();

Long ago, I added these setPlatform* methods to GraphicsContext.  They were later removed because they couldn't work for CG, because CG needs the transform at pattern creation time not at draw time.  It seems that here you're going to end up using the wrong CTM, unless you are later updating the pattern CTM before drawing?

[Stephen White]

(In reply to comment #6)
> (From update of attachment 31662 [details] [review])
> Won't this doe the wrong thing if the ctm is changed after the pattern is set
> on the context?

Yes, I was concerned about this as well.  However, notice this comment in
PatternSkia.cpp:

PlatformPatternPtr Pattern::createPlatformPattern(const TransformationMatrix& patternTransform) const
{
    // Note: patternTransform is ignored since it seems to be applied elsewhere
    // (when the pattern is used?). Applying it to the pattern (i.e.
    // shader->setLocalMatrix) results in a double transformation.

There's no further reference to patternTransform in that function, and since all the layout 
tests pass with my change, I think it's okay.

> Long ago, I added these setPlatform* methods to GraphicsContext.  They were
> later removed because they couldn't work for CG, because CG needs the transform
> at pattern creation time not at draw time. It seems that here you're going to
> end up using the wrong CTM, unless you are later updating the pattern CTM
> before drawing?

I believe this is what happens (it's applied at draw time).  I don't think Skia needs
the transform matrix to be baked into the pattern the way CG does.

[Stephen White]

(In reply to comment #5)
> (In reply to comment #3)
> > > >      platformContext()->setFillColor(color.rgb());
> > > > +    platformContext()->setFillShader(NULL);
> > > 
> > > I'm not clear why this new call was necessary. Is this something setFillColor
> > > should be doing internally because fill colors and fill shaders are mutually
> > > exclusive?
> > 
> > Yes, they're mutually exclusive.  Each shader (fill and stroke) can be either
> > NULL (for solid color), gradient shader, or pattern shader.  Basically I'm
> > using the shader to indicate the same thing as state.fillColorSpace and
> > state.strokeColorSpace (which we'd otherwise have to forward down into the
> > platform layer as well).  It seemed redundant, so I didn't bother.
> 
> I was suggesting that PlatformContextSkia::setFollColor (and the stroke
> equivalent) should internally clear the shader so callers don't have to
> remember to do both, assuming they're mutually exclusive. What do you think?

[Could've sworn I replied to this already; sorry if you get two replies]

There's one place this would be problematic:  internally, PlatformContextSkia::drawRect() 
calls setFillColor() when drawing rect borders.  If this also reset the shader, it would 
then leave it NULL after the draw.

Since GraphicsContextSkia.cpp is the one which translates from WebKit Gradients to Skia 
shaders, I kind of prefer that it's the one that enforces the mutual exclusion. I think it 
would be misleading if PlatformContextSkia::setFillColor() reset the shader, since 
every other setXXX() call in PlatformContextSkia changes only the associated state variable.

I don't want to bikeshed about it, though, so if you think I haven't made my case, I'll 
change it.

[Brett Wilson (Google)]

(In reply to comment #8)
> (In reply to comment #5)
> > (In reply to comment #3)
> > > > >      platformContext()->setFillColor(color.rgb());
> > > > > +    platformContext()->setFillShader(NULL);
> > > > 
> > > > I'm not clear why this new call was necessary. Is this something setFillColor
> > > > should be doing internally because fill colors and fill shaders are mutually
> > > > exclusive?
> > > 
> > > Yes, they're mutually exclusive.  Each shader (fill and stroke) can be either
> > > NULL (for solid color), gradient shader, or pattern shader.  Basically I'm
> > > using the shader to indicate the same thing as state.fillColorSpace and
> > > state.strokeColorSpace (which we'd otherwise have to forward down into the
> > > platform layer as well).  It seemed redundant, so I didn't bother.
> > 
> > I was suggesting that PlatformContextSkia::setFollColor (and the stroke
> > equivalent) should internally clear the shader so callers don't have to
> > remember to do both, assuming they're mutually exclusive. What do you think?
> 
> [Could've sworn I replied to this already; sorry if you get two replies]
> 
> There's one place this would be problematic:  internally,
> PlatformContextSkia::drawRect() 
> calls setFillColor() when drawing rect borders.  If this also reset the shader,
> it would 
> then leave it NULL after the draw.

Actually, it looks like you may have broken this code. It's trying to set up an opaque color tho fill so as to emulate a line. Are you sure you want to apply the fill shader to those rects?


> Since GraphicsContextSkia.cpp is the one which translates from WebKit Gradients
> to Skia 
> shaders, I kind of prefer that it's the one that enforces the mutual exclusion.
> I think it 
> would be misleading if PlatformContextSkia::setFillColor() reset the shader,
> since 
> every other setXXX() call in PlatformContextSkia changes only the associated
> state variable.

Are there any other ones that are mutually exclusive?

[Stephen White]

(In reply to comment #9)
> (In reply to comment #8)
> > There's one place this would be problematic:  internally,
> > PlatformContextSkia::drawRect() 
> > calls setFillColor() when drawing rect borders.  If this also reset the shader,
> > it would 
> > then leave it NULL after the draw.
> 
> Actually, it looks like you may have broken this code. It's trying to set up an
> opaque color tho fill so as to emulate a line. Are you sure you want to apply
> the fill shader to those rects?

No, I don't think it should -- good catch.  Fixed.

Since I had to set it to NULL anyway, I went ahead and just did it in 
setFillColor() and setStrokeColor() as requested (less code tipped it
for me. :)

> > Since GraphicsContextSkia.cpp is the one which translates from WebKit Gradients
> > to Skia 
> > shaders, I kind of prefer that it's the one that enforces the mutual exclusion.
> > I think it 
> > would be misleading if PlatformContextSkia::setFillColor() reset the shader,
> > since 
> > every other setXXX() call in PlatformContextSkia changes only the associated
> > state variable.
> 
> Are there any other ones that are mutually exclusive?

Stroke color / pattern / gradient are also mutually exclusive, if that's
what you meant.  If you mean, in the wider API outside of gradients and
patterns, I don't know.

[Stephen White]

Created attachment 32311 [details]
2nd rev of above -- outlined rect fix, setXXXColor() also reset shaders

[Stephen White]

Created attachment 32326 [details]
3rd rev - setFillColor() unconditionally in rect border code, so shader is always reset

Also removed names from some unused parameters, to silence warnings (treated as errors in Mac Webkit build).

[Brett Wilson (Google)]

This looks better, but I think set...Color and set...Shader should be consistent, so one should clear the other. The way you have it not, setStroke/FillColor clears the shader, but not vice-versa.

[Stephen White]

(In reply to comment #13)
> This looks better, but I think set...Color and set...Shader should be
> consistent, so one should clear the other. The way you have it not,
> setStroke/FillColor clears the shader, but not vice-versa.

What do you think it should it clear it to?  Black or white?  Alpha of zero or one?  Fuchsia or chartreuse?  :)

[Brett Wilson (Google)]

(In reply to comment #14)
> (In reply to comment #13)
> > This looks better, but I think set...Color and set...Shader should be
> > consistent, so one should clear the other. The way you have it not,
> > setStroke/FillColor clears the shader, but not vice-versa.
> 
> What do you think it should it clear it to?  Black or white?  Alpha of zero or
> one?  Fuchsia or chartreuse?  :)

Good point, seems fine to leave it, then.

[Stephen White]

(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #13)
> > > This looks better, but I think set...Color and set...Shader should be
> > > consistent, so one should clear the other. The way you have it not,
> > > setStroke/FillColor clears the shader, but not vice-versa.
> > 
> > What do you think it should it clear it to?  Black or white?  Alpha of zero or
> > one?  Fuchsia or chartreuse?  :)
> 
> Good point, seems fine to leave it, then.

LGTY, then?

[Brett Wilson (Google)]

LGTM

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/46017