Bug 106366 - [Skia] Implement GraphicsContext::fillRoundedRect() using SkCanvas::drawRRect()
Summary: [Skia] Implement GraphicsContext::fillRoundedRect() using SkCanvas::drawRRect()
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Florin Malita
Depends on:
Reported: 2013-01-08 12:42 PST by Florin Malita
Modified: 2013-01-09 06:33 PST (History)
8 users (show)

See Also:

Patch (1.43 MB, patch)
2013-01-08 13:30 PST, Florin Malita
no flags Details | Formatted Diff | Diff
Patch (1.43 MB, patch)
2013-01-08 14:23 PST, Florin Malita
no flags Details | Formatted Diff | Diff
Patch (1.43 MB, patch)
2013-01-08 14:55 PST, Florin Malita
no flags Details | Formatted Diff | Diff
Patch for landing (1.43 MB, patch)
2013-01-09 05:49 PST, Florin Malita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Florin Malita 2013-01-08 12:42:43 PST
Skia supports rounded rect draw commands, so there's no need to degrade fillRoundedRect() to drawPath().
Comment 1 Florin Malita 2013-01-08 13:30:55 PST
Created attachment 181747 [details]
Comment 2 Stephen White 2013-01-08 14:03:18 PST
Comment on attachment 181747 [details]

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

> Source/WebCore/platform/graphics/skia/GraphicsContextSkia.cpp:833
> +    SkVector radii[4];
> +    radii[SkRRect::kUpperLeft_Corner] = SkPoint::Make(topLeft.width(), topLeft.height());
> +    radii[SkRRect::kUpperRight_Corner] = SkPoint::Make(topRight.width(), topRight.height());
> +    radii[SkRRect::kLowerRight_Corner] = SkPoint::Make(bottomRight.width(), bottomRight.height());
> +    radii[SkRRect::kLowerLeft_Corner] = SkPoint::Make(bottomLeft.width(), bottomLeft.height());

Looks a little strange to be assigning an SkPoint to an SkVector (even though they're the same thing).  Since it's a typedef anyway, could you use SkVector::Make() here instead?
Comment 3 Florin Malita 2013-01-08 14:23:30 PST
Created attachment 181761 [details]

Thanks Stephen, updated.
Comment 4 Mike Reed 2013-01-08 14:31:23 PST
How about

radii[SkRRect::kUpperLeft_Corner].set(topLeft.width(), topLeft.height());

No need to make and assign, when you can set.

PS -- if we cared about warnings, we might want to promote width/height to floats (or SkScalars), since they are ints here. Not a request, just a note.
Comment 5 Florin Malita 2013-01-08 14:55:51 PST
Created attachment 181774 [details]

Updated, thanks Mike.
Comment 6 Stephen White 2013-01-08 14:57:09 PST
Comment on attachment 181774 [details]

Looks good.  r=me
Comment 7 WebKit Review Bot 2013-01-08 18:03:14 PST
Comment on attachment 181774 [details]

Attachment 181774 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15771255

New failing tests:
Comment 8 Florin Malita 2013-01-09 05:49:04 PST
Created attachment 181902 [details]
Patch for landing
Comment 9 WebKit Review Bot 2013-01-09 06:33:48 PST
Comment on attachment 181902 [details]
Patch for landing

Clearing flags on attachment: 181902

Committed r139191: <http://trac.webkit.org/changeset/139191>
Comment 10 WebKit Review Bot 2013-01-09 06:33:52 PST
All reviewed patches have been landed.  Closing bug.