Bug 65039 – Update Chromium DRT to output binary (instead of base64-encoded) data for web audio testing

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Update Chromium DRT to output binary (instead of base64-encoded) data for web audio testing

[Chris Rogers]

Created attachment 101753 [details]
Patch

[Chris Rogers]

The WebArrayBufferView class is essentially boilerplate code similar to our other WebKit wrapper classes, such as WebRange...

[Tony Chang]

+fishd since this adds a new WebKit API class.

[Tony Chang]

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
> [phone]  Chris Rogers  <[e-mail]>
> +
> +        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?

[Darin Fisher (:fishd, Google)]

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

[Chris Rogers]

Created attachment 101899 [details]
Patch

[Chris Rogers]

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

[Darin Fisher (:fishd, Google)]

Comment on attachment 101899 [details]
Patch

WebKit API changes LGTM

[Chris Rogers]

Committed r91708: <http://trac.webkit.org/changeset/91708>