Bug 26651

Created attachment 31788 [details] Patch

<rdar://problem/6989101>

Comment on attachment 31788 [details] Patch Needs some more work.

Created attachment 31825 [details] Patch

Comment on attachment 31825 [details] Patch > 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()->acceleratedCompositingDisabled(); 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.

Created attachment 31864 [details] Patch

Comment on attachment 31864 [details] Patch > Index: WebCore/ChangeLog > =================================================================== > --- WebCore/ChangeLog (revision 45129) > +++ WebCore/ChangeLog (working copy) > @@ -1,3 +1,43 @@ > +2009-06-24 Chris Marrin <cmarrin@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()->acceleratedCompositingEnabled(); 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@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.

Sending WebCore/ChangeLog Sending WebCore/WebCore.base.exp Sending WebCore/page/FrameView.cpp Sending WebCore/page/Settings.cpp Sending WebCore/page/Settings.h Sending WebCore/rendering/RenderLayer.cpp Sending WebCore/rendering/RenderLayer.h Sending WebCore/rendering/RenderLayerBacking.cpp Sending WebCore/rendering/RenderLayerCompositor.cpp Sending WebCore/rendering/RenderLayerCompositor.h Sending WebCore/rendering/RenderObject.h Sending WebKit/mac/ChangeLog Sending WebKit/mac/WebView/WebPreferenceKeysPrivate.h Sending WebKit/mac/WebView/WebPreferences.h Sending WebKit/mac/WebView/WebPreferences.mm Sending WebKit/mac/WebView/WebView.mm Transmitting file data ................ Committed revision 45199.

Simon and I talked about it and decided that this flag should always default to true, to avoid someone running on Leopard, getting the flag set to false, then running on SL and having accelerated compositing turned off. I will submit a patch for this.

Created attachment 31947 [details] Additional Patch

Comment on attachment 31947 [details] Additional Patch File is wrong in the patch, will try again

Created attachment 31948 [details] Additional Patch

Sending WebCore/ChangeLog Sending WebCore/page/Settings.cpp Sending WebKit/mac/ChangeLog Sending WebKit/mac/WebView/WebPreferences.mm Transmitting file data .... Committed revision 45292.

*** Bug 23913 has been marked as a duplicate of this bug. ***