Bug 66646

Summary: ArrayBuffer should have slice method.
Product: WebKit Reporter: Shinya Kawanaka <shinyak>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, kbr, morrita, sam, shinyak, webkit.review.bot, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Shinya Kawanaka 2011-08-21 23:45:38 PDT
ArrayBuffer should have slice method.

See also the following references.

http://code.google.com/p/chromium/issues/detail?id=85047
http://www.khronos.org/registry/typedarray/specs/latest/#5
Comment 1 Shinya Kawanaka 2011-08-21 23:47:20 PDT
Sorry I mistook the URL.

http://code.google.com/p/chromium/issues/detail?id=85049
is correct.

(In reply to comment #0)
> ArrayBuffer should have slice method.
> 
> See also the following references.
> 
> http://code.google.com/p/chromium/issues/detail?id=85047
> http://www.khronos.org/registry/typedarray/specs/latest/#5
Comment 2 Shinya Kawanaka 2011-08-21 23:51:35 PDT
Created attachment 104649 [details]
Patch
Comment 3 Sam Weinig 2011-08-22 08:07:58 PDT
Comment on attachment 104649 [details]
Patch

long in IDL is meant to map to int in c++ code.
Comment 4 Shinya Kawanaka 2011-08-23 18:16:09 PDT
Created attachment 104951 [details]
Patch
Comment 5 Hajime Morrita 2011-10-16 21:59:08 PDT
This looks a trivial change.
What needed for this patch is a drop of love from some expert...
Comment 6 Kenneth Russell 2011-10-18 12:02:42 PDT
Comment on attachment 104951 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104951&action=review

Thanks for picking up this work. The code looks fine and the test looks great. A couple of minor nits to clean up before committing. Also, please mark cq? on the bug if you would like it committed after review.

> LayoutTests/ChangeLog:3
> +        Added unittest for ArrayBuffer#slice

Please use the bug description verbatim along with the URL. If necessary, provide detail in additional paragraphs.

> Source/WebCore/ChangeLog:3
> +        Implemented ArrayBuffer#slice.

Same comment here.

> Source/WebCore/html/canvas/ArrayBuffer.cpp:33
> +static long clampValue(long x, long left, long right)

This should take and return ints instead of longs.

> Source/WebCore/html/canvas/ArrayBuffer.h:54
> +    unsigned index(int idx) const;

This method name is a little confusing; what about "clampIndex"? Also, avoid abbreviations (see http://www.webkit.org/coding/coding-style.html ), so the argument should be named "index".
Comment 7 Shinya Kawanaka 2011-10-18 21:43:09 PDT
Created attachment 111561 [details]
Patch
Comment 8 Shinya Kawanaka 2011-10-18 21:46:42 PDT
Created attachment 111562 [details]
Patch
Comment 9 Shinya Kawanaka 2011-10-18 21:47:35 PDT
Hi, I've fixed them. Could you review it again?

(In reply to comment #6)
> (From update of attachment 104951 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104951&action=review
> 
> Thanks for picking up this work. The code looks fine and the test looks great. A couple of minor nits to clean up before committing. Also, please mark cq? on the bug if you would like it committed after review.
> 
> > LayoutTests/ChangeLog:3
> > +        Added unittest for ArrayBuffer#slice
> 
> Please use the bug description verbatim along with the URL. If necessary, provide detail in additional paragraphs.
> 
> > Source/WebCore/ChangeLog:3
> > +        Implemented ArrayBuffer#slice.
> 
> Same comment here.
> 
> > Source/WebCore/html/canvas/ArrayBuffer.cpp:33
> > +static long clampValue(long x, long left, long right)
> 
> This should take and return ints instead of longs.
> 
> > Source/WebCore/html/canvas/ArrayBuffer.h:54
> > +    unsigned index(int idx) const;
> 
> This method name is a little confusing; what about "clampIndex"? Also, avoid abbreviations (see http://www.webkit.org/coding/coding-style.html ), so the argument should be named "index".
Comment 10 Kenneth Russell 2011-10-19 10:36:42 PDT
Comment on attachment 111562 [details]
Patch

Thanks, looks great. r=me
Comment 11 WebKit Review Bot 2011-10-19 16:30:18 PDT
Comment on attachment 111562 [details]
Patch

Clearing flags on attachment: 111562

Committed r97893: <http://trac.webkit.org/changeset/97893>
Comment 12 WebKit Review Bot 2011-10-19 16:30:24 PDT
All reviewed patches have been landed.  Closing bug.