Bug 51504

Summary: Make Speech menu items work in WebKit2
Product: WebKit Reporter: Darin Adler <darin>
Component: WebKit2Assignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, webkit-ews, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: All   
Attachments:
Description Flags
Patch andersca: review+

Description Darin Adler 2010-12-22 16:00:21 PST
Make Speech menu items work in WebKit2
Comment 1 Darin Adler 2010-12-22 16:13:03 PST
Created attachment 77272 [details]
Patch
Comment 2 Darin Adler 2010-12-22 16:14:42 PST
<rdar://problem/8246002>
Comment 3 Early Warning System Bot 2010-12-22 16:27:59 PST
Attachment 77272 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7287108
Comment 4 Anders Carlsson 2010-12-22 16:31:59 PST
Comment on attachment 77272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77272&action=review

> WebKit2/UIProcess/API/mac/WKView.mm:350
> +        return YES;

This should return "enabled".
Comment 5 Darin Adler 2010-12-22 16:45:19 PST
Committed r74521: <http://trac.webkit.org/changeset/74521>
Comment 6 Darin Adler 2010-12-22 16:50:29 PST
Committed r74522: <http://trac.webkit.org/changeset/74522>
Comment 7 WebKit Review Bot 2010-12-22 17:19:21 PST
http://trac.webkit.org/changeset/74521 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
http/tests/local/stylesheet-and-script-load-order-media-print.html
Comment 8 Maciej Stachowiak 2010-12-22 20:25:18 PST
Comment on attachment 77272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77272&action=review

> WebKit2/UIProcess/WebPageProxy.messages.in:119
> +    # Keyboard support messages.

This and other changes seem to add periods to the end of sentence fragments, which seems wrong.
Comment 9 Maciej Stachowiak 2010-12-22 20:27:48 PST
Comment on attachment 77272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77272&action=review

> WebKit2/WebProcess/WebPage/WebPage.cpp:1377
> +bool WebPage::isSpeaking()
> +{
> +    bool result;
> +    return sendSync(Messages::WebPageProxy::GetIsSpeaking(), Messages::WebPageProxy::GetIsSpeaking::Reply(result)) && result;
> +}

It seems suboptimal to do synchronous messaging to the UI process just to validate this one context menu item. Is there a way we can have the validation happen in the UI process in the first place? That seems like the right way to do it, since that's where the action will happen. In fact, the popup menu runs in the UI process, it would be better if WebKit-provided items could be handled in the UI process when appropriate.
Comment 10 Darin Adler 2010-12-23 08:28:21 PST
Comment on attachment 77272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77272&action=review

>> WebKit2/UIProcess/WebPageProxy.messages.in:119
>> +    # Keyboard support messages.
> 
> This and other changes seem to add periods to the end of sentence fragments, which seems wrong.

These periods were on all the other sentence fragments in the file; I corrected the few that were different to be consistent. It’s OK if you want to be consistent the other way.

>> WebKit2/WebProcess/WebPage/WebPage.cpp:1377
>> +}
> 
> It seems suboptimal to do synchronous messaging to the UI process just to validate this one context menu item. Is there a way we can have the validation happen in the UI process in the first place? That seems like the right way to do it, since that's where the action will happen. In fact, the popup menu runs in the UI process, it would be better if WebKit-provided items could be handled in the UI process when appropriate.

Yes, that would be a nice refinement to make later. This works well, but what you describe would be even better.