Use Skia if Skia on Mac Chrome is enabled

• Component: New Bugs
• Status: RESOLVED FIXED

[Cary Clark]

Use Skia if Skia on Mac Chrome is enabled

[Cary Clark]

Created attachment 97970 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 97970 [details]
Patch

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

I'm having trouble figuring out what your design is here.  I also don't understand
why we need another "use skia" macro when we already have WEBKIT_USING_SKIA.  How
is that existing macro not sufficient??

> Source/JavaScriptCore/wtf/Platform.h:549
> +#if USE(SKIA_ON_MAC_CHROME)

nit: in WebKit, we don't say "Chrome", we say "Chromium"

> Source/WebKit/chromium/public/WebCommon.h:42
> +    #if !defined(__APPLE__) || defined(USE_SKIA)

it looks like you are trying to have the embedder define a global USE_SKIA variable.
but in Platform.h, the code looks for a WTF_USE_SKIA_ON_MAC_CHROME define.  it isn't
clear how that gets set, or why we need both USE_SKIA and WTF_USE_SKIA_ON_MAC_CHROME.

[Cary Clark]

Comment on attachment 97970 [details]
Patch

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

>> Source/JavaScriptCore/wtf/Platform.h:549
>> +#if USE(SKIA_ON_MAC_CHROME)
> 
> nit: in WebKit, we don't say "Chrome", we say "Chromium"

I can wag my finger at Eric Seidel, who came up with the SKIA_ON_MAC_CHROME moniker. I've checked in 42 references -- I'll rename them all to SKIA_ON_MAC_CHROMIUM if that sounds good to you.

>> Source/WebKit/chromium/public/WebCommon.h:42
>> +    #if !defined(__APPLE__) || defined(USE_SKIA)
> 
> it looks like you are trying to have the embedder define a global USE_SKIA variable.
> but in Platform.h, the code looks for a WTF_USE_SKIA_ON_MAC_CHROME define.  it isn't
> clear how that gets set, or why we need both USE_SKIA and WTF_USE_SKIA_ON_MAC_CHROME.

When Chromium is built, many files include WebCommon.h without first including config.h. One example is plugins/ppapi/ppb_video_decoder_impl.cc. For these clients, WTF_USE_SKIA_ON_MAC_CHROME isn't an option. On the other hand, WTF_USE_SKIA_ON_MAC_CHROME is necessary for platform/graphics/IntRect.h.

USE_SKIA is defined by common/gypi. WTF_USE_SKIA_ON_MAC_CHROME is defined by chromium/features.gypi. When (if) Skia becomes the default rendering engine for OS X, much of the confusion will go away -- Chrome (non-WebKit) code will no longer need to special-case on Skia.

In the meantime, what do you suggest?

[Cary Clark]

I probably misunderstood -- if USE(xxx) isn't available for some WebCommon.h clients, that doesn't mean that the WTF_USE_xxx define isn't present. I'll try that instead.

[Cary Clark]

I tested and WTF_USE_SKIA_ON_MAC_CHROME isn't visible to WebCommon.h when included by files in src/glue, src/webkit/plugins, etc.

[Darin Fisher (:fishd, Google)]

Yes, WebCommon.h does not see internal WebKit defines from config.h.  We intentionally want to hide config.h from the embedder.

OK, so can you tell me the plan here?  You are saying that within WebCore, you have the macro USE(SKIA_ON_MAC_CHROME), which is controlled by the global USE_SKIA define?  And, the USE_SKIA define also controls WEBKIT_USING_SKIA?

I guess my confusion stems from not seeing where WTF_USE_SKIA_ON_MAC_CHROME gets defined.  I also don't understand why we need both WTF_USE_SKIA_ON_MAC_CHROME and WTF_USE_SKIA.  Why wouldn't WTF_USE_SKIA be sufficient?

[Cary Clark]

USE(SKIA_ON_MAC_CHROME) is a convenience that is equivalent to (USE(SKIA) && PLATFORM(CHROMIUM) && OS(DARWIN)). In those places where it is used, it enables code only appropriate to the Mac that would break the Linux and Windows builds.

The plan:
- Once this is checked in, along with one final CL to update WebCore.gyp, the normal Mac, Windows, and Linux builds should compile unchanged. 
- If you rebuild with:
  export gyp_defines='use_skia=1' ; gclient runhooks ; xcodebuild ...
