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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted 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 31864: Patch
https://bugs.webkit.org/attachment.cgi?id=31864&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 45129)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,43 @@
> +2009-06-24  Chris Marrin  <cmarrin at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=26651
> +
> +	   Preference is named "WebKitAcceleratedCompositingDisabled"

It's WebKitAcceleratedCompositingEnabled now.

> +	   and is a boolean value. Prevents compositing layers from
> +	   being created, which prevents hardware animation from running.
> +	   Also forces video to do software rendering. Added a cache for
> +	   the flag in RenderLayerCompositing and made it all work
> +	   on-the-fly when the flag is changed while a page is loaded.
> + 
> +	   * WebCore.base.exp:
> +	   * page/FrameView.cpp:
> +	   (WebCore::FrameView::updateCompositingLayers):
> +	   * page/Settings.cpp:
> +	   (WebCore::Settings::setAcceleratedCompositingDisabled):
> +	   * page/Settings.h:
> +	   (WebCore::Settings::acceleratedCompositingDisabled):

These method names changed, so you should run prepare-changelog again.

> Index: WebCore/page/Settings.cpp
> ===================================================================

> -static void setNeedsReapplyStylesInAllFrames(Page* page)
> +static void setNeedsReapplyStylesInAllFrames(Page* page, bool
/*updateCompositingLayers*/ = false)
>  {
> -    for (Frame* frame = page->mainFrame(); frame; frame =
frame->tree()->traverseNext())
> +    for (Frame* frame = page->mainFrame(); frame; frame =
frame->tree()->traverseNext()) {
> +	   //if (updateCompositingLayers)
> +	   //	
frame->view()->updateCompositingLayers(FrameView::ForcedCompositingUpdate);
>	   frame->setNeedsReapplyStyles();
> +    }

Some unwanted changes here.

>  }
>  
>  #if USE(SAFARI_THEME)
> @@ -104,6 +108,7 @@ Settings::Settings(Page* page)
>      // they can't use by. Leaving enabled for now to not change existing
behavior.
>      , m_downloadableBinaryFontsEnabled(true)
>      , m_xssAuditorEnabled(false)
> +    , m_acceleratedCompositingEnabled(true)

I think you should #ifdef to default to true if ACCEL_COMP is defined, false
otherwise.



> +void RenderLayerCompositor::cacheAcceleratedCompositingEnabledFlag()
> +{
> +    

Extra blank line here.

> +    bool hasAcceleratedCompositing = m_renderView->document()->settings() &&
m_renderView->frameView()->frame()->page()->settings()->acceleratedCompositingE
nabled();

The cleaner way to do this is:

  bool hasAcceleratedCompositing = false;
  if (Settings* settings = m_renderView->document()->settings())
    hasAcceleratedCompositing = settings-> acceleratedCompositingEnabled();


> Index: WebCore/rendering/RenderLayerCompositor.h
> ===================================================================
> --- WebCore/rendering/RenderLayerCompositor.h (revision 44993)
> +++ WebCore/rendering/RenderLayerCompositor.h (working copy)
> @@ -56,6 +56,12 @@ 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() { return m_hasAcceleratedCompositing; }


Should be const.


> Index: WebKit/mac/ChangeLog
> ===================================================================
> --- WebKit/mac/ChangeLog	(revision 45129)
> +++ WebKit/mac/ChangeLog	(working copy)
> @@ -1,3 +1,25 @@
> +2009-06-24  Chris Marrin  <cmarrin at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=26651
> +
> +	   Preference is named "WebKitAcceleratedCompositingDisabled"
> +	   and is a boolean value. Prevents compositing layers from
> +	   being created, which prevents hardware animation from running.
> +	   Also forces video to do software rendering. Added a cache for
> +	   the flag in RenderLayerCompositing and made it all work
> +	   on-the-fly when the flag is changed while a page is loaded.
> +
> +	   * WebView/WebPreferenceKeysPrivate.h:
> +	   * WebView/WebPreferences.h:
> +	   * WebView/WebPreferences.mm:
> +	   (+[WebPreferences initialize]):
> +	   (-[WebPreferences acceleratedCompositingDisabled]):
> +	   (-[WebPreferences setAcceleratedCompositingDisabled:]):
> +	   * WebView/WebView.mm:
> +	   (-[WebView _preferencesChangedNotification:]):

Need to update this changelog too.

> Index: WebKit/mac/WebView/WebPreferences.mm
> ===================================================================
> --- WebKit/mac/WebView/WebPreferences.mm	(revision 44993)
> +++ WebKit/mac/WebView/WebPreferences.mm	(working copy)
> @@ -348,6 +348,7 @@ static WebCacheModel cacheModelForMainBu
>	   [NSNumber numberWithBool:NO],  
WebKitOfflineWebApplicationCacheEnabledPreferenceKey,
>	   [NSNumber numberWithBool:YES],  WebKitZoomsTextOnlyPreferenceKey,
>	   [NSNumber numberWithBool:NO],  
WebKitXSSAuditorEnabledPreferenceKey,
> +	   [NSNumber numberWithBool:YES], 
WebKitAcceleratedCompositingEnabledPreferenceKey,
>	   nil];

I think it should only default to YES when ACCEL_COMP is defined.

> +- (BOOL)acceleratedCompositingEnabled
> +{
> +    return [self _boolValueForKey:
WebKitAcceleratedCompositingEnabledPreferenceKey];
> +}
> +
> +- (void)setAcceleratedCompositingEnabled:(BOOL)enabled
> +{
> +    [self _setBoolValue: enabled forKey:
WebKitAcceleratedCompositingEnabledPreferenceKey];

WebKit style is no spaces after the colons.

r=me with those cleanups.