Update Chromium DRT to output binary (instead of base64-encoded) data for web audio testing
Created attachment 101753 [details] Patch
The WebArrayBufferView class is essentially boilerplate code similar to our other WebKit wrapper classes, such as WebRange...
+fishd since this adds a new WebKit API class.
Comment on attachment 101753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101753&action=review LGTM, but let's wait for darin to OK the API changes. > LayoutTests/ChangeLog:4 > +2011-07-22 Chris Rogers <crogers@google.com> > + > + Update Chromium DRT to output binary (instead of base64-encoded) data for web audio testing > + https://bugs.webkit.org/show_bug.cgi?id=65039 From a different change?
Comment on attachment 101753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101753&action=review I just reviewed the Chromium WebKit API changes. > Source/WebKit/chromium/public/WebArrayBufferView.h:47 > + WebArrayBufferView(const WebArrayBufferView& r) : m_private(0) { assign(r); } nit: the variable name "r" is an odd choice. normally, a letter is chosen from the typename. in this case "v" might make sense. > Source/WebKit/chromium/public/WebArrayBufferView.h:60 > + WEBKIT_API unsigned byteLength() const; isn't it valuable to expose byteOffset() too? can't the view be a slice at a non-zero offset? > Source/WebKit/chromium/public/WebArrayBufferView.h:62 > + WebArrayBufferView(const WTF::PassRefPtr<WebCore::ArrayBufferView>&); these should be guarded with #if WEBKIT_IMPLEMENTATION so that no one outside of the webkit implementation can use them. > Source/WebKit/chromium/public/WebArrayBufferView.h:68 > + WebArrayBufferViewPrivate* m_private; the "new way" is to just use the WebCore type here and ditch the WebFooPrivate type. you should also be using WebPrivatePtr<T> to help you with the reference counting and boilerplate associated with wrapping a WebCore reference counted type. > Source/WebKit/chromium/src/WebBindings.cpp:298 > + nit: extra new line should be deleted
Created attachment 101899 [details] Patch
Comment on attachment 101753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101753&action=review >> Source/WebKit/chromium/public/WebArrayBufferView.h:47 >> + WebArrayBufferView(const WebArrayBufferView& r) : m_private(0) { assign(r); } > > nit: the variable name "r" is an odd choice. normally, a letter is chosen > from the typename. in this case "v" might make sense. FIXED >> Source/WebKit/chromium/public/WebArrayBufferView.h:60 >> + WEBKIT_API unsigned byteLength() const; > > isn't it valuable to expose byteOffset() too? can't the view be a slice > at a non-zero offset? Good point -- added byteOffset() >> Source/WebKit/chromium/public/WebArrayBufferView.h:62 >> + WebArrayBufferView(const WTF::PassRefPtr<WebCore::ArrayBufferView>&); > > these should be guarded with #if WEBKIT_IMPLEMENTATION so that no one outside of > the webkit implementation can use them. FIXED >> Source/WebKit/chromium/public/WebArrayBufferView.h:68 >> + WebArrayBufferViewPrivate* m_private; > > the "new way" is to just use the WebCore type here and ditch the > WebFooPrivate type. you should also be using WebPrivatePtr<T> to > help you with the reference counting and boilerplate associated > with wrapping a WebCore reference counted type. FIXED >> Source/WebKit/chromium/src/WebBindings.cpp:298 >> + > > nit: extra new line should be deleted FIXED >> LayoutTests/ChangeLog:4 >> + https://bugs.webkit.org/show_bug.cgi?id=65039 > > From a different change? Sorry - FIXED
Comment on attachment 101899 [details] Patch WebKit API changes LGTM
Committed r91708: <http://trac.webkit.org/changeset/91708>