Bug 31187 – Get rid of phase argument to Image::drawPattern

• Component: Platform
• Status: RESOLVED LATER

[Benjamin Otte]

Created attachment 42597 [details]
patch

The attach patch removes the phase argument from Image::dawPattern(). The argument can be expressed equally well as part of the patternTransform. 
In fact, all backends but the Qt one did exactly that manually anyway.

I only compiled and tested this patch on Linux/gtk. I did the work on all other backends too (it was just deleting code after all), but I won't wouch for it.

[Dirk Schulze]

The changes look reasonable to me. I would like to know, why phase was introduced? Oliver, do you know more about this?

> +    TransformationMatrix patternTransform = TransformationMatrix().translate(oneTileRect.x(), oneTileRect.y()).scaleNonUniform(scale.width(), scale.height());

I would like to see the transformations in two lines. Makes reading the code easier.
The ChangeLog needs the link to the bug report too.

Also did you check canvas examples like http://yuiblog.com/assets/slicing/slicing_test.html , or duke 3d? I had to check phase for NaN, otherwise the Cairo port crashed, or stop further rendering (I guess it's a problem of Cairo and handling not invertible matrices).

I'll run pixeltests on Mac, if there are no reasons for phase.

[Benjamin Otte]

(disclaimer: the patch doesn't apply anymore since the colorspace stuff landed)

And yeah, we should probably do a cairo_pattern_status() before doing the cairo_set_source() to avoid bad matrices putting the cairo_t into an error state. The current check is not very good, because it just catches one special case.
Although that won't protect us when only the context's tansform plus the pattern's transform ens up being bad.
Solving that problem correctly requires work in Cairo.
And a testcase exposing it. :/

[Dirk Schulze]

(In reply to comment #2)
> (disclaimer: the patch doesn't apply anymore since the colorspace stuff landed)
> 
> And yeah, we should probably do a cairo_pattern_status() before doing the
> cairo_set_source() to avoid bad matrices putting the cairo_t into an error
> state. The current check is not very good, because it just catches one special
> case.
> Although that won't protect us when only the context's tansform plus the
> pattern's transform ens up being bad.
> Solving that problem correctly requires work in Cairo.
> And a testcase exposing it. :/

One possibiliy is, to make every transformation to TransformationMatrix& patternTransform and check if it is invertible (.isInvertible()) before we transform the pattern. Nevertheless, I would like to see this with this patch.

[Benjamin Otte]

Created attachment 44979 [details]
patch v2

Updated to trunk with Dirk's comments.

The example page you linked to had other bugs, but it isn't using drawPattern at all, so doesn't apply to this bug. And I can't find the Duke one on Google, got a link for that?

[WebKit Review Bot]

Attachment 44979 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/cairo/ImageCairo.cpp:200:  phase_matrix is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/platform/graphics/cairo/ImageCairo.cpp:205:  Extra space before ( in function call  [whitespace/parens] [4]
WebCore/platform/graphics/cairo/ImageCairo.cpp:206:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3

[Darin Adler]

Comment on attachment 44979 [details]
patch v2

This looks like a good idea to me.

> +    TransformationMatrix patternTransform = TransformationMatrix();
> +    patternTransform.translate(oneTileRect.x(), oneTileRect.y());
> +    patternTransform.scaleNonUniform(scale.width(), scale.height());

> +    TransformationMatrix patternTransform = TransformationMatrix().translate(dstRect.x() - hPhase, dstRect.y() - vPhase).scaleNonUniform(scale.width(), scale.height());

Why the different idiom at these two call sites? I think you should choose one style or the other.

[Benjamin Otte]

Created attachment 44991 [details]
patch v3

That might be because I didn't notice there's two occurances. Fixed this and the style issues now.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44991 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 44991 [details]
patch v3

Clearing flags on attachment: 44991

Committed r52206: <http://trac.webkit.org/changeset/52206>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Looks like this caused a bunch of pixel test failures.  I recommend we roll this out.

[Eric Seidel (no email)]

Specifically the pixel tests fail on Mac Leopard and can be seen in the Chromium Mac build bots:
http://build.chromium.org/buildbot/layout_test_results/webkit-rel-mac-webkit-org/34759/

The failures can also be seen when running locally on a mac machine running Apple's mac webkit build.

[Benjamin Otte]

Yeah, if it breaks stuff in some builds, they should be fixed, so back it out. (Just don't expect me to fix it, please, I don't have anything but Linux).

[WebKit Commit Bot]

Committed r52229: <http://trac.webkit.org/changeset/52229>

[Eric Seidel (no email)]

Rolled out, see above.

[Martin Robinson]

Going to close this one for the moment, since it hasn't seen activity for 8 years or so.