Bug 78422

Created attachment 126652 [details] Patch

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Comment on attachment 126652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126652&action=review > Source/WebKit/chromium/public/WebFrame.h:-629 > - virtual bool pauseSVGAnimation(const WebString& animationId, Chromium WebKit API change looks good to me.

FYIL new-run-webkit-tests svg/animations --randomize-order --iterations=100 All 9000 tests ran as expected. I don't see any flakiness anymore, even with -g. Note that this patch also doesn't need any rebaselines, it's all cross platform.

CC'ing Dirk/Zoltan/Antti for review.

Comment on attachment 126652 [details] Patch Seems like we should be able to do this in smaller (more reviewable) pieces.

Comment on attachment 126652 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126652&action=review > Source/WebCore/ChangeLog:19 > + via beginElement/endElement - get removed if the begin/endTImes list is updated. Why? typo: s/endTImes/endTimes/ > Source/WebCore/ChangeLog:30 > + - document.getElementsByTagName('animateMotion')[0], always returned 'object SVGElement', instead of Normally this would be another bug report :) > Source/WebCore/ChangeLog:32 > + from SVGANimationElement were not available. Fix that by removing the last hacks from svgtags.in, s/SVGANimationElement/SVGAnimationElement/ > Source/WebCore/ChangeLog:70 > + * svg/svgtags.in: Enable animateMotion/hkern/mpath JS interfaces, which were not enabled, despite their IDLs existed. Another bug report.. normally ;) Do you test them? > LayoutTests/svg/animations/resources/SVGAnimationTestCase.js:77 > + // We still have to name this function runRepaintTest(), as svg/dynamic-updates/ ? where is repaintTest?

