Bug 76443

Summary: Speech Input: Report speech element rect relative to window rather than frame
Product: WebKit Reporter: Hans Wennborg <hans>
Component: New BugsAssignee: Hans Wennborg <hans>
Status: RESOLVED FIXED    
Severity: Normal CC: fishd, gustavo.noronha, gustavo, pnormand, rakuco, satish, tkent, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 77641, 77083    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch fishd: review+

Description Hans Wennborg 2012-01-17 03:26:03 PST
Speech Input: Report speech element rect relative to window rather than frame
Comment 1 Hans Wennborg 2012-01-17 03:29:03 PST
Created attachment 122745 [details]
Patch
Comment 2 Hans Wennborg 2012-01-17 03:35:33 PST
(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?
Comment 3 Satish Sampath 2012-01-18 03:04:30 PST
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.
Comment 4 Hans Wennborg 2012-01-18 07:23:22 PST
Created attachment 122927 [details]
Patch
Comment 5 Hans Wennborg 2012-01-18 07:25:25 PST
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.
Comment 6 WebKit Review Bot 2012-01-18 07:26:51 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 7 Gyuyoung Kim 2012-01-18 07:30:10 PST
Comment on attachment 122927 [details]
Patch

Attachment 122927 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11150218
Comment 8 Satish Sampath 2012-01-18 09:01:57 PST
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 9 Philippe Normand 2012-01-18 10:59:11 PST
Comment on attachment 122927 [details]
Patch

Attachment 122927 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11194299
Comment 10 Darin Fisher (:fishd, Google) 2012-01-18 13:26:22 PST
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?
Comment 11 Hans Wennborg 2012-01-19 08:40:35 PST
Created attachment 123136 [details]
Patch

Try to green the EWS bots.
Comment 12 Hans Wennborg 2012-01-20 07:14:40 PST
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.
Comment 13 Hans Wennborg 2012-01-20 07:15:08 PST
Created attachment 123311 [details]
Patch
Comment 14 Early Warning System Bot 2012-01-20 07:24:31 PST
Comment on attachment 123311 [details]
Patch

Attachment 123311 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11314098
Comment 15 Hans Wennborg 2012-01-20 07:41:27 PST
Created attachment 123313 [details]
Patch

Try to green the EWS bots.
Comment 16 Collabora GTK+ EWS bot 2012-01-20 09:29:45 PST
Comment on attachment 123313 [details]
Patch

Attachment 123313 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11311125
Comment 17 Hans Wennborg 2012-01-20 09:42:08 PST
Created attachment 123339 [details]
Patch

Try to green the EWS bots, take two.
Comment 18 Satish Sampath 2012-01-23 06:52:45 PST
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.
Comment 19 Hans Wennborg 2012-01-23 08:22:26 PST
(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.
Comment 20 Hans Wennborg 2012-01-23 08:22:57 PST
Created attachment 123559 [details]
Patch
Comment 21 Satish Sampath 2012-01-23 08:30:33 PST
Looks good to me
Comment 22 Darin Fisher (:fishd, Google) 2012-01-23 10:22:52 PST
(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?
Comment 23 Hans Wennborg 2012-01-31 06:13:29 PST
Created attachment 124716 [details]
Patch

Uploading new and simpler patch now that the mock has moved into Chromium's DRT.
Comment 24 WebKit Review Bot 2012-01-31 06:17:12 PST
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.
Comment 25 Hans Wennborg 2012-02-01 00:52:50 PST
Committed r106438: <http://trac.webkit.org/changeset/106438>