Bug 28831 - Make ElementTimeControl methods return void and not throw
Summary: Make ElementTimeControl methods return void and not throw
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Minor
Assignee: Nobody
URL:
Keywords:
Depends on: 29114
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-29 19:19 PDT by Cameron McCormack (:heycam)
Modified: 2009-09-11 11:48 PDT (History)
3 users (show)

See Also:


Attachments
Patch v1 (5.76 KB, patch)
2009-08-29 19:28 PDT, Cameron McCormack (:heycam)
eric: review-
Details | Formatted Diff | Diff
Patch v2 (8.42 KB, patch)
2009-09-01 02:28 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Build problems when doing an incremental build after applying the patch (281.25 KB, text/plain)
2009-09-02 16:38 PDT, Cameron McCormack (:heycam)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2009-08-29 19:19:04 PDT
According to this erratum:

http://www.w3.org/2003/01/REC-SVG11-20030114-errata#elementtimecontrol-interface
Comment 1 Cameron McCormack (:heycam) 2009-08-29 19:28:44 PDT
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 2 Eric Seidel (no email) 2009-08-31 03:10:26 PDT
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!
Comment 3 Cameron McCormack (:heycam) 2009-09-01 02:28:49 PDT
Created attachment 38852 [details]
Patch v2

Added a test.
Comment 4 Eric Seidel (no email) 2009-09-01 08:07:07 PDT
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 5 Eric Seidel (no email) 2009-09-01 08:10:12 PDT
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
Comment 6 Eric Seidel (no email) 2009-09-01 16:13:11 PDT
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. :(
Comment 7 Cameron McCormack (:heycam) 2009-09-02 15:41:40 PDT
(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.
Comment 8 Cameron McCormack (:heycam) 2009-09-02 16:38:34 PDT
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.
Comment 9 Adam Barth 2009-09-06 22:01:00 PDT
Will this wedge the buildbots too?  We might need help landing this without horking the tree.
Comment 10 Cameron McCormack (:heycam) 2009-09-06 22:11:05 PDT
If they don't clean themselves before a build, then I suspect some manual intervention would be required.
Comment 11 Eric Seidel (no email) 2009-09-08 10:16:24 PDT
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 12 Eric Seidel (no email) 2009-09-10 14:35:57 PDT
Comment on attachment 38852 [details]
Patch v2

Now that our build system is extra awesome, adding this back to the commit-queue.
Comment 13 WebKit Commit Bot 2009-09-10 14:52:44 PDT
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 14 Eric Seidel (no email) 2009-09-10 14:55:49 PDT
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
Comment 15 Adam Barth 2009-09-10 14:57:32 PDT
(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- ?
Comment 16 Eric Seidel (no email) 2009-09-10 15:04:04 PDT
(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 17 WebKit Commit Bot 2009-09-11 01:28:00 PDT
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 18 Eric Seidel (no email) 2009-09-11 09:19:22 PDT
Comment on attachment 38852 [details]
Patch v2

commit queue error.  not your fault.
Comment 19 WebKit Commit Bot 2009-09-11 10:26:50 PDT
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 20 Eric Seidel (no email) 2009-09-11 10:32:53 PDT
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 21 WebKit Commit Bot 2009-09-11 10:45:41 PDT
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 22 Eric Seidel (no email) 2009-09-11 10:48:25 PDT
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 23 WebKit Commit Bot 2009-09-11 11:08:10 PDT
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 24 Eric Seidel (no email) 2009-09-11 11:36:02 PDT
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 25 WebKit Commit Bot 2009-09-11 11:47:56 PDT
Comment on attachment 38852 [details]
Patch v2

Clearing flags on attachment: 38852

Committed r48311: <http://trac.webkit.org/changeset/48311>
Comment 26 WebKit Commit Bot 2009-09-11 11:48:07 PDT
All reviewed patches have been landed.  Closing bug.