Bug 86893 – [Chromium] Call canAccelerate2dCanvas directly

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Mark Pilgrim (Google)]

[Chromium] Call canAccelerate2dCanvas directly

[Mark Pilgrim (Google)]

Created attachment 142780 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

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.

[James Robinson]

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 ?

[Mark Pilgrim (Google)]

Created attachment 142781 [details]
Patch

[James Robinson]

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.

[Mark Pilgrim (Google)]

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

[Mark Pilgrim (Google)]

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

[James Robinson]

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

[Adam Barth]

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.

[Darin Fisher (:fishd, Google)]

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.

[Mark Pilgrim (Google)]

Created attachment 143409 [details]
Patch

[Mark Pilgrim (Google)]

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

[Mark Pilgrim (Google)]

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

[James Robinson]

Comment on attachment 143409 [details]
Patch

This seems fine to me.

[WebKit Review Bot]

Comment on attachment 143409 [details]
Patch

Clearing flags on attachment: 143409

Committed r118110: <http://trac.webkit.org/changeset/118110>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.