Bug 86893 - [Chromium] Call canAccelerate2dCanvas directly
Summary: [Chromium] Call canAccelerate2dCanvas directly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Pilgrim (Google)
URL:
Keywords:
Depends on:
Blocks: 82948
  Show dependency treegraph
 
Reported: 2012-05-18 13:37 PDT by Mark Pilgrim (Google)
Modified: 2012-05-22 20:22 PDT (History)
8 users (show)

See Also:


Attachments
Patch (22.14 KB, patch)
2012-05-18 13:38 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (22.11 KB, patch)
2012-05-18 13:52 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff
Patch (5.88 KB, patch)
2012-05-22 17:20 PDT, Mark Pilgrim (Google)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Pilgrim (Google) 2012-05-18 13:37:32 PDT
[Chromium] Call canAccelerate2dCanvas directly
Comment 1 Mark Pilgrim (Google) 2012-05-18 13:38:27 PDT
Created attachment 142780 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-18 13:41:39 PDT
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.
Comment 3 WebKit Review Bot 2012-05-18 13:42:03 PDT
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 4 James Robinson 2012-05-18 13:45:30 PDT
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 ?
Comment 5 Mark Pilgrim (Google) 2012-05-18 13:52:07 PDT
Created attachment 142781 [details]
Patch
Comment 6 James Robinson 2012-05-18 14:00:40 PDT
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.
Comment 7 Mark Pilgrim (Google) 2012-05-18 14:23:02 PDT
(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.
Comment 8 Mark Pilgrim (Google) 2012-05-18 14:24:01 PDT
(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.
Comment 9 James Robinson 2012-05-18 14:28:27 PDT
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
Comment 10 Adam Barth 2012-05-20 21:20:43 PDT
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.
Comment 11 Darin Fisher (:fishd, Google) 2012-05-21 11:45:15 PDT
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.
Comment 12 Mark Pilgrim (Google) 2012-05-22 17:20:11 PDT
Created attachment 143409 [details]
Patch
Comment 13 Mark Pilgrim (Google) 2012-05-22 17:20:57 PDT
HTMLCanvasElement.cpp now calls WebKit::Platform::current()->canAccelerate2dCanvas().
Comment 14 Mark Pilgrim (Google) 2012-05-22 17:21:33 PDT
GYP changes to add Platform.gyp to webcore_prerequisites was already added in another bug, so not part of this patch.
Comment 15 James Robinson 2012-05-22 17:28:09 PDT
Comment on attachment 143409 [details]
Patch

This seems fine to me.
Comment 16 WebKit Review Bot 2012-05-22 20:22:40 PDT
Comment on attachment 143409 [details]
Patch

Clearing flags on attachment: 143409

Committed r118110: <http://trac.webkit.org/changeset/118110>
Comment 17 WebKit Review Bot 2012-05-22 20:22:56 PDT
All reviewed patches have been landed.  Closing bug.