Bug 76443

Created attachment 122745 [details] Patch

(This is for Chromium bug http://crbug.com/101742) This is a one-line fix, but I'm unsure of how to test it. The best way I can see would be to add a method to layout test controller like layoutTestController.dumpSpeechInputRequests() which would cause DumpRenderTree to print out the rect each time a request is made to the mock speech input controller. Not sure if it is worth the added complexity, though?

The change itself looks good. For testing, I can think of 2 more ways: - SpeechInputClientMock accepts a lang-result pair so you can set a different result for few languages when testing multi-language support. You could expand that to also include a rect so setting different results for expected rects and checking the result received could work - There could be a new boolean flag in SpeechInputClientMock settable by the test script and if that is set the element rect could be included as part of the result string given to the input element I don't have a strong preference for either of the 3, but would like whichever method used to be resilient to small changes in the positioning due to the other rendering logic changes.

Created attachment 122927 [details] Patch

Uploaded new patch that also has a test :) I've done sort of a combination of your suggestions, allowing an extra parameter to be passed in to addMockSpeechInputResult() on the LayoutTestController which will cause the controller to report back the element's position rather than a text value.

Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.

Comment on attachment 122927 [details] Patch Attachment 122927 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11150218

Comment on attachment 122927 [details] Patch The test looks great, but I'm thinking overloading addMockSpeechInputResult to add the dumpRect flag can be better. Right now the first 3 params are unused if the flag is set to true. Would it be simpler to add a new LayoutTestController::mockSpeechInputDumpRect() or something similar to set the flag to true?

Comment on attachment 122927 [details] Patch Attachment 122927 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11194299

Comment on attachment 122927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122927&action=review > Source/WebKit/chromium/public/WebSpeechInputControllerMock.h:47 > + virtual void addMockRecognitionResult(const WebString& result, double confidence, const WebString& language, bool dumpRect) = 0; nit: It would be helpful to explain what these parameters mean. As is, it requires someone to study the implementation to understand how to use this API. > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1932 > if (WebSpeechInputControllerMock* controller = m_shell->webViewHost()->speechInputControllerMock()) Would it be better to extend WebCore/testing/Internals.idl instead? Do we really need this "mock" to be exposed as a WebKit API like this?

Created attachment 123136 [details] Patch Try to green the EWS bots.

Thanks for the comments! New patch coming up. (In reply to comment #8) > Would it be simpler to add a new LayoutTestController::mockSpeechInputDumpRect() or something similar to set the flag to true? Yes, that's probably better. Doing that. (In reply to comment #10) > nit: It would be helpful to explain what these parameters mean. As is, it requires > someone to study the implementation to understand how to use this API. Done. > > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1932 > > if (WebSpeechInputControllerMock* controller = m_shell->webViewHost()->speechInputControllerMock()) > > Would it be better to extend WebCore/testing/Internals.idl instead? Do we really > need this "mock" to be exposed as a WebKit API like this? The reason we have a wrapper around the mock as part of the WebKit API is so we can pass it in instead of the "real" speech input controller. The way the controller is passed in is via the WebViewClient interface, which can only refer to types in the WebKit API. I don't see any way we could just pass in the WebCore mock and get rid of the wrapper. If I understand correctly, the Internals object is per-frame, whereas we want to use the same mock for a whole Page, so we couldn't have the Internals object own the mock.

Created attachment 123311 [details] Patch

Comment on attachment 123311 [details] Patch Attachment 123311 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11314098

Created attachment 123313 [details] Patch Try to green the EWS bots.

Comment on attachment 123313 [details] Patch Attachment 123313 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11311125

Created attachment 123339 [details] Patch Try to green the EWS bots, take two.

Comment on attachment 123339 [details] Patch WebCore/platform/mock/SpeechInputClientMock.cpp: > ts << rect.x() << ", " << rect.y(); For completeness perhaps also send width and height (since you call it 'dump rect')? (and without spaces so easier for script to check) WebKit/chromium/public/WebSpeechInputControllerMock.h: > // Add a result string with some confidence and language that will be returned for the next speech input request. The given result & confidence value are returned only if the start() call asked for the specified language. So the comment should probably be like "Add a result string and confidence value to be returned as result for the next speech input request in the given language." LayoutTests/fast/speech/bubble-position-expected.txt: > PASS input.value is "157, 111" We should remove this hard coded position string so that it doesn't require a rebaseline when subtle layout changes are made in future LayoutTests/fast/speech/bubble-position.html: > shouldBeEqualToString("input.value", expectedX + ", " + expectedY); For clarity declare expectedX and expectedY as top level vars? Right now they are undeclared and by default end up in global scope.

(In reply to comment #18) > (From update of attachment 123339 [details]) > WebCore/platform/mock/SpeechInputClientMock.cpp: > > ts << rect.x() << ", " << rect.y(); > For completeness perhaps also send width and height (since you call it 'dump rect')? (and without spaces so easier for script to check) Done. > WebKit/chromium/public/WebSpeechInputControllerMock.h: > > // Add a result string with some confidence and language that will be returned for the next speech input request. > The given result & confidence value are returned only if the start() call asked for the specified language. So the comment should probably be like "Add a result string and confidence value to be returned as result for the next speech input request in the given language." Done. I used your suggestion verbatim :) > LayoutTests/fast/speech/bubble-position-expected.txt: > > PASS input.value is "157, 111" > We should remove this hard coded position string so that it doesn't require a rebaseline when subtle layout changes are made in future Done. > LayoutTests/fast/speech/bubble-position.html: > > shouldBeEqualToString("input.value", expectedX + ", " + expectedY); > For clarity declare expectedX and expectedY as top level vars? Right now they are undeclared and by default end up in global scope. I've prefixed them with "window." to make it more clear that I want them in the global scope. I believe they have to be in that scope to be used in "shouldBe*" functions.

Created attachment 123559 [details] Patch

Looks good to me

(In reply to comment #12) ... > > > Tools/DumpRenderTree/chromium/LayoutTestController.cpp:1932 > > > if (WebSpeechInputControllerMock* controller = m_shell->webViewHost()->speechInputControllerMock()) > > > > Would it be better to extend WebCore/testing/Internals.idl instead? Do we really > > need this "mock" to be exposed as a WebKit API like this? > > The reason we have a wrapper around the mock as part of the WebKit API is so we can pass it in instead of the "real" speech input controller. The way the controller is passed in is via the WebViewClient interface, which can only refer to types in the WebKit API. Why does the Mock live at the WebKit layer. Shouldn't it just be part of DumpRenderTree? Why do other consumers of WebKit need to carry this testing code? > I don't see any way we could just pass in the WebCore mock and get rid of the wrapper. Why is there a mock in WebCore? If the mock needs to live in WebCore, then activating the mock via a method on Internals.idl seems to make more sense. It would allow you to have fewer layers of interfaces. > If I understand correctly, the Internals object is per-frame, whereas we want to use the same mock for a whole Page, so we couldn't have the Internals object own the mock. Couldn't you just interact with the Internals object on the main frame, and it could then attach the mock to the Page?

Created attachment 124716 [details] Patch Uploading new and simpler patch now that the mock has moved into Chromium's DRT.

Attachment 124716 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Committed r106438: <http://trac.webkit.org/changeset/106438>