Apply tranformation to pattern locally to fix some layout tests....

Apply tranformation to pattern locally to fix some layout tests.
Note that we avoid using positive translations that causes problems
in Skia. See the changes for more details.

Fixed the following layout tests:
LayoutTests/svg/batik/paints/patternRegionA.svg
LayoutTests/svg/W3C-SVG-1.1/pservers-grad-06-b.svg
LayoutTests/svg/W3C-SVG-1.1/pservers-pattern-01-b.svg
LayoutTests/svg/W3C-SVG-1.1/coords-units-01-b.svg

Change list for re-baselineing the above tests:
http://codereview.chromium.org/43105/show

[Evan Stade, 2009-03-11 23:28:02]

some nits:

http://codereview.chromium.org/42100/diff/1/2
File WebCore/platform/graphics/skia/PatternSkia.cpp (right):

http://codereview.chromium.org/42100/diff/1/2#newcode43
Line 43: // Note: patternTransform is ignored since it is not a local
transformation
this comment is really cryptic. Can you make it clearer? Perhaps:

|patternTransform| is ignored because it is a global transformation applied by
the GraphicsContext. Pattern-specific transformations are stored in
|m_patternSpaceTransformation|.

http://codereview.chromium.org/42100/diff/1/2#newcode50
Line 50: // translations, so make them negative only in tile mode.
should read: "But Skia seems to have a problem with positive translations in
tile mode, so adjust the translation to an equivalent negative value if we are
in tile mode."

http://codereview.chromium.org/42100/diff/1/2#newcode53
Line 53: //   transform.g() == 0   (Y translation)
this line irrelevant to your example.

http://codereview.chromium.org/42100/diff/1/2#newcode61
Line 61: //   translateX = translateX - width * ceil(translateX / width);
lines 60-61 are confusing since translateX here is not used to mean the same
thing as the variable translateX below. I'd just delete these two lines if I
were you; I think the text explanation + the code is sufficient.

http://codereview.chromium.org/42100/diff/1/2#newcode81
Line 81: // Skia does not have a "draw the tile only once" option.
Clamp_TileMode
I think that it would be better if you don't call shader->setLocalMatrix()
twice. To avoid this, you'll have to make a local |shader| variable and put the
code below into a giant else{ } case, but I think it's worth it so that future
developers don't accidentally touch one code path and not the other.

