[webkit-reviews] review requested: [Bug 73593] Add WebArrayBuffer to chromium API : [Attachment 117498] Patch

[bugzilla-daemon at webkit.org]

Dave Michael <dmichael at chromium.org> has asked	for review:
Bug 73593: Add WebArrayBuffer to chromium API
https://bugs.webkit.org/show_bug.cgi?id=73593

Attachment 117498: Patch
https://bugs.webkit.org/attachment.cgi?id=117498&action=review

------- Additional Comments from Dave Michael <dmichael at chromium.org>
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.