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
Created attachment 31662 [details] Patterns/Gradients refactoring for Skia impl.
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.
(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. >
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.
(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?
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?
(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.
(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.
(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?
(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.
Created attachment 32311 [details] 2nd rev of above -- outlined rect fix, setXXXColor() also reset shaders
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).
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.
(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? :)
(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.
(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?
LGTM
Landed as: http://trac.webkit.org/changeset/46017