According to this erratum: http://www.w3.org/2003/01/REC-SVG11-20030114-errata#elementtimecontrol-interface
Created attachment 38781 [details] Patch v1 Didn't add a test though. There don't seem to be any SVG animation tests (apart from the SVG 1.1 test suite files), is that right?
Comment on attachment 38781 [details] Patch v1 Looks great! I assume the spec must have changed from when we wrote these? Looks good except for lack of testing. r- for lack of tests. Just something simple to make sure these return undefined would be sufficient. DOM-only tests would be best for these: http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree shouldBeUndefined("svg.beginElement()"); shouldBeUndefined("svg.beginElementAt(NaN)"); shouldBeUndefined("svg.endElement()"); shouldBeUndefined("svg.endElementAt(NaN)"); etc. Thanks for the patch!
Created attachment 38852 [details] Patch v2 Added a test.
Comment on attachment 38852 [details] Patch v2 Looks fine. I generally write SVG tests as DOM-only tests and just add an iframe with teh SVG content. But what you've done is also totally fine. :) We really should make the js testing framework smart enough to handle a TEMPLATE.svg file as well.
Comment on attachment 38852 [details] Patch v2 Rejecting patch 38852 from commit-queue. This patch will require manual commit. WebKitTools/Scripts/build-webkit failed with exit code 1
Something about this patch caused the build to fail. The commit queue doesn't currently keep build logs, so I can't tell you what the error is w/o applying the patch myself. :(
(In reply to comment #6) > Something about this patch caused the build to fail. The commit queue doesn't > currently keep build logs, so I can't tell you what the error is w/o applying > the patch myself. :( I just tried applying the patch to a clean tree and building from scratch and it worked. It didn't work if I applied it to a tree that had already been built and then ran build-webkit; I got some compilation errors in the generated Objective C bindings for some reason. Does the commit queue tool do a clean before it builds? I can try doing the incremental build again and paste the errors in here if that's helpful.
Created attachment 38946 [details] Build problems when doing an incremental build after applying the patch So I think the problem is that changing ElementTimeControl.idl should cause DOMSVGAnimationElement.mm to be rebuilt (since SVGAnimationElement inherits from ElementTimeControl, and the ObjC bindings have some multiple inheritance flattening thing going on), but the build system doesn't know about this dependency.
Will this wedge the buildbots too? We might need help landing this without horking the tree.
If they don't clean themselves before a build, then I suspect some manual intervention would be required.
It seems we need to file a bug about this class of build problems. Eventually we'll need to devise a solution. That does not need a block landing of this bug, but if it's landed w/o such a fix the committer will need to send an email to webkit-dev about it and be prepared to force the bots to clobber build. It would be best if we could fix the build dependency issue before landing. :)
Comment on attachment 38852 [details] Patch v2 Now that our build system is extra awesome, adding this back to the commit-queue.
Comment on attachment 38852 [details] Patch v2 Rejecting patch 38852 from commit-queue. This patch will require manual commit. Failed to run "['git', 'svn', 'dcommit']" exit_code: 1 cwd: None
Comment on attachment 38852 [details] Patch v2 Race condition when landing the change. Merge conflict during commit: Your file or directory 'WebCore/ChangeLog' is probably out-of-date: resource out of date; try updating at /usr/local/libexec/git-core//git-svn line 469
(In reply to comment #14) > (From update of attachment 38852 [details]) > Race condition when landing the change. > Merge conflict during commit: Your file or directory 'WebCore/ChangeLog' is > probably out-of-date: resource out of date; try updating at > /usr/local/libexec/git-core//git-svn line 469 Can we recognize that error and not mark the patch commit-queue- ?
(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 38852 [details] [details]) > > Race condition when landing the change. > > Merge conflict during commit: Your file or directory 'WebCore/ChangeLog' is > > probably out-of-date: resource out of date; try updating at > > /usr/local/libexec/git-core//git-svn line 469 > > Can we recognize that error and not mark the patch commit-queue- ? Yes, but even if we recognize it we have to be able to recover from it. We can't update w/o having to fix ChangeLogs and deal with possible ChangeLog conflicts during git rebase or update-webkit. If update-webkit was smart enough to handle ChangeLog conflicts for both git and svn we could use it, but it's not yet. I could make the commit-queue to just abort the run silently and loop back to the top in that case. That's slightly dangerous as we might infinite loop, but I agree that behavior would be much better than marking cq-. I'll file a bug and make the change.
Comment on attachment 38852 [details] Patch v2 commit queue error. not your fault.
Comment on attachment 38852 [details] Patch v2 Rejecting patch 38852 from commit-queue. This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1
Comment on attachment 38852 [details] Patch v2 Wow. The commit queue really doesn't like you today, eh? You were bitten by a flakey media test. :( bug 28624. compositing/color-matching/image-color-matching.html -> timed out
Comment on attachment 38852 [details] Patch v2 Oh goody. Now you've hit bug 28845. A variant of the same media tests issue. :( media/video-source-error.html -> crashed I guess I could just commit this manually w/o running the tests. I need to just ignore the media tests from the commit-queue too, bug 28845 is so debilitating. :(
Comment on attachment 38852 [details] Patch v2 Race condition again. My apologies for the un-ending spam. If this doesn't land this time, I'll just land it manually. I'm about to fix this particular type of failure (bug 28316).
Comment on attachment 38852 [details] Patch v2 Clearing flags on attachment: 38852 Committed r48311: <http://trac.webkit.org/changeset/48311>
All reviewed patches have been landed. Closing bug.