(In reply to comment #7) > typo: s/endTImes/endTimes/ > > > Source/WebCore/ChangeLog:30 > > + - document.getElementsByTagName('animateMotion')[0], always returned 'object SVGElement', instead of > > Normally this would be another bug report :) This is covered by animate-path-nested-transforms.js & animate-mpath-insert.js. > s/SVGANimationElement/SVGAnimationElement/ Fixed. > Another bug report.. normally ;) Do you test them? See above, both covered by svg/animations/ tests already. > > > LayoutTests/svg/animations/resources/SVGAnimationTestCase.js:77 > > + // We still have to name this function runRepaintTest(), as svg/dynamic-updates/ Oh, that comment is outdated, will fix.

(In reply to comment #6) > (From update of attachment 126652 [details]) > Seems like we should be able to do this in smaller (more reviewable) pieces. Point taken - but I already discussed with Dirk that he'll review it as-is.

Committed r107682: <http://trac.webkit.org/changeset/107682>

Needs a follow-up change, for some last minute changes to lead to assertions in svg/animations/. Sorry.

Created attachment 126938 [details] Follow-up patch

Attachment 126938 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource From git://git.webkit.org/WebKit + 6612fe4...3db9340 master -> origin/master (forced update) Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ... Currently at 107691 = 6612fe401de446ba8e6f5ae9e558e9f8593ff272 r107688 = 8a7f22a3d3816472d1f57c4f18232bbd60ff4597 r107689 = 818888131b08465aa4d43d5e865741d09553627e r107690 = fe093cab606d262debc8805bb541892699e59265 r107691 = c38205f7c7b217af975be90407f3d5bc69157e8c r107692 = 3db9340249163e9ff8eca4792746293bbc60277d Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc First, rewinding head to replay your work on top of it... Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets Using index info to reconstruct a base tree... <stdin>:1578: trailing whitespace. <stdin>:1647: trailing whitespace. <stdin>:1657: trailing whitespace. <stdin>:1672: trailing whitespace. return 0; <stdin>:1674: trailing whitespace. warning: squelched 7 whitespace errors warning: 12 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 168765 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Committed r107693: <http://trac.webkit.org/changeset/107693>

Reopening to attach new patch.

Created attachment 126955 [details] Cleanup patch

Attachment 126955 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets Using index info to reconstruct a base tree... <stdin>:1578: trailing whitespace. <stdin>:1647: trailing whitespace. <stdin>:1657: trailing whitespace. <stdin>:1672: trailing whitespace. return 0; <stdin>:1674: trailing whitespace. warning: squelched 7 whitespace errors warning: 12 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 168768 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging Source/WebCore/ChangeLog Auto-merging Tools/ChangeLog CONFLICT (content): Merge conflict in Tools/ChangeLog Failed to merge in the changes. Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 126955 [details] Cleanup patch The "cleanup patch" does some house-keeping, and updates chromium expectations.

Comment on attachment 126955 [details] Cleanup patch Attachment 126955 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11518262 New failing tests: svg/animations/animate-text-nested-transforms.html

Created attachment 126977 [details] Cleanup patch - v2

Attachment 126977 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets Using index info to reconstruct a base tree... <stdin>:1578: trailing whitespace. <stdin>:1647: trailing whitespace. <stdin>:1657: trailing whitespace. <stdin>:1672: trailing whitespace. return 0; <stdin>:1674: trailing whitespace. warning: squelched 7 whitespace errors warning: 12 lines add whitespace errors. Falling back to patching base and 3-way merge... warning: too many files (created: 168776 deleted: 3), skipping inexact rename detection Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Auto-merging Source/WebKit2/ChangeLog CONFLICT (content): Merge conflict in Source/WebKit2/ChangeLog Auto-merging Tools/ChangeLog CONFLICT (content): Merge conflict in Tools/ChangeLog Failed to merge in the changes. Patch failed at 0001 [Mac][Win][WK2] Switch to RFC 6455 protocol for WebSockets When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 126977 [details] Cleanup patch - v2 Attachment 126977 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11521280 New failing tests: svg/animations/animate-text-nested-transforms.html

(In reply to comment #22) > (From update of attachment 126977 [details]) > Attachment 126977 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11521280 > > New failing tests: > svg/animations/animate-text-nested-transforms.html Please ignore this failure for review, I'll skip it again, before landing, then check later on with the flakiness dashboard what's happening here. (The test is already marked as failing in trunk, I tried to revert it)

Comment on attachment 126977 [details] Cleanup patch - v2 View in context: https://bugs.webkit.org/attachment.cgi?id=126977&action=review What is that for a style problem? > LayoutTests/ChangeLog:13 > + to expect values like "199.98". Use "200" instead. That doesn't reduce the Can there still be rounding issues between platforms? > LayoutTests/ChangeLog:-400 > - (g.setAttribute.rect.createSVGElement.rect.setAttribute.rect.setAttribute.rect.setAttribute.rect.setAttribute.rect.setAttribute.g.appendChild.animateMotion.createSVGElement.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.g.appendChild.rootSVGElement.appendChild.startSample): What is that? Your change log is broken, or is it really a previous commit that caused that? > LayoutTests/ChangeLog:-408 > - (rootSVGElement.setAttribute.text.createSVGElement.text.setAttribute.text.textContent.string_appeared_here.text.setAttribute.animateMotion.createSVGElement.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.text.appendChild.rootSVGElement.appendChild.startSample): Ditto. > LayoutTests/svg/animations/script-tests/animate-path-nested-transforms.js:20 > +animateMotion.setAttribute("dur", "40s") 40s? Why not 4? > LayoutTests/svg/animations/script-tests/animate-path-nested-transforms.js:41 > + ["animation", 39.999, endSample] Why not 3.999? > LayoutTests/svg/animations/script-tests/animate-text-nested-transforms.js:13 > +animateMotion.setAttribute("dur", "40s") Ditto.

Ah, your ChangeLog seems indeed be broken, why does it work with the other bots?

(In reply to comment #24) > (From update of attachment 126977 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126977&action=review > > What is that for a style problem? Bot is wedged, see webkit-dev. > > > LayoutTests/ChangeLog:13 > > + to expect values like "199.98". Use "200" instead. That doesn't reduce the > > Can there still be rounding issues between platforms? I don't think so - you'll never know though. > > LayoutTests/ChangeLog:-400 > > - (g.setAttribute.rect.createSVGElement.rect.setAttribute.rect.setAttribute.rect.setAttribute.rect.setAttribute.rect.setAttribute.g.appendChild.animateMotion.createSVGElement.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.g.appendChild.rootSVGElement.appendChild.startSample): > > What is that? Your change log is broken, or is it really a previous commit that caused that? Well, prepare-ChangeLog is broken, I have to fix up the ChangeLog manually again. > > > LayoutTests/ChangeLog:-408 > > - (rootSVGElement.setAttribute.text.createSVGElement.text.setAttribute.text.textContent.string_appeared_here.text.setAttribute.animateMotion.createSVGElement.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.animateMotion.setAttribute.text.appendChild.rootSVGElement.appendChild.startSample): > > Ditto. Thanks. > > > LayoutTests/svg/animations/script-tests/animate-path-nested-transforms.js:20 > > +animateMotion.setAttribute("dur", "40s") > > 40s? Why not 4? Ah, I talked with Rob about this on IRC. The animation runs so quick, that 0.001s may result in quite large differences from the start value (eg. rect beginning, x=50, animating to x=5000 in 1s). So I raised the duration - to make it slower. It doesn't matter at all for testing - it doesn't make the testcase slower. > Why not 3.999? See above.

Style bot is still wedged - I will upload a new version anyway to turn cr-linux ews green.

Created attachment 127127 [details] Cleanup patch - v3

Comment on attachment 127127 [details] Cleanup patch - v3 rs=me

Comment on attachment 127127 [details] Cleanup patch - v3 Thanks Simon, landed patch in r107791.