Bug 38220 - [chromium] Add API for querying whether a WebWidget is using GPU accelerated compositing
Summary: [chromium] Add API for querying whether a WebWidget is using GPU accelerated ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-27 14:40 PDT by Vangelis Kokkevis
Modified: 2010-05-09 13:49 PDT (History)
5 users (show)

See Also:


Attachments
Proposed patch (3.09 KB, patch)
2010-04-27 15:01 PDT, Vangelis Kokkevis
fishd: review-
Details | Formatted Diff | Diff
Proposed patch (5.60 KB, text/plain)
2010-04-30 12:40 PDT, Vangelis Kokkevis
no flags Details
Proposed patch addressing review comments (5.96 KB, patch)
2010-04-30 18:59 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Proposed patch addressing review comments (5.98 KB, patch)
2010-05-03 15:14 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Patch - fixed style issue (5.97 KB, patch)
2010-05-03 15:30 PDT, Vangelis Kokkevis
fishd: review-
Details | Formatted Diff | Diff
patch. Modified to address review comments (5.59 KB, patch)
2010-05-04 11:51 PDT, Vangelis Kokkevis
fishd: review-
Details | Formatted Diff | Diff
Path - fixing placement of isAcceleratedCompositingActive in the WebViewImpl files (5.83 KB, patch)
2010-05-04 15:29 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Patch (50.02 KB, patch)
2010-05-09 13:45 PDT, Dirk Schulze
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vangelis Kokkevis 2010-04-27 14:40:19 PDT
This functionality is necessary to allow chromium to disable rendering from the browser process when the gpu process is actually responsible for doing all the drawing for the page.
Comment 1 Vangelis Kokkevis 2010-04-27 15:01:39 PDT
Created attachment 54458 [details]
Proposed patch
Comment 2 Darin Fisher (:fishd, Google) 2010-04-29 20:41:29 PDT
Comment on attachment 54458 [details]
Proposed patch

WebKit/chromium/public/WebPopupMenu.h:44
 +      virtual bool isAcceleratedCompositing() const { return false; }
You should instead implement this method in WebPopupMenuImpl.{h,cpp} just as you have done in WebViewImpl.{h,cpp}.

WebKit/chromium/public/WebWidget.h:95
 +      virtual bool isAcceleratedCompositing() const = 0;
nit: isAcceleratedCompositingEnabled would be a better (more consistent with naming conventions) name for this.

WebKit/chromium/src/WebViewImpl.cpp:2041
 +      return m_isAcceleratedCompositing;
ditto.  this variable would be better named m_isAcceleratedCompositingEnabled

WebKit/chromium/src/WebViewImpl.h:363
 +      void setAcceleratedCompositing(bool);
