Bug 86893

Summary: [Chromium] Call canAccelerate2dCanvas directly
Product: WebKit Reporter: Mark Pilgrim (Google) <pilgrim>
Component: WebKit Misc.Assignee: Mark Pilgrim (Google) <pilgrim>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, fishd, haraken, jamesr, rakuco, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 82948    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Mark Pilgrim (Google)
Reported 2012-05-18 13:37:32 PDT
[Chromium] Call canAccelerate2dCanvas directly
Attachments
Patch (22.14 KB, patch)
2012-05-18 13:38 PDT, Mark Pilgrim (Google)
no flags
Patch (22.11 KB, patch)
2012-05-18 13:52 PDT, Mark Pilgrim (Google)
no flags
Patch (5.88 KB, patch)
2012-05-22 17:20 PDT, Mark Pilgrim (Google)
no flags
Mark Pilgrim (Google)
Comment 1 2012-05-18 13:38:27 PDT
WebKit Review Bot
Comment 2 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.
WebKit Review Bot
Comment 3 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.
James Robinson
Comment 4 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 ?
Mark Pilgrim (Google)
Comment 5 2012-05-18 13:52:07 PDT
James Robinson
Comment 6 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.
Mark Pilgrim (Google)
Comment 7 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.
Mark Pilgrim (Google)
Comment 8 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.
James Robinson
Comment 9 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
Adam Barth
Comment 10 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.
Darin Fisher (:fishd, Google)
Comment 11 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.
Mark Pilgrim (Google)
Comment 12 2012-05-22 17:20:11 PDT
Mark Pilgrim (Google)
Comment 13 2012-05-22 17:20:57 PDT
HTMLCanvasElement.cpp now calls WebKit::Platform::current()->canAccelerate2dCanvas().
Mark Pilgrim (Google)
Comment 14 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.
James Robinson
Comment 15 2012-05-22 17:28:09 PDT
Comment on attachment 143409 [details] Patch This seems fine to me.
WebKit Review Bot
Comment 16 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>
WebKit Review Bot
Comment 17 2012-05-22 20:22:56 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.