Bug 61548

Summary: Add 'Skia for rendering, CG for UI' option to Chromium porting layer
Product: WebKit Reporter: Cary Clark <caryclark>
Component: New BugsAssignee: Cary Clark <caryclark>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, fishd, mark, thakis
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Cary Clark 2011-05-26 11:44:43 PDT
Add 'Skia for rendering, CG for UI' option to Chromium porting layer
Comment 1 Cary Clark 2011-05-26 11:57:16 PDT
Created attachment 95014 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-05-26 12:04:36 PDT
Comment on attachment 95014 [details]
Patch

I'm confused.  Can we draw the UI with anything other than CG?  If not, seems we wouldn't need this.
Comment 3 Cary Clark 2011-05-26 12:14:52 PDT
As checked in, if WebKit uses Skia, the UI doesn't use CG, and vice-versa.

Windows and Linux ports use Skia, but don't use CG for the UI.

In the future, the Mac port uses Skia for WebKit, and uses CG for the UI.

In the distant future, the Mac port uses hardware-accelerated Skia, and may not use CG for the UI.

Some of the current scaffolding will go away once all of Chrome uses Skia for WebKit. Allowing the scaffolding for now allows checking in Skia support incrementally instead of all at once.
Comment 4 Darin Fisher (:fishd, Google) 2011-05-26 14:45:40 PDT
Comment on attachment 95014 [details]
Patch

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

> Source/WebKit/chromium/public/WebCommon.h:44
> +        #define UI_USING_CG 0

this seems like the wrong place for this define.  this is the webkit layer,
which either renders using SKIA or CG.  that's what WEBKIT_USING_SKIA conveys.

it sounds like maybe you want to add a #define in the chromium tree somewhere.

> Source/WebKit/chromium/src/DragClientImpl.cpp:91
> +#if WEBKIT_USING_SKIA && !UI_USING_CG

this seems problematic.  it seems like we should modify the chromium
code which implements WebViewClient::startDragging to understand that
it is getting a SkBitmap instead of a CGImage here.

it would be much better to patch some conversion code into chromium
than to confuse webkit with the notion of supporting two graphics
libraries in different contexts.
Comment 5 Cary Clark 2011-05-27 05:13:07 PDT
Comment on attachment 95014 [details]
Patch

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

>> Source/WebKit/chromium/public/WebCommon.h:44
>> +        #define UI_USING_CG 0
> 
> this seems like the wrong place for this define.  this is the webkit layer,
> which either renders using SKIA or CG.  that's what WEBKIT_USING_SKIA conveys.
> 
> it sounds like maybe you want to add a #define in the chromium tree somewhere.

Could you please be more specific and suggest where 'somewhere' is?

>> Source/WebKit/chromium/src/DragClientImpl.cpp:91
>> +#if WEBKIT_USING_SKIA && !UI_USING_CG
> 
> this seems problematic.  it seems like we should modify the chromium
> code which implements WebViewClient::startDragging to understand that
> it is getting a SkBitmap instead of a CGImage here.
> 
> it would be much better to patch some conversion code into chromium
> than to confuse webkit with the notion of supporting two graphics
> libraries in different contexts.

I thought that this code was chromium-specific already. Since there are no references to 'startDrag' or 'DragImageRef' outside of Source/WebCore and Source/WebKit, I'm at a loss as to what I should do.

Who can I work with to restructure this as you suggest?
Comment 6 Cary Clark 2011-05-31 07:49:03 PDT
Created attachment 95433 [details]
Patch
Comment 7 Eric Seidel (no email) 2011-05-31 07:59:19 PDT
Comment on attachment 95433 [details]
Patch

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

> Source/WebKit/chromium/public/WebCommon.h:50
> +    #if defined(__APPLE__) && !WEBKIT_USING_SKIA

What if WEBKIT_USING_SKIA is not defined?  I assume ! will do the right thing?
Comment 8 Cary Clark 2011-05-31 08:39:39 PDT
Comment on attachment 95433 [details]
Patch

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

>> Source/WebKit/chromium/public/WebCommon.h:50
>> +    #if defined(__APPLE__) && !WEBKIT_USING_SKIA
> 
> What if WEBKIT_USING_SKIA is not defined?  I assume ! will do the right thing?

WEBKIT_USING_SKIA is always defined either on line 43 or line 45, just above these lines.
Comment 9 Eric Seidel (no email) 2011-05-31 08:43:44 PDT
Comment on attachment 95433 [details]
Patch

OK.
Comment 10 WebKit Commit Bot 2011-05-31 09:05:45 PDT
Comment on attachment 95433 [details]
Patch

Clearing flags on attachment: 95433

Committed r87728: <http://trac.webkit.org/changeset/87728>
Comment 11 WebKit Commit Bot 2011-05-31 09:05:52 PDT
All reviewed patches have been landed.  Closing bug.