This one should be named enableAcceleratedCompositing to be consistent with naming conventions.
Comment 3 Vangelis Kokkevis 2010-04-30 10:34:43 PDT
(In reply to comment #2)
> (From update of attachment 54458 [details])
> WebKit/chromium/public/WebPopupMenu.h:44
>  +      virtual bool isAcceleratedCompositing() const { return false; }
> You should instead implement this method in WebPopupMenuImpl.{h,cpp} just as
> you have done in WebViewImpl.{h,cpp}.
> 
> WebKit/chromium/public/WebWidget.h:95
>  +      virtual bool isAcceleratedCompositing() const = 0;
> nit: isAcceleratedCompositingEnabled would be a better (more consistent with
> naming conventions) name for this.
> 

I agree that name is kind of ugly although I used it for consistency with Apple's Windows implementation (WebKit/WebKit/win/WebView.h). The original mac implementation seems to be using:  "isUsingAcceleratedCompositing()" which is more accurate as it's not about so much about it being enabled but rather about being active for the View. So, any objections to switching ours to isUsingAcceleratedCompositing() too? 

> WebKit/chromium/src/WebViewImpl.cpp:2041
>  +      return m_isAcceleratedCompositing;
> ditto.  this variable would be better named m_isAcceleratedCompositingEnabled
> 
> WebKit/chromium/src/WebViewImpl.h:363
>  +      void setAcceleratedCompositing(bool);
> This one should be named enableAcceleratedCompositing to be consistent with
> naming conventions.
Comment 4 Vangelis Kokkevis 2010-04-30 12:40:49 PDT
Created attachment 54818 [details]
Proposed patch

Renamed isAcceleratedCompositing to isUsingAcceleratedCompositing and moved WebPopupMenu implementation of the method to the Impl class per Darin's request.
Comment 5 Darin Fisher (:fishd, Google) 2010-04-30 12:58:55 PDT
I suggested is*Enabled because that is how we have been expressing things like this in the webkit api today:

WebKit/chromium/public> grep 'is.*Enabled' *
WebAccessibilityObject.h:    bool isEnabled() const;
WebContextMenuData.h:    bool isSpellCheckingEnabled;
WebFormControlElement.h:    WEBKIT_API bool isEnabled() const;
WebFrame.h:    virtual bool isViewSourceModeEnabled() const = 0;
WebFrame.h:    virtual bool isCommandEnabled(const WebString&) const = 0;
WebFrame.h:    virtual bool isContinuousSpellCheckingEnabled() const = 0;
WebGraphicsContext3D.h:    virtual bool isEnabled(unsigned long cap) = 0;
WebInputElement.h:        WEBKIT_API bool isEnabledFormControl() const;
WebRuntimeFeatures.h:    WEBKIT_API static bool isDatabaseEnabled();
WebRuntimeFeatures.h:    WEBKIT_API static bool isLocalStorageEnabled();
WebRuntimeFeatures.h:    WEBKIT_API static bool isSessionStorageEnabled();
WebRuntimeFeatures.h:    WEBKIT_API static bool isMediaPlayerEnabled();
WebRuntimeFeatures.h:    WEBKIT_API static bool isSocketsEnabled();
WebRuntimeFeatures.h:    WEBKIT_API static bool isNotificationsEnabled();
WebRuntimeFeatures.h:    WEBKIT_API static bool isApplicationCacheEnabled();
WebRuntimeFeatures.h:    WEBKIT_API static bool isGeolocationEnabled();
WebRuntimeFeatures.h:    WEBKIT_API static bool isIndexedDatabaseEnabled();
WebRuntimeFeatures.h:    WEBKIT_API static bool isWebGLEnabled();
WebRuntimeFeatures.h:    WEBKIT_API static bool isPushStateEnabled(bool);
WebRuntimeFeatures.h:    WEBKIT_API static bool isTouchEnabled();
WebViewClient.h:    virtual bool isSmartInsertDeleteEnabled() { return true; }
WebViewClient.h:    virtual bool isSelectTrailingWhitespaceEnabled() { return true; }

I agree that isUsingAcceleratedCompositing is better than isAcceleratedCompositing, but since this is just bikeshed territory, I lean toward matching existing conventions.
Comment 6 Vangelis Kokkevis 2010-04-30 15:10:09 PDT
There's a semantic difference that I'm trying to capture here which I think it's important. What this API call is supposed to return is not whether the accelerated compositing feature is enabled by the user but whether it's actually active for that page. It seems that most (if not all) of the is*Enabled() calls currently reflect a mode entered into at the request of the user.

This is based on my little experience with existing code. I'll rely on judgement.



(In reply to comment #5)
> I suggested is*Enabled because that is how we have been expressing things like
> this in the webkit api today:
> 
> WebKit/chromium/public> grep 'is.*Enabled' *
> WebAccessibilityObject.h:    bool isEnabled() const;
> WebContextMenuData.h:    bool isSpellCheckingEnabled;
> WebFormControlElement.h:    WEBKIT_API bool isEnabled() const;
> WebFrame.h:    virtual bool isViewSourceModeEnabled() const = 0;
> WebFrame.h:    virtual bool isCommandEnabled(const WebString&) const = 0;
> WebFrame.h:    virtual bool isContinuousSpellCheckingEnabled() const = 0;
> WebGraphicsContext3D.h:    virtual bool isEnabled(unsigned long cap) = 0;
> WebInputElement.h:        WEBKIT_API bool isEnabledFormControl() const;
> WebRuntimeFeatures.h:    WEBKIT_API static bool isDatabaseEnabled();
> WebRuntimeFeatures.h:    WEBKIT_API static bool isLocalStorageEnabled();
> WebRuntimeFeatures.h:    WEBKIT_API static bool isSessionStorageEnabled();
> WebRuntimeFeatures.h:    WEBKIT_API static bool isMediaPlayerEnabled();
> WebRuntimeFeatures.h:    WEBKIT_API static bool isSocketsEnabled();
> WebRuntimeFeatures.h:    WEBKIT_API static bool isNotificationsEnabled();
> WebRuntimeFeatures.h:    WEBKIT_API static bool isApplicationCacheEnabled();
> WebRuntimeFeatures.h:    WEBKIT_API static bool isGeolocationEnabled();
> WebRuntimeFeatures.h:    WEBKIT_API static bool isIndexedDatabaseEnabled();
> WebRuntimeFeatures.h:    WEBKIT_API static bool isWebGLEnabled();
> WebRuntimeFeatures.h:    WEBKIT_API static bool isPushStateEnabled(bool);
> WebRuntimeFeatures.h:    WEBKIT_API static bool isTouchEnabled();
> WebViewClient.h:    virtual bool isSmartInsertDeleteEnabled() { return true; }
> WebViewClient.h:    virtual bool isSelectTrailingWhitespaceEnabled() { return
> true; }
> 
> I agree that isUsingAcceleratedCompositing is better than
> isAcceleratedCompositing, but since this is just bikeshed territory, I lean
> toward matching existing conventions.
Comment 7 Darin Fisher (:fishd, Google) 2010-04-30 16:01:58 PDT
(In reply to comment #6)
> There's a semantic difference that I'm trying to capture here which I think
> it's important. What this API call is supposed to return is not whether the
> accelerated compositing feature is enabled by the user but whether it's
> actually active for that page. It seems that most (if not all) of the
> is*Enabled() calls currently reflect a mode entered into at the request of the
> user.

That's an interesting observation.  It is true that many of the is*Enabled methods query user controlled settings.

I was just thinking in terms of whether or not accelerated compositing is enabled for the page.  In which case, asking the question "is accelerated compositing enabled?" seems natural to me.  I don't see "enable" as something necessarily connected to the user.  It seems like "use" could equally imply per request of the user.

Are there any examples in WebKit API or WebCore that match the semantic distinction you are going for?  It'd be nice to copy some convention.
Comment 8 Vangelis Kokkevis 2010-04-30 18:53:10 PDT
(In reply to comment #7)
> (In reply to comment #6)
> I was just thinking in terms of whether or not accelerated compositing is
> enabled for the page.  In which case, asking the question "is accelerated
> compositing enabled?" seems natural to me.  I don't see "enable" as something
> necessarily connected to the user.  It seems like "use" could equally imply per
> request of the user.

You are right. "use" has the same issues.

> 
> Are there any examples in WebKit API or WebCore that match the semantic
> distinction you are going for?  It'd be nice to copy some convention.

I couldn't find any but what I did find, is WebCore's Settings class defined  in WebCore/page/Settings.h, acceleratedCompositingEnabled() and setAcceleratedCompositingEnabled() are both defined to mean whether the accelerated compositing feature is allowed or not.  I think it would be confusing if we were to use the exact same name with a different meaning. 

A couple of other possible options:
* isAcceleratedCompositingActive()
* isDoingAcceleratedCompositing()
Comment 9 Vangelis Kokkevis 2010-04-30 18:59:45 PDT
Created attachment 54844 [details]
Proposed patch addressing review comments

Patch with isAcceleratedCompositing renamed to isAcceleratedCompositingEnabled per Darin's suggestion, in case we don't come up with a more suitable name.
Comment 10 Darin Fisher (:fishd, Google) 2010-04-30 23:30:42 PDT
> I couldn't find any but what I did find, is WebCore's Settings class defined 
> in WebCore/page/Settings.h, acceleratedCompositingEnabled() and
> setAcceleratedCompositingEnabled() are both defined to mean whether the
> accelerated compositing feature is allowed or not.  I think it would be
> confusing if we were to use the exact same name with a different meaning. 

^^^ good observation


> A couple of other possible options:
> * isAcceleratedCompositingActive()
> * isDoingAcceleratedCompositing()

+1 for isAcceleratedCompositingActive :-)
Comment 11 Darin Fisher (:fishd, Google) 2010-04-30 23:31:30 PDT
We can then use the "Active" and "DidActivate" on the Chromium side to refer to the same concept.
Comment 12 Vangelis Kokkevis 2010-05-03 15:14:48 PDT
Created attachment 54966 [details]
Proposed patch addressing review comments

Ok, the names have now be changed to isAcceleratedCompositingActive.  Please have another look.
Comment 13 WebKit Review Bot 2010-05-03 15:18:20 PDT
Attachment 54966 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/ChangeLog:5:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Vangelis Kokkevis 2010-05-03 15:30:51 PDT
Created attachment 54969 [details]
Patch - fixed style issue
Comment 15 Darin Fisher (:fishd, Google) 2010-05-04 11:05:51 PDT
Comment on attachment 54969 [details]
Patch - fixed style issue

WebKit/chromium/public/WebPopupMenu.h:44
> +      virtual bool isAcceleratedCompositingActive() const = 0;
You can delete this line as WebPopupMenu inherits from WebWidget.

WebKit/chromium/src/WebPopupMenuImpl.h:82
> +      virtual bool isAcceleratedCompositingActive() const { return false; }
please create a separate section labelled WebPopupMenu and move this
method there.

the method declarations are grouped by interfaces being implemented.

otherwise, LGTM!
Comment 16 Vangelis Kokkevis 2010-05-04 11:32:38 PDT
(In reply to comment #15)
> WebKit/chromium/src/WebPopupMenuImpl.h:82
> > +      virtual bool isAcceleratedCompositingActive() const { return false; }
> please create a separate section labelled WebPopupMenu and move this
> method there.
> 
> the method declarations are grouped by interfaces being implemented.
> 
Since this is a method inherited from WebWidget, shouldn't it go in the WebWidget section? Thanks.
Comment 17 Vangelis Kokkevis 2010-05-04 11:51:45 PDT
Created attachment 55033 [details]
patch. Modified to address review comments
Comment 18 Vangelis Kokkevis 2010-05-04 11:53:09 PDT
(In reply to comment #15)
> (From update of attachment 54969 [details])
> WebKit/chromium/public/WebPopupMenu.h:44
> > +      virtual bool isAcceleratedCompositingActive() const = 0;
> You can delete this line as WebPopupMenu inherits from WebWidget.

Done.

> 
> WebKit/chromium/src/WebPopupMenuImpl.h:82
> > +      virtual bool isAcceleratedCompositingActive() const { return false; }
> please create a separate section labelled WebPopupMenu and move this
> method there.
> 
> the method declarations are grouped by interfaces being implemented.

Moved with the other WebWidget methods.

> 
> otherwise, LGTM!

Thanks!
Comment 19 Darin Fisher (:fishd, Google) 2010-05-04 14:16:08 PDT
Comment on attachment 55033 [details]
patch. Modified to address review comments

sorry, more of the same kind of issue:

WebKit/chromium/src/WebViewImpl.h:188
 +      virtual bool isAcceleratedCompositingActive() const;
this should be in the WebWidget section

WebKit/chromium/src/WebViewImpl.cpp:2054
 +  bool WebViewImpl::isAcceleratedCompositingActive() const
this should be inserted after setTextDirection so that method
implementations are listed in the same order that they are
declared in the header and grouped by interface.
Comment 20 Vangelis Kokkevis 2010-05-04 15:29:53 PDT
Created attachment 55055 [details]
Path - fixing placement of isAcceleratedCompositingActive in the WebViewImpl files
Comment 21 Vangelis Kokkevis 2010-05-04 15:30:34 PDT
Ooops, yes, I should have fixed those too..

(In reply to comment #19)
> (From update of attachment 55033 [details])
> sorry, more of the same kind of issue:
> 
> WebKit/chromium/src/WebViewImpl.h:188
>  +      virtual bool isAcceleratedCompositingActive() const;
> this should be in the WebWidget section

Done

> 
> WebKit/chromium/src/WebViewImpl.cpp:2054
>  +  bool WebViewImpl::isAcceleratedCompositingActive() const
> this should be inserted after setTextDirection so that method
> implementations are listed in the same order that they are
> declared in the header and grouped by interface.

Done
Comment 22 WebKit Commit Bot 2010-05-04 18:08:43 PDT
Comment on attachment 55055 [details]
Path - fixing placement of isAcceleratedCompositingActive in the WebViewImpl files

Clearing flags on attachment: 55055

Committed r58793: <http://trac.webkit.org/changeset/58793>
Comment 23 WebKit Commit Bot 2010-05-04 18:08:50 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Dirk Schulze 2010-05-09 13:45:09 PDT
Created attachment 55507 [details]
Patch
Comment 25 Dirk Schulze 2010-05-09 13:46:39 PDT
Comment on attachment 55507 [details]
Patch

Wrong bug, sorry.
Comment 26 Eric Seidel (no email) 2010-05-09 13:49:17 PDT
Was this webkit-patch error, or human error?