[Webkit-unassigned] [Bug 41518] Speech input plumbing in webcore and webkit

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jul 6 07:15:11 PDT 2010


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


Steve Block <steveblock at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #60359|review?                     |review-
               Flag|                            |




--- Comment #4 from Steve Block <steveblock at google.com>  2010-07-06 07:15:11 PST ---
(From update of attachment 60359)
LayoutTests/fast/forms/input-text-speechbutton.html:22
 +              layoutTestController.notifyDone();
if (window.layoutTestController)

LayoutTests/fast/forms/input-text-speechbutton.html:1
 +  <html>
I think we should be using script-tests for these kind of tests.

WebCore/Android.mk:383
 +  ifeq ($(ENABLE_INPUT_SPEECH), true)
No need to guard this. Contents of file should be guarded.

WebCore/html/HTMLInputElement.cpp:2777
 +          return hasAttribute(speechAttr) && document()->page()->isSpeechInputSupported();
Do we need this extra check? Surely any platform that sets ENABLE(INPUT_SPEECH) will provide a speech client, and this entire method is guarded by ENABLE(INPUT_SPEECH).

WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:170
 +  #if ENABLE(INPUT_SPEECH)
I think this shouldn't be guarded

WebKitTools/ChangeLog:13
 +          * DumpRenderTree/chromium/LayoutTestController.h:
I think that all new LayoutTestController methods need to be implemented for Mac.

WebKit/chromium/ChangeLog:5
 +          Speech input plumbing in webcore and webkit
Can we leave the WebKit/chromium changes for a separate patch to keep this one simpler?

WebCore/page/SpeechInput.h:45
 +  class SpeechInputElementListener {
Shouldn't this be named SpeechInputListener, as it listens to SpeechInput?

WebCore/page/SpeechInputClient.h:39
 +  class SpeechInputListener {
Shouldn't this be named SpeechInputClientListener, as it listens to SpeechInputClient?

WebCore/page/SpeechInputClient.h:42
 +      virtual void recognitionResult(const String& result) = 0;
setRecognitionResult()?

WebCore/page/Page.h:155
 +          void setSpeechInputClient(SpeechInputClient* client) { m_speechInputClient = client; }
Is there any reason for the client to be reset, other than for testing purposes? If not, I don't think we need this method. The embedder can configure their SpeechInputClient to be mocked out without having to call into WebCore.

WebCore/page/Page.cpp:133
 +      , m_speechInputClient(0)
For other services (eg Geolocation, DeviceOrientation), the Page owns a FooController object, which holds a weak reference to the client, which is owned by the embedder. The controller takes care of multiplexing multiple requests from the page to the client. Here the Page holds a reference to the client directly. I see that you have a separate SpeechInputElement object for each input. Is there no need for a controller type object? Even if not, it might be best to add a SpeechInputController just to keep the pattern the same. It can have a single createSpeechElement(SpeechInputElementListener*) method.

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



More information about the webkit-unassigned mailing list