Bug 61800 – Ready Chromium port for Skia on Mac

• Component: New Bugs
• Status: RESOLVED FIXED

[Cary Clark]

Ready Chromium port for Skia on Mac

[Cary Clark]

Created attachment 95466 [details]
Patch

[Eric Seidel (no email)]

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.

[Cary Clark]

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.

[Eric Seidel (no email)]

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

[WebKit Commit Bot]

Comment on attachment 95466 [details]
Patch

Clearing flags on attachment: 95466

Committed r87821: <http://trac.webkit.org/changeset/87821>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.