Bug 73593 - Add WebArrayBuffer to chromium API
Summary: Add WebArrayBuffer to chromium API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Michael
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-01 15:05 PST by Dave Michael
Modified: 2011-12-19 11:22 PST (History)
2 users (show)

See Also:


Attachments
Patch (11.75 KB, patch)
2011-12-01 15:06 PST, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (11.77 KB, patch)
2011-12-02 09:38 PST, Dave Michael
no flags Details | Formatted Diff | Diff
Patch (11.64 KB, patch)
2011-12-02 13:09 PST, Dave Michael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Michael 2011-12-01 15:05:14 PST
Add WebArrayBuffer to chromium API
Comment 1 Dave Michael 2011-12-01 15:06:14 PST
Created attachment 117498 [details]
Patch
Comment 2 WebKit Review Bot 2011-12-01 15:13:52 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 Darin Fisher (:fishd, Google) 2011-12-01 22:29:26 PST
Comment on attachment 117498 [details]
Patch

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

> Source/WebKit/chromium/public/WebArrayBuffer.h:66
> +    void* data();

you need WEBKIT_EXPORT in front of non-inline methods that can be reached by the embedder.

> Source/WebKit/chromium/public/WebArrayBuffer.h:67
> +    const void* data() const;

hmm, why do we need both a const and non-const version of the data() access?  when would you
need non-const access?  do you intend to mutate the WebArrayBuffer contents this way?

> Source/WebKit/chromium/public/WebArrayBuffer.h:71
> +    WEBKIT_EXPORT v8::Handle<v8::Value>  toV8Value();

nit: looks like there is an extra whitespace in front of toV8Value.
Comment 4 Dave Michael 2011-12-02 09:38:01 PST
Created attachment 117640 [details]
Patch
Comment 5 Dave Michael 2011-12-02 09:38:27 PST
Comment on attachment 117498 [details]
Patch

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

>> Source/WebKit/chromium/public/WebArrayBuffer.h:66
>> +    void* data();
> 
> you need WEBKIT_EXPORT in front of non-inline methods that can be reached by the embedder.

Done.

>> Source/WebKit/chromium/public/WebArrayBuffer.h:67
>> +    const void* data() const;
> 
> hmm, why do we need both a const and non-const version of the data() access?  when would you
> need non-const access?  do you intend to mutate the WebArrayBuffer contents this way?

Yes. This eventually gets plumbed out to the PPB_VarArrayBuffer_Dev interface, where Map() needs to be able to provide a non-const pointer to the buffer. The plugin needs to be able to write to the buffer as well as read it. I could probably get away with _only_ the non-const one, but I may have to const_cast in some contexts in chromium code. This seemed the least evil option :-)

>> Source/WebKit/chromium/public/WebArrayBuffer.h:71
>> +    WEBKIT_EXPORT v8::Handle<v8::Value>  toV8Value();
> 
> nit: looks like there is an extra whitespace in front of toV8Value.

Done.
Comment 6 Dave Michael 2011-12-02 09:39:05 PST
Comment on attachment 117498 [details]
Patch

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

>> Source/WebKit/chromium/public/WebArrayBuffer.h:66
>> +    void* data();
> 
> you need WEBKIT_EXPORT in front of non-inline methods that can be reached by the embedder.

Done.

>> Source/WebKit/chromium/public/WebArrayBuffer.h:67
>> +    const void* data() const;
> 
> hmm, why do we need both a const and non-const version of the data() access?  when would you
> need non-const access?  do you intend to mutate the WebArrayBuffer contents this way?

Yes. This eventually gets plumbed out to the PPB_VarArrayBuffer_Dev interface, where Map() needs to be able to provide a non-const pointer to the buffer. The plugin needs to be able to write to the buffer as well as read it. I could probably get away with _only_ the non-const one, but I may have to const_cast in some contexts in chromium code. This seemed the least evil option :-)

>> Source/WebKit/chromium/public/WebArrayBuffer.h:71
>> +    WEBKIT_EXPORT v8::Handle<v8::Value>  toV8Value();
> 
> nit: looks like there is an extra whitespace in front of toV8Value.

Done.
Comment 7 Darin Fisher (:fishd, Google) 2011-12-02 11:29:19 PST
Comment on attachment 117640 [details]
Patch

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

> Source/WebKit/chromium/public/WebArrayBuffer.h:66
> +    WEBKIT_EXPORT void* data();

WebArrayBufferView has a baseAddress method that is const yet return a void*.
Why shouldn't WebArrayBuffer use the same interface as WebArrayBufferView?
Should WebArrayBufferView::baseAddress() return a |const void*| instead?
Comment 8 Dave Michael 2011-12-02 12:13:06 PST
Comment on attachment 117640 [details]
Patch

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

>> Source/WebKit/chromium/public/WebArrayBuffer.h:66
>> +    WEBKIT_EXPORT void* data();
> 
> WebArrayBufferView has a baseAddress method that is const yet return a void*.
> Why shouldn't WebArrayBuffer use the same interface as WebArrayBufferView?
> Should WebArrayBufferView::baseAddress() return a |const void*| instead?

It's kind of a judgement call. I was more-or-less plumbing out what ArrayBuffer has. I like this way, because it feels more const-correct to me, since WebArrayBuffer effectively owns the buffer.

But a const method just means the implicit 'this' is const, which means the member _pointer_ itself is const, so it would be easy to return a |void*|. The _pointee_ is still mutable. In this case, I think treating the pointee (i.e., the contents of the buffer) as having the same const-ness as the owning WebArrayBuffer makes sense. WebArrayBufferView is somewhat more reasonable to do with just one const method, since the view doesn't own the buffer.

tl;dr: I think it makes sense as I have it, but I'm open to changing it.
Comment 9 Dave Michael 2011-12-02 13:09:24 PST
Created attachment 117674 [details]
Patch
Comment 10 WebKit Review Bot 2011-12-02 19:14:01 PST
Comment on attachment 117674 [details]
Patch

Clearing flags on attachment: 117674

Committed r101896: <http://trac.webkit.org/changeset/101896>
Comment 11 WebKit Review Bot 2011-12-02 19:14:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Eric Seidel (no email) 2011-12-19 11:22:01 PST
Comment on attachment 117498 [details]
Patch

Cleared review? from obsolete attachment 117498 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).