[webkit-reviews] review denied: [Bug 65039] Update Chromium DRT to output binary (instead of base64-encoded) data for web audio testing : [Attachment 101753] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Chris Rogers
<crogers at google.com>'s request for review:
Bug 65039: Update Chromium DRT to output binary (instead of base64-encoded)
data for web audio testing
https://bugs.webkit.org/show_bug.cgi?id=65039

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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