Last Comment Bug 722071 - Implement array style indexing for SVGStringList
: Implement array style indexing for SVGStringList
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Jonathan Watt [:jwatt] (back in October - email directly if necessary)
:
Mentors:
Depends on: 631437 724993
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-28 11:07 PST by Jonathan Watt [:jwatt] (back in October - email directly if necessary)
Modified: 2012-02-11 13:53 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (6.11 KB, patch)
2012-01-28 11:12 PST, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
mrbkap: review+
Details | Diff | Splinter Review

Description Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-28 11:07:23 PST
For compatibility with the other list types (see bug 631437), we should implement array style indexing for SVGStringList.
Comment 1 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-28 11:12:31 PST
Created attachment 592427 [details] [diff] [review]
patch
Comment 2 Mounir Lamouri (:mounir) 2012-01-30 05:49:38 PST
Comment on attachment 592427 [details] [diff] [review]
patch

Review of attachment 592427 [details] [diff] [review]:
-----------------------------------------------------------------

Blake seems more appropriate to do this review.
Comment 3 Blake Kaplan (:mrbkap) 2012-01-30 06:16:32 PST
Comment on attachment 592427 [details] [diff] [review]
patch

Review of attachment 592427 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMClassInfo.cpp
@@ +10912,5 @@
> +// SVGStringList helper
> +
> +NS_IMETHODIMP
> +nsSVGStringListSH::GetProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
> +                               JSObject *obj, jsid id, jsval *vp, bool *_retval)

This implementation of GetProperty doesn't seem to add anything to the base class' implementation. Seems like it can be removed.

@@ +10954,5 @@
> +    list->GetLength(&length);
> +    NS_ASSERTION(PRUint32(aIndex) >= length, "Item should only return null for out-of-bounds access");
> +  }
> +#endif
> +  if (rv == NS_ERROR_DOM_INDEX_SIZE_ERR) {

Are there any tests to exercise this codepath?
Comment 4 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-01-30 06:25:48 PST
(In reply to Blake Kaplan (:mrbkap) from comment #3)
> This implementation of GetProperty doesn't seem to add anything to the base
> class' implementation. Seems like it can be removed.

Ah, yeah I'll remove that.

> Are there any tests to exercise this codepath?

Yes, specifically the test that is modified in the patch (the ok() test after the 'for' loop).

Thanks for the speedy review!
Comment 5 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2012-02-01 11:52:02 PST
https://hg.mozilla.org/mozilla-central/rev/7b0c5dcf5bcb

Note You need to log in before you can comment on or make changes to this bug.