[webkit-reviews] review denied: [Bug 81667] Speech JavaScript API: Plumbing for Chromium : [Attachment 132832] 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 132832: Patch
https://bugs.webkit.org/attachment.cgi?id=132832&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=132832&action=review


> Source/WebKit/chromium/public/WebSpeechGrammar.h:45
> +    WEBKIT_EXPORT WebURL src() const;

nit: src -> sourceURL ?

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

is visibilityHidden a command?	it seems like it is a notification about the
visibility state changing to hidden?

> Source/WebKit/chromium/public/WebViewClient.h:320
> +    // Scripted speech -----------------------------------------------------


does scripted speech really require a different section?  can we just extend
the "Speech" section?

> Source/WebKit/chromium/public/WebViewClient.h:323
> +    virtual WebSpeechRecognitionClient* speechRecognitionClient() { return
0; }

is the caller expected to delete this pointer when they are done with it?  i'm
guessing
the answer is NO since this function is not named "createFoo".	assuming the
answer is
NO, then WebSpeechRecognitionClient should have a protected destructor to
prevent unwanted
deletion through this pointer.

> Source/WebKit/chromium/src/SpeechRecognitionClientProxy.cpp:50
> +    if (!m_recognitionMap.contains(recognition)) {

usually we put ID maps like this on the chromium side?	does WebKit need
this map?  it seems like it might be better to create an API wrapper for
WebSpeechRecognition instead.

the WebKit API is supposed to be a thin layer between chromium and webcore.
if chromium needs IDs for IPC purposes, then that is a chromium concern,
not a webkit concern.