Add WebArrayBuffer to chromium API
Created attachment 117498 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
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.
Created attachment 117640 [details] Patch
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 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 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.
Created attachment 117674 [details] Patch
Comment on attachment 117674 [details] Patch Clearing flags on attachment: 117674 Committed r101896: <http://trac.webkit.org/changeset/101896>
All reviewed patches have been landed. Closing bug.
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).