[webkit-reviews] review denied: [Bug 81667] Speech JavaScript API: Plumbing for Chromium : [Attachment 133811] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Hans Wennborg
<hans at chromium.org>'s request for review:
Bug 81667: Speech JavaScript API: Plumbing for Chromium
https://bugs.webkit.org/show_bug.cgi?id=81667

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
Sorry for the high latency in getting feedback to you.	I think there are some
pretty considerable design problems in this API that should be resolved.


View in context: https://bugs.webkit.org/attachment.cgi?id=133811&action=review


> Source/WebKit/chromium/public/WebSpeechRecognition.h:42
> +// Wrapper around a SpeechRecognition pointer.

nit: this comment is not very helpful to WebKit API users.  the point of the
API is to hide WebCore details.  it would be better to have a comment that
explains what a WebSpeechRecognition object is.  it seems to be an object that
represents a "notification sink" for events related to a speech recognition
request.
i think the class is probably poorly named since its name doesn't really match
up
with its function.

> Source/WebKit/chromium/public/WebSpeechRecognition.h:107
> +    WEBKIT_EXPORT void didReceiveError(const WebString& msg, unsigned short
code);

nit: spell it out... msg -> message

> Source/WebKit/chromium/public/WebSpeechRecognitionClient.h:38
> +class WebSpeechRecognitionClient {

i'm still not sure why this is named with a Client suffix.  its methods are
clearly
about controlling something...

> Source/WebKit/chromium/public/WebSpeechRecognitionClient.h:49
> +    virtual void notifyVisibilityHidden() { WEBKIT_ASSERT_NOT_REACHED(); }

i don't understand why this method exists.  the embedder already knows when the

page is hidden.  afterall, it is the embedder who communicates that to WebKit
via WebView::setVisibilityState.  Why does the embedder need WebKit to tell it
what it already knows?

perhaps you are just trying to add a method to tell the embedder to abort any
ongoing speech recognition?  in that case, the method name should be specific
to that.