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

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


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

I understand that this is just a reflection of a DOM attribute.  What is
the purpose of exposing this attribute?  Are you going to use it for
security decisions?  Or, will you do some resource loading externally to
WebKit?

> Source/WebKit/chromium/public/WebSpeechRecognition.h:43
> +#if WEBKIT_IMPLEMENTATION

nit: we usually put WEBKIT_IMPLEMENTATION sections at the end of the 'public'
section.

> Source/WebKit/chromium/public/WebSpeechRecognition.h:47
> +    WEBKIT_EXPORT void audioStartCallback();

nit: how about giving these "event style" names?

  didStartAudio
  didStartSound
  didStartSpeech
  didEndSpeech
  didEndAudio
  ...

It is not quite clear to me how to rename resultCallback or
noMatchCallback or resultDeletedCallback.  Are those three
possible responses to something?

It would also be helpful to document some of the event flow here.
How is the embedder expected to call these methods?

My point is that I think you should do more than just cleave your
code into two pieces.  I think you should try make the API make
sense on its own.  Imagine someone is trying to hook this up to
a different embedder.  What do they need to know?  Please explain
with comments and helpful function names.

> Source/WebKit/chromium/public/WebSpeechRecognition.h:60
> +    WebCore::SpeechRecognition* m_speechRecognition;

are you sure this shouldn't be a WebPrivatePtr?  SpeechRecognition is
a reference counted class, right?

> Source/WebKit/chromium/public/WebSpeechRecognitionClient.h:40
> +    virtual void start(WebSpeechRecognition*, const
WebVector<WebSpeechGrammar>&, WebString language, bool continuous) {
WEBKIT_ASSERT_NOT_REACHED(); }

are you sure you want to pass WebSpeechRecognition by pointer and not by const
ref?
WebSpeechRecognition could just be a smart pointer around SpeechRecognition.

nit: "language" should be passed as "const WebString&"

nit: are you sure this interface wouldn't be better named
WeBSpeechRecognitionController?  with methods like 'start', 'stop', and
'abort', it sure seems like this interface is more of a controller and less of
a client.  A client is an interface that receives notifications, observes a
system, updates UI, implements policy, etc.  A controller is an interface that
takes action, drives a system, updates state, etc.

> Source/WebKit/chromium/src/SpeechRecognitionClientProxy.cpp:51
> +	   m_recognitionMap.set(recognition, adoptPtr(new
WebSpeechRecognition(recognition)));

I don't understand why you have a map here.  A WebSpeechRecognition should just
be
a wrapper around a SpeechRecognition.  You should be able to map back-n-forth
between
WebSpeechRecognition and SpeechRecognition just like we can map back-n-forth
between
WebNode and Node.

To support an IDMap on the Chromium side, you can give WebSpeechRecognition an
equals
and lessThan method, like we have for WebNode.	Also, you can define operator==
and
operator<.  Then you can use it as a key in a map.  Would that address your
concern?