Bug 38220 – [chromium] Add API for querying whether a WebWidget is using GPU accelerated compositing

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Vangelis Kokkevis]

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.

[Vangelis Kokkevis]

Created attachment 54458 [details]
Proposed patch

[Darin Fisher (:fishd, Google)]

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.

[Vangelis Kokkevis]

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

[Vangelis Kokkevis]

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.

[Darin Fisher (:fishd, Google)]

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.

[Vangelis Kokkevis]

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.

[Darin Fisher (:fishd, Google)]

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

[Vangelis Kokkevis]

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

[Vangelis Kokkevis]

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.

[Darin Fisher (:fishd, Google)]

> 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 :-)

[Darin Fisher (:fishd, Google)]

We can then use the "Active" and "DidActivate" on the Chromium side to refer to the same concept.

[Vangelis Kokkevis]

Created attachment 54966 [details]
Proposed patch addressing review comments

Ok, the names have now be changed to isAcceleratedCompositingActive.  Please have another look.

[WebKit Review Bot]

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.

[Vangelis Kokkevis]

Created attachment 54969 [details]
Patch - fixed style issue

[Darin Fisher (:fishd, Google)]

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!

[Vangelis Kokkevis]

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

[Vangelis Kokkevis]

Created attachment 55033 [details]
patch. Modified to address review comments

[Vangelis Kokkevis]

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

[Darin Fisher (:fishd, Google)]

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.

[Vangelis Kokkevis]

Created attachment 55055 [details]
Path - fixing placement of isAcceleratedCompositingActive in the WebViewImpl files

[Vangelis Kokkevis]

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

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Schulze]

Created attachment 55507 [details]
Patch

[Dirk Schulze]

Comment on attachment 55507 [details]
Patch

Wrong bug, sorry.

[Eric Seidel (no email)]

Was this webkit-patch error, or human error?