Summary: | Add WebArrayBuffer to chromium API | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dave Michael <dmichael> | ||||||||
Component: | New Bugs | Assignee: | Dave Michael <dmichael> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | fishd, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Dave Michael
2011-12-01 15:05:14 PST
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 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). |