[webkit-reviews] review denied: [Bug 26651] Add a preference to disable hardware acceleration : [Attachment 31825] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 26651: Add a preference to disable hardware acceleration
https://bugs.webkit.org/show_bug.cgi?id=26651

Attachment 31825: Patch
https://bugs.webkit.org/attachment.cgi?id=31825&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/page/Settings.cpp
> ===================================================================

Is m_acceleratedCompositingDisabled initialized in the constructor?

> +void Settings::setAcceleratedCompositingDisabled(bool disabled)
> +{
> +    if (m_acceleratedCompositingDisabled == disabled)
> +	   return;

It's not worth doing this optimization here.

> Index: WebCore/page/Settings.h
> ===================================================================
> --- WebCore/page/Settings.h	(revision 44993)
> +++ WebCore/page/Settings.h	(working copy)
> @@ -241,6 +241,9 @@ namespace WebCore {
>	   void setXSSAuditorEnabled(bool);
>	   bool xssAuditorEnabled() const { return m_xssAuditorEnabled; }
>  
> +	   void setAcceleratedCompositingDisabled(bool);
> +	   bool acceleratedCompositingDisabled() const { return
m_acceleratedCompositingDisabled; }

I think we should turn this around, and rename these as
"acceleratedCompositingEnabled".
It can default to true if ACCELERATED_COMPOSITING is defined.

> +bool RenderLayer::hasAcceleratedCompositing() const
> +{
> +#if USE(ACCELERATED_COMPOSITING)
> +    return compositor()->hasAcceleratedCompositing();
> +#else
> +    return false;
> +#endif
> +}

I don't think this is ever used.

> Index: WebCore/rendering/RenderLayerCompositor.cpp
> ===================================================================

> +bool RenderLayerCompositor::hasAcceleratedCompositing(bool updateFromPref
/*= false*/)
> +{
> +    if (updateFromPref) {
> +	   bool accelerationDisabled =
m_renderView->frameView()->frame()->page()->settings() &&
m_renderView->frameView()->frame()->page()->settings()->acceleratedCompositingD
isabled();

m_renderView->document()->settings() is shorter.


> Index: WebCore/rendering/RenderLayerCompositor.h
> ===================================================================
> --- WebCore/rendering/RenderLayerCompositor.h (revision 44993)
> +++ WebCore/rendering/RenderLayerCompositor.h (working copy)
> @@ -56,6 +56,9 @@ public:
>      // This will make a compositing layer at the root automatically, and
hook up to
>      // the native view/window system.
>      void enableCompositingMode(bool enable = true);
> +    
> +    // Returns true if the accelerated compositing is enabled
> +    bool hasAcceleratedCompositing(bool updateFromPref=false);

Darin hates boolean arguments, because they make the code at the call site hard
to read, and I agree.
An enum would make it more readable, or just two methods:
hasAcceleratedCompositing() and hasAcceleratedCompositingCached()
or something.

> Index: WebKit/mac/WebView/WebPreferenceKeysPrivate.h
> ===================================================================
> --- WebKit/mac/WebView/WebPreferenceKeysPrivate.h	(revision 44993)
> +++ WebKit/mac/WebView/WebPreferenceKeysPrivate.h	(working copy)
> @@ -82,6 +82,7 @@
>  #define WebKitOfflineWebApplicationCacheEnabledPreferenceKey
@"WebKitOfflineWebApplicationCacheEnabled"
>  #define WebKitZoomsTextOnlyPreferenceKey @"WebKitZoomsTextOnly"
>  #define WebKitXSSAuditorEnabledPreferenceKey @"WebKitXSSAuditorEnabled"
> +#define WebKitAcceleratedCompositingDisabledPreferenceKey
@"WebKitAcceleratedCompositingDisabled"

I'm torn about whether the key should have "enabled" or "disabled" in the name.
The closest equivalent
is WebKitEnableDeferredUpdates, which is only on for SnowLeopard, so I suggest
we follow that.

> Index: WebKit/mac/WebView/WebPreferences.h
> ===================================================================

> +    @method acceleratedCompositingDisabled
> +*/
> +- (BOOL)acceleratedCompositingDisabled;
> +
> +/*!
> +    @method setAcceleratedCompositingDisabled:
> +    @param size

size?

> +*/
> +- (void)setAcceleratedCompositingDisabled:(BOOL)disabled;

I also think these should use 'enabled'.

So one more round and I think we've got it.