| Summary: | Ready Chromium port for Skia on Mac | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Cary Clark <caryclark> | ||||
| Component: | New Bugs | Assignee: | Cary Clark <caryclark> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | commit-queue, eric, fishd | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Cary Clark
2011-05-31 12:56:54 PDT
Created attachment 95466 [details]
Patch
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 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 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 on attachment 95466 [details] Patch Clearing flags on attachment: 95466 Committed r87821: <http://trac.webkit.org/changeset/87821> All reviewed patches have been landed. Closing bug. |