Bug 76443 – Speech Input: Report speech element rect relative to window rather than frame

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

Speech Input: Report speech element rect relative to window rather than frame

[Hans Wennborg]

Created attachment 122745 [details]
Patch

[Hans Wennborg]

(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?

[Satish Sampath]

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.

[Hans Wennborg]

Created attachment 122927 [details]
Patch

[Hans Wennborg]

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.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Gyuyoung Kim]

Comment on attachment 122927 [details]
Patch

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

[Satish Sampath]

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?

[Philippe Normand]

Comment on attachment 122927 [details]
Patch

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

[Darin Fisher (:fishd, Google)]

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?

[Hans Wennborg]

Created attachment 123136 [details]
Patch

Try to green the EWS bots.

[Hans Wennborg]

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.

[Hans Wennborg]

Created attachment 123311 [details]
Patch

[Early Warning System Bot]

Comment on attachment 123311 [details]
Patch

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

[Hans Wennborg]

Created attachment 123313 [details]
Patch

Try to green the EWS bots.

[Collabora GTK+ EWS bot]

Comment on attachment 123313 [details]
Patch

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

[Hans Wennborg]

Created attachment 123339 [details]
Patch

Try to green the EWS bots, take two.

[Satish Sampath]

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.

[Hans Wennborg]

(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.

[Hans Wennborg]

Created attachment 123559 [details]
Patch

[Satish Sampath]

Looks good to me

[Darin Fisher (:fishd, Google)]

(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?

[Hans Wennborg]

Created attachment 124716 [details]
Patch

Uploading new and simpler patch now that the mock has moved into Chromium's DRT.

[WebKit Review Bot]

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.

[Hans Wennborg]

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