Bug 25741 - insertItemBefore boundary condition incorrect
Summary: insertItemBefore boundary condition incorrect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-12 16:17 PDT by Adam Barth
Modified: 2009-05-15 18:08 PDT (History)
2 users (show)

See Also:


Attachments
patch (no tests) (800 bytes, patch)
2009-05-12 23:36 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
patch (7.87 KB, patch)
2009-05-15 17:21 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
better patch (7.77 KB, patch)
2009-05-15 17:27 PDT, Adam Barth
oliver: review+
Details | Formatted Diff | Diff
even better patch? (8.62 KB, patch)
2009-05-15 17:52 PDT, Adam Barth
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2009-05-12 16:17:56 PDT
When we get an out-of-bound index here:

http://trac.webkit.org/browser/trunk/WebCore/svg/SVGList.h#L99

we should append instead of throwing an error according to the spec:

http://www.w3.org/TR/SVG/types.html
Comment 1 Adam Barth 2009-05-12 16:18:15 PDT
I have a patch that I can upload when I get home.
Comment 2 Adam Barth 2009-05-12 23:36:24 PDT
Created attachment 30262 [details]
patch (no tests)

Turns out unhorking my build was too much for Mac Mini to handle tonight.  Here's the code part of the patch.  I still need to update the tests.
Comment 3 Adam Barth 2009-05-15 17:21:00 PDT
Created attachment 30405 [details]
patch

patch!
Comment 4 Adam Barth 2009-05-15 17:22:07 PDT
Comment on attachment 30405 [details]
patch

No, that't not right.
Comment 5 Adam Barth 2009-05-15 17:27:16 PDT
Created attachment 30406 [details]
better patch

That's better.
Comment 6 Oliver Hunt 2009-05-15 17:35:48 PDT
Comment on attachment 30406 [details]
better patch

r=me, but i'd prefer it if the test were updated to actually confirm the correct insertion order
Comment 7 Adam Barth 2009-05-15 17:38:47 PDT
(In reply to comment #6)
> (From update of attachment 30406 [details] [review])
> r=me, but i'd prefer it if the test were updated to actually confirm the
> correct insertion order

You mean add the shouldBes after each modification?
Comment 8 Oliver Hunt 2009-05-15 17:42:50 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 30406 [details] [review] [review])
> > r=me, but i'd prefer it if the test were updated to actually confirm the
> > correct insertion order
> 
> You mean add the shouldBes after each modification?
> 
Yeah -- i find myself wondering what inserting null is meant to do? (and we;ll probably want to insert distinguishable values)
Comment 9 Adam Barth 2009-05-15 17:52:28 PDT
Created attachment 30407 [details]
even better patch?

Like the new svglist-insertBeforeItem-appends test?
Comment 10 Oliver Hunt 2009-05-15 17:59:15 PDT
Comment on attachment 30407 [details]
even better patch?

r=me
Comment 11 Adam Barth 2009-05-15 18:08:11 PDT
Sending        LayoutTests/ChangeLog
Sending        LayoutTests/svg/dom/svglist-exception-on-out-bounds-error-expected.txt
Sending        LayoutTests/svg/dom/svglist-exception-on-out-bounds-error.html
Adding         LayoutTests/svg/dom/svglist-insertItemBefore-appends-expected.txt
Adding         LayoutTests/svg/dom/svglist-insertItemBefore-appends.html
Sending        WebCore/ChangeLog
Sending        WebCore/svg/SVGList.h
Transmitting file data .......
Committed revision 43795.