Bug 73593 – Add WebArrayBuffer to chromium API

• Component: New Bugs
• Status: RESOLVED FIXED

[Dave Michael]

Add WebArrayBuffer to chromium API

[Dave Michael]

Created attachment 117498 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

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.

[Dave Michael]

Created attachment 117640 [details]
Patch

[Dave Michael]

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.

[Dave Michael]

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.

[Darin Fisher (:fishd, Google)]

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?

[Dave Michael]

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.

[Dave Michael]

Created attachment 117674 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117674 [details]
Patch

Clearing flags on attachment: 117674

Committed r101896: <http://trac.webkit.org/changeset/101896>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

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).