Add setVoiceSearchSupported to the searchbox API. This will be used to determine whether to show a …

Add setVoiceSearchSupported to the searchbox API. This will be used to determine whether to show a microphone icon in the Omnibox.

BUG=251384
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207746

[jeremycho, 2013-06-18 23:11:40]

palmer for security review
samarth for everything else
pkasting mainly for FYI (as he'll be implementing the icon)

Thanks!

[Peter Kasting, 2013-06-18 23:45:42]

I'll need to know answers to a couple of questions:

* Will this always be called while the active navigation entry is a committed
search result page?  Or can this be called after we navigate away (due to, say,
IPC delays) or when another navigation has become pending?  Seems like at least
the former issue, if present, should be addressed in this CL by throwing away
the message if the current page doesn't match.  In general, I'm not sure whether
data is supposed to be stored on a WebContents, a NavigationEntry, something
else...

* Does receipt of this message mean we should always show a microphone icon, or
do we need to separately check that e.g. search term replacement is in progress?

[David Black, 2013-06-18 23:53:46]

> * Will this always be called while the active navigation entry is a committed
> search result page?  Or can this be called after we navigate away (due to,
say,
> IPC delays) or when another navigation has become pending?  Seems like at
least
> the former issue, if present, should be addressed in this CL by throwing away
> the message if the current page doesn't match.  In general, I'm not sure
whether
> data is supposed to be stored on a WebContents, a NavigationEntry, something
> else...

No, IPC delays are indeed an annoying thing and thus this message could in
theory come across the renderer->browser boundary at any time.  However, I
*think* the check you're asking for is already there in the
if (!contents()->IsActiveEntry(page_id))
line in instant_page.cc.

> * Does receipt of this message mean we should always show a microphone icon,
or
> do we need to separately check that e.g. search term replacement is in
progress?

We should also check separately for search term replacement.

[Peter Kasting, 2013-06-19 00:02:50]

On 2013/06/18 23:53:46, David Black wrote:
> I
> *think* the check you're asking for is already there in the
> if (!contents()->IsActiveEntry(page_id))
> line in instant_page.cc.

Ah, awesome, I suspect you're correct that that eliminates the bad cases.

There is one potential problem with that, though.  Say I start a pending
navigation, then the "voice search supported" IPC call comes through and this
code throws it away.  Then I cancel the pending navigation.  (Might also need to
hit "back" now to get "back to the search page" after creis' change to make
pending navigations act a bit more like committed ones.)  At this point I'm
sitting on a search page that doesn't have a microphone.

It seems kind of like the handler for this should always mark some relevant data
structure as supporting voice search, but only result in a call to update the
visible UI if that data structure corresponds to the active entry.

[samarth, 2013-06-19 00:38:15]

There's a worse bug in the current version:
1) Start loading search page in tab A
2) Switch to tab B
3) IPC for voice search support arrives

As this CL stands, we won't be listening to tab A and drop this IPC. You need to
do something similar to what we have for the bookmarks bar API and add the
listeners to SearchTabHelper. A plus is that this automatically gives you
per-tab state for the voice search support.

Samarth

[jeremycho, 2013-06-19 02:24:00]

On 2013/06/19 00:38:15, samarth wrote:
> There's a worse bug in the current version:
> 1) Start loading search page in tab A
> 2) Switch to tab B
> 3) IPC for voice search support arrives
> 
> As this CL stands, we won't be listening to tab A and drop this IPC. You need
to
> do something similar to what we have for the bookmarks bar API and add the
> listeners to SearchTabHelper. A plus is that this automatically gives you
> per-tab state for the voice search support.
> 
> Samarth

Done.  This CL now implements some of the model change logic of
https://codereview.chromium.org/16035020/.  PTAL.

