[Webkit-unassigned] [Bug 42367] Speech input plumbing in webkit

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=42367





--- Comment #5 from Marcus Bulach <bulach at chromium.org>  2010-07-16 04:17:16 PST ---
(In reply to comment #4)
> Doh, didn't add the new files to chromium/WebKit.gyp (since I tested on windows in local machine and added manually to the .vcproj). I'll add them along with the review comments once they come in.

Hi Satish,

I can't do normative reviews yet, so here's some drive-by comments:

39 class WebSpeechInputClient {
 40 public:
 41     // Attaches a listener which receives future callbacks.
 42     // Returns false if a listener was already attached.
 43     virtual bool attachListener(WebSpeechInputClientListener* listener) = 0;
 I think the style is now to avoid pure virtuals and have:
 WEBKIT_ASSERT_NOT_REACHED();
 see: http://trac.webkit.org/browser/trunk/WebKit/chromium/public/WebIDBCallbacks.h
Also, no need to name the parameter when it's obvious from the type (please, check all other headers).

  39 class WebSpeechInputClientListener {
As above.

47 class SpeechInputClientImpl
both file / classname should be prefixed with "Web", since they're in WebKit namespace.

43 SpeechInputClientImpl::SpeechInputClientImpl(WebSpeechInputClient* client)
use initializer for m_client, and also zero-initialize m_listener!


64     WebCore::SpeechInputClientListener* m_listener; // Valid when recognition is in progress.
This should probably be a WebPrivatePtr:
http://trac.webkit.org/browser/trunk/WebKit/chromium/public/WebSerializedScriptValue.h

 71 void SpeechInputClientImpl::recordingComplete()
 72 {
 73     ASSERT(m_listener);
 74     if (m_listener)
no need for the if since the assert will terminate.

 78 void SpeechInputClientImpl::setRecognitionResult(const WebString& result)
 79 {
 80     ASSERT(m_listener);
 81     if (m_listener)   
as above.

67 } // namespace WebCore
 68 
 69 #endif // ENABLE(INPUT_SPEECH)
this is closing WebKit, not WebCore namespace.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.