Bug 62999

Created attachment 97970 [details] Patch

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.

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?

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.

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.

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?

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)', + ], + }], ], }, }

Created attachment 98175 [details] Patch

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.

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?

Comment on attachment 98175 [details] Patch Seems reasonable to me. I can't tell if Darin's comments are concerns.

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

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.

Created attachment 98207 [details] Patch

The commit-queue encountered the following flaky tests while processing attachment 98207 [details]: http/tests/local/formdata/send-form-data.html bug 63034 (author: jianli@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 98207 [details] Patch Clearing flags on attachment: 98207 Committed r89489: <http://trac.webkit.org/changeset/89489>

All reviewed patches have been landed. Closing bug.

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

Created attachment 98744 [details] Patch

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.

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.

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.

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?

Created attachment 98767 [details] Patch

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

(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?

Reopening since the patch was reverted

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

Comment on attachment 98767 [details] Patch Clearing flags on attachment: 98767 Committed r89968: <http://trac.webkit.org/changeset/89968>

All reviewed patches have been landed. Closing bug.

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.

Created attachment 99310 [details] Patch

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.

Comment on attachment 99310 [details] Patch If at first you don't succeed...

Comment on attachment 99310 [details] Patch Clearing flags on attachment: 99310 Committed r90167: <http://trac.webkit.org/changeset/90167>

All reviewed patches have been landed. Closing bug.

The commit-queue encountered the following flaky tests while processing attachment 99310 [details]: http/tests/local/blob/send-hybrid-blob.html bug 63761 (author: kinuko@chromium.org) The commit-queue is continuing to process your patch.