Bug 31187

Summary: Get rid of phase argument to Image::drawPattern
Product: WebKit Reporter: Benjamin Otte <otte>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED LATER    
Severity: Normal CC: commit-queue, eric, krit, mrobinson, oliver, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch
none
patch v2
none
patch v3 none

Description Benjamin Otte 2009-11-05 14:25:25 PST
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.
Comment 1 Dirk Schulze 2009-12-05 23:48:13 PST
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.
Comment 2 Benjamin Otte 2009-12-07 04:13:13 PST
(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. :/
Comment 3 Dirk Schulze 2009-12-07 13:16:17 PST
(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.
Comment 4 Benjamin Otte 2009-12-16 08:28:31 PST
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?
Comment 5 WebKit Review Bot 2009-12-16 08:29:25 PST
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 6 Darin Adler 2009-12-16 09:41:17 PST
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.
Comment 7 Benjamin Otte 2009-12-16 10:43:27 PST
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.
Comment 8 WebKit Review Bot 2009-12-16 10:44:08 PST
style-queue ran check-webkit-style on attachment 44991 [details] without any errors.
Comment 9 WebKit Commit Bot 2009-12-16 11:00:34 PST
Comment on attachment 44991 [details]
patch v3

Clearing flags on attachment: 44991

Committed r52206: <http://trac.webkit.org/changeset/52206>
Comment 10 WebKit Commit Bot 2009-12-16 11:00:42 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Eric Seidel (no email) 2009-12-16 16:08:53 PST
Looks like this caused a bunch of pixel test failures.  I recommend we roll this out.
Comment 12 Eric Seidel (no email) 2009-12-16 16:12:42 PST
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.
Comment 13 Benjamin Otte 2009-12-16 16:20:42 PST
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).
Comment 14 WebKit Commit Bot 2009-12-16 16:41:15 PST
Committed r52229: <http://trac.webkit.org/changeset/52229>
Comment 15 Eric Seidel (no email) 2009-12-16 16:42:08 PST
Rolled out, see above.
Comment 16 Martin Robinson 2017-04-14 04:07:46 PDT
Going to close this one for the moment, since it hasn't seen activity for 8 years or so.