Bug 114915

<rdar://problem/13088947>

Created attachment 198965 [details] Patch

Attachment 198965 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'LayoutTests/animations/added-while-suspended-expected.txt', u'LayoutTests/animations/added-while-suspended.html', u'LayoutTests/transitions/started-while-suspended-expected.txt', u'LayoutTests/transitions/started-while-suspended.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/page/animation/AnimationBase.cpp', u'Source/WebCore/page/animation/AnimationBase.h', u'Source/WebCore/page/animation/AnimationController.cpp', u'Source/WebCore/page/animation/AnimationController.h', u'Source/WebCore/page/animation/AnimationControllerPrivate.h', u'Source/WebCore/page/animation/CompositeAnimation.cpp', u'Source/WebCore/page/animation/CompositeAnimation.h', u'Source/WebCore/page/animation/KeyframeAnimation.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebView/WebView.mm', u'Source/WebKit/mac/WebView/WebViewData.h', u'Source/WebKit/mac/WebView/WebViewData.mm', u'Source/autotools/symbols.filter']" exit_code: 1 Source/WebCore/page/animation/AnimationBase.h:81: One space before end of line comments [whitespace/comments] [5] Total errors found: 1 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #3) > Attachment 198965 [details] did not pass style-queue: > > Source/WebCore/page/animation/AnimationBase.h:81: One space before end of line comments [whitespace/comments] [5] > Total errors found: 1 in 23 files Following existing style.

Comment on attachment 198965 [details] Patch Attachment 198965 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/181035

Comment on attachment 198965 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198965&action=review > Source/WebCore/page/animation/AnimationBase.cpp:375 > + Extra blank line. > Source/WebCore/page/animation/AnimationController.cpp:269 > + if (isSuspended()) > + return; I wonder if we should make suspend/resume nestable, with a counter?? > Source/WebCore/page/animation/AnimationController.h:75 > + void animationsForDocumentMayStart(Document*); This name is ambiguous: it can be read as a getter, and it's not clear how it interacts with suspend/resume. I guess it's startAnimationsIfNotSuspended()? > Source/WebCore/page/animation/CompositeAnimation.h:95 > - bool m_suspended; > + bool m_isSuspended; I'd suggest not renaming the member var. > Source/WebKit/mac/WebView/WebView.mm:-2774 > - (BOOL)cssAnimationsSuspended > { > - return _private->cssAnimationsSuspended; > + Frame* frame = core([self mainFrame]); > + if (frame) > + return frame->animation()->isSuspended(); > + > + return false; > } > > - (void)setCSSAnimationsSuspended:(BOOL)suspended > { > - if (suspended == _private->cssAnimationsSuspended) > + Frame* frame = core([self mainFrame]); > + if (suspended == frame->animation()->isSuspended()) > return; > > - _private->cssAnimationsSuspended = suspended; > - > - Frame* frame = core([self mainFrame]); After these changes does the suspended state survive across pages loads, which may change the Frame? I think we need to preserve that behavior.

Comment on attachment 198965 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198965&action=review >> Source/WebCore/page/animation/AnimationBase.cpp:375 >> + > > Extra blank line. Fixed. >> Source/WebCore/page/animation/AnimationController.cpp:269 >> + return; > > I wonder if we should make suspend/resume nestable, with a counter?? I don't want to change our API behaviour in this patch. This is a private API anyway (at the moment). >> Source/WebCore/page/animation/AnimationController.h:75 >> + void animationsForDocumentMayStart(Document*); > > This name is ambiguous: it can be read as a getter, and it's not clear how it interacts with suspend/resume. I guess it's startAnimationsIfNotSuspended()? Yes, fixed. >> Source/WebCore/page/animation/CompositeAnimation.h:95 >> + bool m_isSuspended; > > I'd suggest not renaming the member var. OK. Reverted. >> Source/WebKit/mac/WebView/WebView.mm:-2774 >> - Frame* frame = core([self mainFrame]); > > After these changes does the suspended state survive across pages loads, which may change the Frame? I think we need to preserve that behavior. That behaviour didn't exist. Previously, we'd just return the member variable in WebViewData, which did not necessarily reflect reality. As you say, the frame could change, or the document could restart animations, but we'd never know. At least now the API does reflect the actual value. You're right that it might change from underneath us. However, I don't want to fix that in this patch. The few clients we have should be checking in the load delegates.

Created attachment 198978 [details] Patch

New patch with Simon's suggestions (and ignores some of them :)