then the Mac build will use Skia instead of CG.
- If you rebuild Linux or Windows with or without use_skia=1, it has no effect.
- When (if) Chrome decides to ship Mac using Skia, or if Chrome decides it will never use Skia on OS X, I will remove all scaffolding that allows choosing between Skia and CG.

Most of the code controlling gyp behavior and #defines is checked in:

build/common.gypi
        # Use Skia as WebKit renderer on Mac
        ['OS=="mac"', {
          'use_skia%': 0,
        }, {
          'use_skia%': 1,
        }],
...
      'use_skia%': '<(use_skia)',
...
      ['use_skia==1', {
        'defines': [
          'USE_SKIA=1',
        ],
      }],

wtf/Platform.h
   /* USE(SKIA) for Win/Linux, CG for Mac, unless enabled */
   #if PLATFORM(CHROMIUM)
   #if OS(DARWIN)
   #if USE(SKIA_ON_MAC_CHROME)
   #define WTF_USE_SKIA 1
   #else
   #define WTF_USE_CG 1
   #endif

Now I see that I haven't submitted the CL for chromium/features.gypi. Oops. It should look like:

--- features.gypi	(revision 89023)
+++ features.gypi	(working copy)
@@ -106,9 +106,11 @@
       'enable_svg%': 1,
       'enable_touch_events%': 1,
       'use_skia_gpu%': 0,
+      'use_skia%': 0,
       'enable_touch_icon_loading%' : 0,
     },
     'use_accelerated_compositing%': '<(use_accelerated_compositing)',
+    'use_skia%': '<(use_skia)',
     'use_threaded_compositing%': '<(use_threaded_compositing)',
     'enable_svg%': '<(enable_svg)',
     'enable_touch_events%': '<(enable_touch_events)',
@@ -119,7 +121,7 @@
           'ENABLE_3D_RENDERING=1',
         ],
       }],
-      ['use_accelerated_compositing==1 and OS!="mac"', {
+      ['use_accelerated_compositing==1 and (OS!="mac" or use_skia==1)', {
         'feature_defines': [
           'ENABLE_ACCELERATED_2D_CANVAS=1',
         ],
@@ -145,12 +147,18 @@
         'feature_defines': [
           'WTF_USE_WEBAUDIO_FFMPEG=1',
         ],
+        'use_skia%': 1,
       }],
       ['enable_register_protocol_handler==1', {
         'feature_defines': [
           'ENABLE_REGISTER_PROTOCOL_HANDLER=1',
         ],
       }],
+      ['OS=="mac"', {
+        'feature_defines': [
+          'WTF_USE_SKIA_ON_MAC_CHROME=<(use_skia)',
+        ],
+      }],
     ],
   },
 }

[Cary Clark]

Created attachment 98175 [details]
Patch

[Cary Clark]

I added all remaining patches here so that you can see the full extent of the changes. I am happy to split this up into multiple changes if that makes it more manageable.

[Darin Fisher (:fishd, Google)]

I see.  So, the reason for USE(SKIA_ON_MAC_CHROMIUM) is because we are still using CG in some parts of the Mac build.  I was about to ask why can't you just use USE(SKIA) instead, but apparently you need to distinguish using skia on windows/linux from using skia on mac.

What are some of the components that still need to use CG?  Is it a long term condition that we will always need to use CG even when we are primarily using Skia?

[Eric Seidel (no email)]

Comment on attachment 98175 [details]
Patch

Seems reasonable to me.  I can't tell if Darin's comments are concerns.

[Darin Fisher (:fishd, Google)]

Comment on attachment 98175 [details]
Patch

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

I think it would be helpful to have USE_SKIA defined uniformly across all platforms that are using skia.  I think that'll help us avoid accidentally introducing confusing code that assumes that USE_SKIA can only be true on the mac.

> Source/WebCore/platform/chromium/DragImageRef.h:32
> +#if OS(DARWIN) && !USE(SKIA_ON_MAC_CHROMIUM)

maybe this should be #if USE(CG)?

> Source/WebCore/platform/chromium/DragImageRef.h:40
> +#if OS(DARWIN) && !USE(SKIA_ON_MAC_CHROMIUM)

#if USE(CG)

