Bug 86893

Created attachment 142780 [details] Patch

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Attachment 142780 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1 Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 14 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 142780 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142780&action=review > Source/WebCore/platform/chromium/GPUSupportChromium.cpp:40 > +bool GPUSupport::canAccelerate2dCanvas() > +{ > + return WebKit::Platform::current()->canAccelerate2dCanvas(); is there any particular reason to have this instead of just calling WebKit::Platform::current()->canAccelerate2dCanvas() directly from HTMLCanvasElement.cpp ?

Created attachment 142781 [details] Patch

Comment on attachment 142781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142781&action=review > Source/WebCore/platform/GPUSupport.h:39 > +class GPUSupport { > +public: > + static bool canAccelerate2dCanvas(); > +}; This feels much too weird to me to put in a cross-platform place. The only caller is guarded by PLATFORM(CHROMIUM) so we should keep the implementation in chromium-specific locations.

(In reply to comment #6) > (From update of attachment 142781 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142781&action=review > > > Source/WebCore/platform/GPUSupport.h:39 > > +class GPUSupport { > > +public: > > + static bool canAccelerate2dCanvas(); > > +}; > > This feels much too weird to me to put in a cross-platform place. The only caller is guarded by PLATFORM(CHROMIUM) so we should keep the implementation in chromium-specific locations. We had the same situation in bug 85412 (even the same guard) and we decided to do this same thing (static function in a platform class, redirect to WebKit::Platform in a Chromium-specific class). Not sure why this is different.

(In reply to comment #4) > (From update of attachment 142780 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142780&action=review > > > Source/WebCore/platform/chromium/GPUSupportChromium.cpp:40 > > +bool GPUSupport::canAccelerate2dCanvas() > > +{ > > + return WebKit::Platform::current()->canAccelerate2dCanvas(); > > is there any particular reason to have this instead of just calling WebKit::Platform::current()->canAccelerate2dCanvas() directly from HTMLCanvasElement.cpp ? Because we're trying to separate out PlatformSupport from WebCore so we can eventually split the DLLs/libraries.

It's probably useful to get some background on what this bit is and why it's structured this way currently. Normally in WebCore the embedder decides ahead of time what the settings will be and passes those down (often to WebCore::Settings). Accelerated 2d canvas in chromium is a bit different since even if the setting says accelerate, we have to check some capabilities before starting rendering. Checking these capabilities requires spinning up the GPU process and some additional initialization that's expensive so we want to do it at late as possible. I don't know of any other port with this particular requirement. If you do decide that you need a thunk then put it somewhere chromium-specific. I don't really think it adds much value, though, it should be OK to call a virtual from WebCore that's exported by Platform.dll

This looks like another instance of wanting to refer to the Platform API from outside WebCore/platform. It's sounding more and more like we should allow that. We just need to be careful not to introduce Chromium-dependencies to too much of WebCore. @Mark: You might need to adjust the include paths in the GYP files a bit to get this to compile without the GPUSupport.h header. I'm happy to help with that part if you'd like.

Since WebCore depends on Platform, I think it is fine for WebCore to talk directly to Platform-defined interfaces. My one concern is that when we are in platform-independent code, we really should try to minimize the introduction of "#if PLATFORM(FOO)" code. In this case, we already have an #if PLATFORM(CHROMIUM), and eliminating that would just add extra code to other ports for no real value. I think we should just always challenge ourselves when introducing an #if PLATFORM(CHROMIUM) to common code to see if there isn't a reasonable way to avoid it. Once inside PLATFORM(CHROMIUM) code, we shouldn't hesitate to use interfaces from Platform/chromium/public/ directly as that reduces layering, which helps reduce the cost of maintaining the code.

Created attachment 143409 [details] Patch

HTMLCanvasElement.cpp now calls WebKit::Platform::current()->canAccelerate2dCanvas().

GYP changes to add Platform.gyp to webcore_prerequisites was already added in another bug, so not part of this patch.

Comment on attachment 143409 [details] Patch This seems fine to me.

Comment on attachment 143409 [details] Patch Clearing flags on attachment: 143409 Committed r118110: <http://trac.webkit.org/changeset/118110>

All reviewed patches have been landed. Closing bug.