Attachment 198978 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'LayoutTests/animations/added-while-suspended-expected.txt', u'LayoutTests/animations/added-while-suspended.html', u'LayoutTests/transitions/started-while-suspended-expected.txt', u'LayoutTests/transitions/started-while-suspended.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/page/animation/AnimationBase.cpp', u'Source/WebCore/page/animation/AnimationBase.h', u'Source/WebCore/page/animation/AnimationController.cpp', u'Source/WebCore/page/animation/AnimationController.h', u'Source/WebCore/page/animation/AnimationControllerPrivate.h', u'Source/WebCore/page/animation/CompositeAnimation.cpp', u'Source/WebCore/page/animation/CompositeAnimation.h', u'Source/WebCore/page/animation/KeyframeAnimation.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/testing/Internals.h', u'Source/WebCore/testing/Internals.idl', u'Source/WebKit/ChangeLog', u'Source/WebKit/WebKit.vcxproj/WebKitExportGenerator/WebKitExports.def.in', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebView/WebView.mm', u'Source/WebKit/mac/WebView/WebViewData.h', u'Source/WebKit/mac/WebView/WebViewData.mm', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebKit.vcproj/WebKitExports.def.in', u'Source/autotools/symbols.filter']" exit_code: 1 Source/WebCore/page/animation/AnimationBase.h:81: One space before end of line comments [whitespace/comments] [5] Total errors found: 1 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #7) > > After these changes does the suspended state survive across pages loads, which may change the Frame? I think we need to preserve that behavior. > > That behaviour didn't exist. Previously, we'd just return the member variable in WebViewData, which did not necessarily reflect reality. As you say, the frame could change, or the document could restart animations, but we'd never know. At least now the API does reflect the actual value. > > You're right that it might change from underneath us. However, I don't want to fix that in this patch. The few clients we have should be checking in the load delegates. But previously someone could do: [webview setCSSAnimationsSuspended:YES] ...start page load Are you saying that the animations in the loaded document would not be suspended in this case, i.e. that WebCore didn't ask the WebView for the suspended state? With your patch (and possibly before), the situation is: [webview setCSSAnimationsSuspended:YES] ...load a page... animations are not suspended. so it feels like the SPI is broken. State set on the WebView should stick. I know this is a behavior change, but seems like the right thing to fix.

(In reply to comment #11) > (In reply to comment #7) > > > After these changes does the suspended state survive across pages loads, which may change the Frame? I think we need to preserve that behavior. > > > > That behaviour didn't exist. Previously, we'd just return the member variable in WebViewData, which did not necessarily reflect reality. As you say, the frame could change, or the document could restart animations, but we'd never know. At least now the API does reflect the actual value. > > > > You're right that it might change from underneath us. However, I don't want to fix that in this patch. The few clients we have should be checking in the load delegates. > > But previously someone could do: > > [webview setCSSAnimationsSuspended:YES] > ...start page load > > Are you saying that the animations in the loaded document would not be suspended in this case, i.e. that WebCore didn't ask the WebView for the suspended state? That is correct. WebView kept its own flag. If you changed the flag it would call into WebCore. However, WebCore was doing whatever it wanted under the hood, and never worrying what the WebView had requested. > With your patch (and possibly before), the situation is: > > [webview setCSSAnimationsSuspended:YES] > ...load a page... > animations are not suspended. > > so it feels like the SPI is broken. State set on the WebView should stick. I know this is a behavior change, but seems like the right thing to fix. OK.

Comment on attachment 198978 [details] Patch Attachment 198978 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/110857 New failing tests: transitions/transition-end-event-rendering.html transitions/transition-end-event-left.html http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html animations/transition-and-animation-1.html transitions/transition-end-event-multiple-02.html transitions/transition-end-event-attributes.html fast/css/image-rendering.html transitions/transition-duration-cleared-in-transitionend-crash.html transitions/transition-end-event-multiple-03.html transitions/svg-transitions.html transitions/transition-end-event-multiple-01.html animations/unanimated-style.html transitions/transform-op-list-match.html transitions/transition-drt-api-delay.html transitions/transition-end-event-container.html transitions/transform-op-list-no-match.html animations/timing-functions.html transitions/transition-end-event-all-properties.html transitions/text-indent-transition.html transitions/transition-drt-api.html transitions/transition-end-event-destroy-renderer.html transitions/transition-end-event-nested.html platform/mac/fast/speechsynthesis/speech-synthesis-pause-resume.html animations/transition-and-animation-2.html platform/mac/editing/deleting/deletionUI-single-instance.html animations/transition-and-animation-3.html transitions/transition-end-event-multiple-04.html fast/loader/javascript-url-in-object.html transitions/transition-end-event-set-none.html animations/width-using-ems.html

Created attachment 199220 [details] Archive of layout-test-results from webkit-ews-08 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.2

(In reply to comment #12) > > so it feels like the SPI is broken. State set on the WebView should stick. I know this is a behavior change, but seems like the right thing to fix. > > OK. It turns out that this actually is the case, so this patch is ready.

Committed r149576: <http://trac.webkit.org/changeset/149576>