Summary: | Add speech input controller mock in WebKit and a layout test. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Satish Sampath <satish> | ||||||||||||
Component: | Forms | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | hans, jorlow, webkit.review.bot | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 39485 | ||||||||||||||
Attachments: |
|
Description
Satish Sampath
2010-08-04 04:13:51 PDT
Created attachment 63439 [details]
Patch
Attachment 63439 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3624485 Created attachment 63441 [details]
Patch
Comment on attachment 63441 [details]
Patch
WebKitTools/DumpRenderTree/chromium/WebViewHost.cpp:504
+ WebKit::WebSpeechInputController* WebViewHost::speechInputController(WebKit::WebSpeechInputListener* listener)
No need for WEbKit::
WebKitTools/DumpRenderTree/chromium/LayoutTestController.h:46
+ #include "public/WebSpeechInputControllerMock.h"
Are you sure you need to do the include in this file rather than forward declare?
WebKit/chromium/public/WebSpeechInputControllerMock.h:45
+ virtual void setMockRecognitionResult(const WebString& result) = 0;
Why make this abstract/virtual?
WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1381
+ m_speechInputControllerMock->setMockRecognitionResult(cppVariantToWebString(arguments[0]));
Are you sure m_speechInputControllerMock will always be created in time? I'm not sure it will.
WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:44
+ class WebSpeechInputControllerMockImpl
no need to put on the same line
WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:40
+ #if ENABLE(INPUT_SPEECH)
move above the initial includes....actually delete the guards since it'll always be on for Chromium
WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:64
+ WebCore::SpeechInputClientMock m_webcoreMock;
Hmm....usually we use an OwnPtr, but I don't have a good reason why you'd need to.
WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:116
+ #if ENABLE(INPUT_SPEECH)
Just assume INPUT_SPEECH is always on for Chromium
WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:44
+ class WebSpeechInputControllerMockImpl
I'd slightly lean towards moving this impl stuff to a header file. It just seems weird to have an Impl.cpp whose corresponding .h is of a different name and I don't know of any other similar examples.
WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:113
+ // Factory method.
I'd put the factory more towards the top.
looks pretty good
Created attachment 63449 [details]
Patch
Addressed Jeremy's comments, more detailed reply below
(In reply to comment #4) Addressed all comments, some replies below: > (From update of attachment 63441 [details]) > WebKitTools/DumpRenderTree/chromium/LayoutTestController.h:46 > + #include "public/WebSpeechInputControllerMock.h" > Are you sure you need to do the include in this file rather than forward declare? Used forward declaration now, this required TestShell.cpp to also include the mock header file as it constructs the LayoutTestController object. > WebKit/chromium/public/WebSpeechInputControllerMock.h:45 > + virtual void setMockRecognitionResult(const WebString& result) = 0; > Why make this abstract/virtual? since it is an interface and the implementation lives in chromium/src/WebSpeechInputControllerMockImpl.h > WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:1381 > + m_speechInputControllerMock->setMockRecognitionResult(cppVariantToWebString(arguments[0])); > Are you sure m_speechInputControllerMock will always be created in time? I'm not sure it will. WebKit::WebViewImpl creates WebKit::SpeechInputClientImpl during construction, which calls the DRT WebViewClient for the speech input controller. The mock gets created in this call and returned to WebKit. So by the time the page loads, the mock is valid and available. > WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:64 > + WebCore::SpeechInputClientMock m_webcoreMock; > Hmm....usually we use an OwnPtr, but I don't have a good reason why you'd need to. Made to an OwnPtr now since the class declaration has been moved to impl.h. > WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:44 > + class WebSpeechInputControllerMockImpl > I'd slightly lean towards moving this impl stuff to a header file. It just seems weird to have an Impl.cpp whose corresponding .h is of a different name and I don't know of any other similar examples. Moved class declaration to impl.h Attachment 63449 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/chromium/src/WebSpeechInputControllerMockImpl.h:36: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 23 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 63450 [details]
Patch
Patch with style issues fixed
Comment on attachment 63450 [details]
Patch
WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:69
+ const WebCore::String& result)
put on one line
WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:52
+ void WebSpeechInputControllerMockImpl::setMockRecognitionResult(
ditto
WebKit/chromium/src/WebSpeechInputControllerMockImpl.cpp:39
+ // Factory method.
comment not needed
WebKit/chromium/public/WebSpeechInputControllerMock.h:43
+ static WebSpeechInputControllerMock* create(WebSpeechInputListener* listener);
Add virtual destructor.
r=me
Created attachment 63469 [details]
Patch
Addressed Jeremy's comments.
Comment on attachment 63469 [details]
Patch
rubber stamp
Comment on attachment 63469 [details] Patch Clearing flags on attachment: 63469 Committed r64749: <http://trac.webkit.org/changeset/64749> All reviewed patches have been landed. Closing bug. |