[samarth, 2013-06-19 05:39:20]

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1: // Copyright 2012 The Chromium
Authors. All rights reserved.
Why any changes to this file at all?

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_page.cc (right):

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_page.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Why do you need any changes in this file at all?  (looks like you're getting
some of Kausalya's changes)

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:93:
last_known_most_visited_items_.clear();
You should reset voice_search_supported_ here.

https://codereview.chromium.org/17132011/diff/11002/chrome/renderer/resources...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/17132011/diff/11002/chrome/renderer/resources...
chrome/renderer/resources/extensions/searchbox_api.js:185:
this.setVoiceSearchSupported = function(supported) {
nit: extra space

[jeremycho, 2013-06-19 07:16:48]

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1: // Copyright 2012 The Chromium
Authors. All rights reserved.
On 2013/06/19 05:39:20, samarth wrote:
> Why any changes to this file at all?

I figured this change would be needed eventually to update the microphone (same
for instant_page), however I'd be fine leaving this for the follow-up CL.

[samarth, 2013-06-19 16:40:23]

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1: // Copyright 2012 The Chromium
Authors. All rights reserved.
On 2013/06/19 07:16:48, jeremycho wrote:
> On 2013/06/19 05:39:20, samarth wrote:
> > Why any changes to this file at all?
> 
> I figured this change would be needed eventually to update the microphone
(same
> for instant_page), however I'd be fine leaving this for the follow-up CL.

No, by going through SearchTabHelper, now Peter's change only needs to listen to
SearchModel changes via the observer. So the changes to InstantPage and
InstantController are not necessary.

[jeremycho, 2013-06-19 20:11:33]

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1: // Copyright 2012 The Chromium
Authors. All rights reserved.
On 2013/06/19 16:40:23, samarth wrote:
> On 2013/06/19 07:16:48, jeremycho wrote:
> > On 2013/06/19 05:39:20, samarth wrote:
> > > Why any changes to this file at all?
> > 
> > I figured this change would be needed eventually to update the microphone
> (same
> > for instant_page), however I'd be fine leaving this for the follow-up CL.
> 
> No, by going through SearchTabHelper, now Peter's change only needs to listen
to
> SearchModel changes via the observer. So the changes to InstantPage and
> InstantController are not necessary.

I see.  Reverted changes.

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/17132011/diff/11002/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:93:
last_known_most_visited_items_.clear();
On 2013/06/19 05:39:20, samarth wrote:
> You should reset voice_search_supported_ here.

Done.

https://codereview.chromium.org/17132011/diff/11002/chrome/renderer/resources...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/17132011/diff/11002/chrome/renderer/resources...
chrome/renderer/resources/extensions/searchbox_api.js:185:
this.setVoiceSearchSupported = function(supported) {
On 2013/06/19 05:39:20, samarth wrote:
> nit: extra space

Done.

[palmer, 2013-06-19 20:31:04]

IPC security LGTM

[samarth, 2013-06-19 22:55:16]

One more comment below, but otherwise this looks good. Let me know once you've
synced past her change and I'll give this another whirl.

Thanks,
Samarth

https://codereview.chromium.org/17132011/diff/22001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/17132011/diff/22001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:94:
model_.SetVoiceSearchSupported(false);
Sorry, this is a bit more subtle. You should do this unless the navigation was
"in-page".  Kausalya's adding checks for that in
https://codereview.chromium.org/16035020/ so if you wait for her change to land
and sync past it, you can pick up that change.

[jeremycho, 2013-06-20 19:35:52]

Rebased and added test.  PTAL.

https://codereview.chromium.org/17132011/diff/22001/chrome/browser/ui/search/...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/17132011/diff/22001/chrome/browser/ui/search/...
chrome/browser/ui/search/search_tab_helper.cc:94:
model_.SetVoiceSearchSupported(false);
On 2013/06/19 22:55:16, samarth wrote:
> Sorry, this is a bit more subtle. You should do this unless the navigation was
> "in-page".  Kausalya's adding checks for that in
> https://codereview.chromium.org/16035020/ so if you wait for her change to
land
> and sync past it, you can pick up that change.

Done.

[samarth, 2013-06-20 21:01:25]

lgtm

[jeremycho, 2013-06-20 21:05:56]

Thanks for the review.  Peter - is this good on your end?

[Peter Kasting, 2013-06-20 22:35:09]

On 2013/06/20 21:05:56, jeremycho wrote:
> Thanks for the review.  Peter - is this good on your end?

I don't think you need my OK.  It's more that I need to know how to consume
this.  It sounds as if I just need to use a search model observer to watch this
value, which is a class I've never used but can find out about from Samarth as
need be.  That sounds easy enough.

[jeremycho, 2013-06-20 22:47:20]

On 2013/06/20 22:35:09, Peter Kasting wrote:
> On 2013/06/20 21:05:56, jeremycho wrote:
> > Thanks for the review.  Peter - is this good on your end?
> 
> I don't think you need my OK.  It's more that I need to know how to consume
> this.  It sounds as if I just need to use a search model observer to watch
this
> value, which is a class I've never used but can find out about from Samarth as
> need be.  That sounds easy enough.

Yeah, I have the OWNERs approvals, just wanted to see if this will fit your
purposes.  Thanks, I'll submit then.

[commit-bot: I haz the power, 2013-06-20 22:50:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremycho@chromium.org/17132011/33001

[commit-bot: I haz the power, 2013-06-21 07:16:12]

Change committed as 207746