Bug 112828 – [skia] feConvolveMatrix should use accelerated path

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Stephen White]

[skia] feConvolveMatrix should use accelerated path

[Stephen White]

Created attachment 194099 [details]
Patch

[Stephen White]

Created attachment 194100 [details]
Fix copyright date

[Stephen White]

Created attachment 194105 [details]
Patch

[Stephen White]

James, would you mind taking a look?

[James Robinson]

Comment on attachment 194105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194105&action=review

Hm, ok. I don't think I understand this very well but seems reasonable.

> Source/WebCore/platform/graphics/filters/skia/FEConvolveMatrixSkia.cpp:43
> +    case WebCore::EDGEMODE_UNKNOWN:
> +    case WebCore::EDGEMODE_DUPLICATE:
> +    default:
> +        return SkMatrixConvolutionImageFilter::kClamp_TileMode;

this looks weird. why not do

switch () {
case ..._WRAP:
   return ...;
case ..._NONE:
   return ...;
default:
   return ....;
}

?

[Stephen White]

Comment on attachment 194105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194105&action=review

>> Source/WebCore/platform/graphics/filters/skia/FEConvolveMatrixSkia.cpp:43
>> +        return SkMatrixConvolutionImageFilter::kClamp_TileMode;
> 
> this looks weird. why not do
> 
> switch () {
> case ..._WRAP:
>    return ...;
> case ..._NONE:
>    return ...;
> default:
>    return ....;
> }
> 
> ?

I'd prefer to call out the DUPLICATE edge mode explicitly, since this one is the one that really maps to Clamp.  I'll remove the default case entirely, since it shouldn't be necessary. UNKNOWN is a weird one, that's not in the spec (perhaps just there as a placeholder for parsing?) but unfortunately I still seed to do something with it.

[Stephen White]

Created attachment 194228 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 194228 [details]
Patch for landing

Clearing flags on attachment: 194228

Committed r146454: <http://trac.webkit.org/changeset/146454>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 112906

[Stephen White]

Created attachment 194245 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 194245 [details]
Patch for landing

Rejecting attachment 194245 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-03', 'validate-changelog', '--non-interactive', 194245, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-commit-queue.appspot.com/results/17292022

[Stephen White]

Created attachment 194246 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 194246 [details]
Patch for landing

Clearing flags on attachment: 194246

Committed r146469: <http://trac.webkit.org/changeset/146469>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Why was this patch landed without any new expected results? Please rebaseline results for at least one platform in the patch. Otherwise we have no way of knowing what the expected results is.

[Ryosuke Niwa]

http://trac.webkit.org/changeset/146492 didn't refer to either this bug or http://trac.webkit.org/changeset/146469.