Bug 28831 – Make ElementTimeControl methods return void and not throw

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Cameron McCormack (:heycam)]

According to this erratum:

http://www.w3.org/2003/01/REC-SVG11-20030114-errata#elementtimecontrol-interface

[Cameron McCormack (:heycam)]

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?

[Eric Seidel (no email)]

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!

[Cameron McCormack (:heycam)]

Created attachment 38852 [details]
Patch v2

Added a test.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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. :(

[Cameron McCormack (:heycam)]

(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.

[Cameron McCormack (:heycam)]

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.

[Adam Barth]

Will this wedge the buildbots too?  We might need help landing this without horking the tree.

[Cameron McCormack (:heycam)]

If they don't clean themselves before a build, then I suspect some manual intervention would be required.

[Eric Seidel (no email)]

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. :)

[Eric Seidel (no email)]

Comment on attachment 38852 [details]
Patch v2

Now that our build system is extra awesome, adding this back to the commit-queue.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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

[Adam Barth]

(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- ?

[Eric Seidel (no email)]

(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.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 38852 [details]
Patch v2

commit queue error.  not your fault.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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. :(

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

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).

[WebKit Commit Bot]

Comment on attachment 38852 [details]
Patch v2

Clearing flags on attachment: 38852

Committed r48311: <http://trac.webkit.org/changeset/48311>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.