(that's actually how it used to be the last time we applied a transform in this
function, before mmoss's change)

[Alpha Left Google, 2009-03-12 00:02:17]

http://codereview.chromium.org/42100/diff/1/2
File WebCore/platform/graphics/skia/PatternSkia.cpp (right):

http://codereview.chromium.org/42100/diff/1/2#newcode43
Line 43: // Note: patternTransform is ignored since it is not a local
transformation
On 2009/03/11 23:28:02, estade wrote:
> this comment is really cryptic. Can you make it clearer? Perhaps:
> 
> |patternTransform| is ignored because it is a global transformation applied by
> the GraphicsContext. Pattern-specific transformations are stored in
> |m_patternSpaceTransformation|.

Done.

http://codereview.chromium.org/42100/diff/1/2#newcode50
Line 50: // translations, so make them negative only in tile mode.
On 2009/03/11 23:28:02, estade wrote:
> should read: "But Skia seems to have a problem with positive translations in
> tile mode, so adjust the translation to an equivalent negative value if we are
> in tile mode."

Done.

http://codereview.chromium.org/42100/diff/1/2#newcode53
Line 53: //   transform.g() == 0   (Y translation)
On 2009/03/11 23:28:02, estade wrote:
> this line irrelevant to your example.

Done.

http://codereview.chromium.org/42100/diff/1/2#newcode61
Line 61: //   translateX = translateX - width * ceil(translateX / width);
On 2009/03/11 23:28:02, estade wrote:
> lines 60-61 are confusing since translateX here is not used to mean the same
> thing as the variable translateX below. I'd just delete these two lines if I
> were you; I think the text explanation + the code is sufficient.

Done.

http://codereview.chromium.org/42100/diff/1/2#newcode81
Line 81: // Skia does not have a "draw the tile only once" option.
Clamp_TileMode
On 2009/03/11 23:28:02, estade wrote:
> I think that it would be better if you don't call shader->setLocalMatrix()
> twice. To avoid this, you'll have to make a local |shader| variable and put
the
> code below into a giant else{ } case, but I think it's worth it so that future
> developers don't accidentally touch one code path and not the other.
> 
> (that's actually how it used to be the last time we applied a transform in
this
> function, before mmoss's change)

Done.

[Evan Stade, 2009-03-12 00:52:05]

a couple more nits, aside from that LGTM, good find

http://codereview.chromium.org/42100/diff/1/2
File WebCore/platform/graphics/skia/PatternSkia.cpp (right):

http://codereview.chromium.org/42100/diff/1/2#newcode50
Line 50: // translations, so make them negative only in tile mode.
I think you forgot this one?

http://codereview.chromium.org/42100/diff/3001/3002#newcode44
Line 44: // for the pattern but rather a global transformation for which this
pattern
nit: grammar. Should be "The pattern-specific..."

http://codereview.chromium.org/42100/diff/3001/3002#newcode75
Line 75: if (m_repeatX && m_repeatY) {
nit: grammar. Should be "The local transform ..., and we need to..."

http://codereview.chromium.org/42100/diff/3001/3002#newcode82
Line 82: // repeats the last line of the image after drawing one tile. To avoid
"largest negative number" is ambiguous. It's kind of hard to get around, though.
Maybe this intro could read "An equivalent negative translation would be 56 less
some multiple of 20, e.g. -6, -26, etc. We can obtain the first such number that
satisfies this property by ... "

http://codereview.chromium.org/42100/diff/3001/3002#newcode86
Line 86: SkShader::TileMode tileModeX = m_repeatX ? SkShader::kRepeat_TileMode :
SkShader::kClamp_TileMode;
cam you move lines 86-87 to be directly above the code they refer to?

[Alpha Left Google, 2009-03-12 01:05:00]

http://codereview.chromium.org/42100/diff/1/2
File WebCore/platform/graphics/skia/PatternSkia.cpp (right):

http://codereview.chromium.org/42100/diff/1/2#newcode50
Line 50: // translations, so make them negative only in tile mode.
On 2009/03/12 00:52:05, estade wrote:
> I think you forgot this one?

Done.

http://codereview.chromium.org/42100/diff/3001/3002#newcode75
Line 75: if (m_repeatX && m_repeatY) {
On 2009/03/12 00:52:06, estade wrote:
> nit: grammar. Should be "The local transform ..., and we need to..."

Done.

http://codereview.chromium.org/42100/diff/3001/3002#newcode82
Line 82: // repeats the last line of the image after drawing one tile. To avoid
On 2009/03/12 00:52:06, estade wrote:
> "largest negative number" is ambiguous. It's kind of hard to get around,
though.
> Maybe this intro could read "An equivalent negative translation would be 56
less
> some multiple of 20, e.g. -6, -26, etc. We can obtain the first such number
that
> satisfies this property by ... "

Done.

http://codereview.chromium.org/42100/diff/3001/3002#newcode86
Line 86: SkShader::TileMode tileModeX = m_repeatX ? SkShader::kRepeat_TileMode :
SkShader::kClamp_TileMode;
On 2009/03/12 00:52:06, estade wrote:
> cam you move lines 86-87 to be directly above the code they refer to?

Done.

[Evan Stade, 2009-03-12 01:15:12]

lgtm

[brettw, 2009-03-18 18:19:55]

I would like to let the Skia folks know about this before we work around
something. If it is a bug, it should be fixed there, or we should know a fix is
pending before checking in.