> Source/WebKit/chromium/WebKit.gyp:693
> +                      'sources/': [

bad indentation

[Cary Clark]

Comment on attachment 98175 [details]
Patch

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

>> Source/WebCore/platform/chromium/DragImageRef.h:32
>> +#if OS(DARWIN) && !USE(SKIA_ON_MAC_CHROMIUM)
> 
> maybe this should be #if USE(CG)?

Done.

>> Source/WebCore/platform/chromium/DragImageRef.h:40
>> +#if OS(DARWIN) && !USE(SKIA_ON_MAC_CHROMIUM)
> 
> #if USE(CG)

Done.

>> Source/WebKit/chromium/WebKit.gyp:693
>> +                      'sources/': [
> 
> bad indentation

Done.

[Cary Clark]

Created attachment 98207 [details]
Patch

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 98207 [details]:

http/tests/local/formdata/send-form-data.html bug 63034 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 98207 [details]
Patch

Clearing flags on attachment: 98207

Committed r89489: <http://trac.webkit.org/changeset/89489>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This doesn't compile on chromium mac.  Rollout in progress...

[Cary Clark]

Created attachment 98744 [details]
Patch

[Cary Clark]

The uploaded patch is nearly identical to the previous approved patch. The only difference are merges in IntSize.h and FloatSize.h.

The last time, the cr-mac build failed in the bot here:

 http://build.webkit.org/builders/Chromium%20Mac%20Release/builds/25018/steps/gclient/logs/stdio

with this error:

 NameError: name 'use_skia' is not defined while evaluating condition 'use_skia==0' in WebKit.gyp while trying to load WebKit.gyp

I cannot reproduce this error either on a Chromium build, or by doing:

  update-webkit --chromium ; build-webkit --chromium

Since I don't have permission for the mac and cr-mac bot builds, I need help diagnosing why this cannot land in its current state. I've re-uploaded so I can get that help.

[Cary Clark]

Additionally, I tried

  perl ./Tools/Scripts/update-webkit-chromium --force
  cd Source/WebKit/chromium/
  python gyp_webkit

which looks like the command sequence the bot tries that fails, and that worked for me too.

[James Robinson]

Comment on attachment 98744 [details]
Patch

cq- until somebody figures out why this didn't compile last time and what will make it compile this time.

[Cary Clark]

I've sent two copies of the letter in to Apple, but in the meantime, is there anyway to override my committer status and get the cr-mac build to run on this patch?

[Cary Clark]

Created attachment 98767 [details]
Patch

[Cary Clark]

Thanks to Tony for finding my gyp bug. (adding 

  'use_skia%': '<(use_skia)',

to line 119 of chromium/features.gypi )

[James Robinson]

(In reply to comment #25)
> Thanks to Tony for finding my gyp bug. (adding 
> 
>   'use_skia%': '<(use_skia)',
> 
> to line 119 of chromium/features.gypi )

Do you know why you couldn't reproduce the failure locally?

[James Robinson]

Reopening since the patch was reverted

[Cary Clark]

It worked locally because I had added the same variable setup to build/common.gypi, which is outside of WebKit.

[WebKit Review Bot]

Comment on attachment 98767 [details]
Patch

Clearing flags on attachment: 98767

Committed r89968: <http://trac.webkit.org/changeset/89968>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

This caused compile failures on multiple windows bots, example:
http://build.webkit.org/builders/Chromium%20Win%20Release/builds/27392/steps/compile-webkit/logs/stdio

the failure:
17>VDMXParser.cpp
17>..\platform\graphics\chromium\VDMXParser.cpp(36) : fatal error C1083: Cannot open include file: 'arpa/inet.h': No such file or directory

I have no idea what that error means or how it's correlated with this patch, but the same failure showed up on multiple bots when this patch landed so I'm assuming it's correlated.

[Cary Clark]

Created attachment 99310 [details]
Patch

[Cary Clark]

The only change from the previous patch is to correctly conditionalize the WebCore.gyp file so that VMDX is excluded from the build unless the toolkit uses GTK, or the build targets a Skia-enabled Mac build. This fixes the Windows build.

[James Robinson]

Comment on attachment 99310 [details]
Patch

If at first you don't succeed...

[WebKit Review Bot]

Comment on attachment 99310 [details]
Patch

Clearing flags on attachment: 99310

Committed r90167: <http://trac.webkit.org/changeset/90167>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 99310 [details]:

http/tests/local/blob/send-hybrid-blob.html bug 63761 (author: [e-mail])
The commit-queue is continuing to process your patch.