Bug 31187

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.

(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. :/

(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.

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?

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

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.

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.

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

Comment on attachment 44991 [details] patch v3 Clearing flags on attachment: 44991 Committed r52206: <http://trac.webkit.org/changeset/52206>

All reviewed patches have been landed. Closing bug.

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

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.

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).

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

Rolled out, see above.

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