Bug 61800 - Ready Chromium port for Skia on Mac
Summary: Ready Chromium port for Skia on Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Cary Clark
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-31 12:56 PDT by Cary Clark
Modified: 2011-06-01 10:20 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.72 KB, patch)
2011-05-31 13:04 PDT, Cary Clark
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cary Clark 2011-05-31 12:56:54 PDT
Ready Chromium port for Skia on Mac
Comment 1 Cary Clark 2011-05-31 13:04:19 PDT
Created attachment 95466 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-05-31 19:35:11 PDT
Comment on attachment 95466 [details]
Patch

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

Seems OK, but could be cleaner.

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:412
> +    SkCanvas* canvas = context->platformContext()->canvas();

Seems like we want a direct accessor here somehow.  Or maybe a skia(context) helper function.

> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:438
> +    gfx::SkiaBitLocker bitLocker(drawingContext->platformContext()->canvas());

We might want a WebCore helper which knows how to bitlock directly from a GraphicsContext instead of having to do this canvas lookup every time.

We could also use an abstraction around the drawingContext which would lock in teh skia case, and otherwise just give you back the CGContext.  That would remove a bunch of these #if USE(SKIA) branches.
Comment 3 Cary Clark 2011-06-01 05:40:16 PDT
Comment on attachment 95466 [details]
Patch

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

>> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:438
>> +    gfx::SkiaBitLocker bitLocker(drawingContext->platformContext()->canvas());
> 
> We might want a WebCore helper which knows how to bitlock directly from a GraphicsContext instead of having to do this canvas lookup every time.
> 
> We could also use an abstraction around the drawingContext which would lock in teh skia case, and otherwise just give you back the CGContext.  That would remove a bunch of these #if USE(SKIA) branches.

The bitlock/canvas lookup combination only happens four times in the sources. Also, the #if USE(SKIA) scaffolding is temporary; once the Chrome Mac port uses Skia, it will be removed.
Comment 4 Eric Seidel (no email) 2011-06-01 08:57:11 PDT
Comment on attachment 95466 [details]
Patch

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

>>> Source/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm:438
>>> +    gfx::SkiaBitLocker bitLocker(drawingContext->platformContext()->canvas());
>> 
>> We might want a WebCore helper which knows how to bitlock directly from a GraphicsContext instead of having to do this canvas lookup every time.
>> 
>> We could also use an abstraction around the drawingContext which would lock in teh skia case, and otherwise just give you back the CGContext.  That would remove a bunch of these #if USE(SKIA) branches.
> 
> The bitlock/canvas lookup combination only happens four times in the sources. Also, the #if USE(SKIA) scaffolding is temporary; once the Chrome Mac port uses Skia, it will be removed.

I don't think "temporary" is every a very good excuse for ugly code.  But I think this patch is OK.  Best of luck. :)
Comment 5 WebKit Commit Bot 2011-06-01 10:19:55 PDT
Comment on attachment 95466 [details]
Patch

Clearing flags on attachment: 95466

Committed r87821: <http://trac.webkit.org/changeset/87821>
Comment 6 WebKit Commit Bot 2011-06-01 10:20:00 PDT
All reviewed patches have been landed.  Closing bug.