Use whether a query is from the app list to determine the RLZ parameter.

Use whether a query is from the app list to determine the RLZ parameter.

Currently, searches using the app launcher search box use the RLZ access
point for the omnibox. With this change, the app launcher RLZ access
point is used to construct url parameters for searches from the app
launcher.

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

[Sam McNally, 2014-04-08 08:49:00]

pkasting: Please review chrome/browser/{autocomplete,search_engines}
koz: Please review chrome/browser/ui/app_list

[Peter Kasting, 2014-04-08 20:33:13]

Be consistent about names and comments using "app list" versus "app launcher". 
Personally, I think "app launcher" is clearer.

https://codereview.chromium.org/228013002/diff/70001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/228013002/diff/70001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.cc:406:
zero_suggest_provider_->SetInAppList();
Is the app list really going to support zerosuggest?  It seems like if not, then
not only can you remove that provider from the list, but instead of adding a
setter to AutocompleteController, OmniboxProvider() can just call
controller_->serach_provider()->...

https://codereview.chromium.org/228013002/diff/70001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/228013002/diff/70001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:909: void
BaseSearchProvider::SetInAppList() { in_app_list_ = true; }
Style: trivial setters should be inlined into the declaration and named
unix_hacker()-style.

https://codereview.chromium.org/228013002/diff/70001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/228013002/diff/70001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_unittest.cc:692: #elif
defined(OS_ANDROID)
Is this test even useful on Android?

[Sam McNally, 2014-04-09 01:48:56]

I've changed it to all use app list instead of app launcher to be consistent
with the existing code.

https://codereview.chromium.org/228013002/diff/70001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/228013002/diff/70001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.cc:406:
zero_suggest_provider_->SetInAppList();
On 2014/04/08 20:33:14, Peter Kasting wrote:
> Is the app list really going to support zerosuggest?  It seems like if not,
then
> not only can you remove that provider from the list, but instead of adding a
> setter to AutocompleteController, OmniboxProvider() can just call
> controller_->serach_provider()->...

Done.

https://codereview.chromium.org/228013002/diff/70001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/228013002/diff/70001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:909: void
BaseSearchProvider::SetInAppList() { in_app_list_ = true; }
On 2014/04/08 20:33:14, Peter Kasting wrote:
> Style: trivial setters should be inlined into the declaration and named
> unix_hacker()-style.

Done.

https://codereview.chromium.org/228013002/diff/70001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url_unittest.cc (right):

https://codereview.chromium.org/228013002/diff/70001/chrome/browser/search_en...
chrome/browser/search_engines/template_url_unittest.cc:692: #elif
defined(OS_ANDROID)
On 2014/04/08 20:33:14, Peter Kasting wrote:
> Is this test even useful on Android?

Done.

[Peter Kasting, 2014-04-09 02:12:32]

LGTM, but here's a possible consideration.

What if instead of passing a bool around everywhere, you actually passed around
the RLZ access point value?  Not only would this be slightly simpler, it would
be more self-documenting -- the type and the value names both would be more
obvious.

And if we're going to store the knowledge of "in app list" for the purposes of
RLZ origins in a few more classes anyway, I don't see any real layering problems
in making those classes refer to RLZ access points directly.

Not required, but give it some thought.

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.h (right):

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:76: // Sets this as being in
the app list.
Nit: No need for a comment here as long as the one on the member below is clear.

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:488: // True if this is in
the app list. Otherwise, this is in the omnibox.
Nit: Maybe:

True if this provider's results are being displayed in the app list.  By default
this is false, meaning that the results will be shown in the omnibox.

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.cc:240: virtual base::string16
GetRlzParameterValue(bool from_app_list) const
Nit: Wrap before param instead of between const and OVERRIDE

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/omnibox_provider.cc (right):

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/omnibox_provider.cc:138:
AutocompleteClassifier::kDefaultOmniboxProviders)) {
You should consider using a different provider set here.  You don't need
ZeroSuggest, and I would imagine there are probably other providers you don't
actually want either (though I'm not totally sure).

Depending on how different your set ends up being from the default set you could
either construct the appropriate value from scratch or just mask the default
value a bit.

[Sam McNally, 2014-04-09 05:33:43]

> What if instead of passing a bool around everywhere, you actually passed
around
> the RLZ access point value?  Not only would this be slightly simpler, it would
> be more self-documenting -- the type and the value names both would be more
> obvious.
> 
> And if we're going to store the knowledge of "in app list" for the purposes of
> RLZ origins in a few more classes anyway, I don't see any real layering
problems
> in making those classes refer to RLZ access points directly.
> 
> Not required, but give it some thought.

I think that approach sounds cleaner, but the RLZ access points are only defined
with ENABLE_RLZ defined.

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.h (right):

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:76: // Sets this as being in
the app list.
On 2014/04/09 02:12:33, Peter Kasting wrote:
> Nit: No need for a comment here as long as the one on the member below is
clear.

Done.

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:488: // True if this is in
the app list. Otherwise, this is in the omnibox.
On 2014/04/09 02:12:33, Peter Kasting wrote:
> Nit: Maybe:
> 
> True if this provider's results are being displayed in the app list.  By
default
> this is false, meaning that the results will be shown in the omnibox.

Done.

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/autocompl...
File chrome/browser/autocomplete/history_url_provider.cc (right):

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/autocompl...
chrome/browser/autocomplete/history_url_provider.cc:240: virtual base::string16
GetRlzParameterValue(bool from_app_list) const
On 2014/04/09 02:12:33, Peter Kasting wrote:
> Nit: Wrap before param instead of between const and OVERRIDE

Done.

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/search/omnibox_provider.cc (right):

https://codereview.chromium.org/228013002/diff/90001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/search/omnibox_provider.cc:138:
AutocompleteClassifier::kDefaultOmniboxProviders)) {
On 2014/04/09 02:12:33, Peter Kasting wrote:
> You should consider using a different provider set here.  You don't need
> ZeroSuggest, and I would imagine there are probably other providers you don't
> actually want either (though I'm not totally sure).
> 
> Depending on how different your set ends up being from the default set you
could
> either construct the appropriate value from scratch or just mask the default
> value a bit.

Done. Other than zero suggest, the rest of the defaults seem reasonable to me.

+xiyuan FYI.

[Peter Kasting, 2014-04-09 05:36:50]

On 2014/04/09 05:33:43, Sam McNally wrote:
> > What if instead of passing a bool around everywhere, you actually passed
> around
> > the RLZ access point value?  Not only would this be slightly simpler, it
would
> > be more self-documenting -- the type and the value names both would be more
> > obvious.
> > 
> > And if we're going to store the knowledge of "in app list" for the purposes
of
> > RLZ origins in a few more classes anyway, I don't see any real layering
> problems
> > in making those classes refer to RLZ access points directly.
> > 
> > Not required, but give it some thought.
> 
> I think that approach sounds cleaner, but the RLZ access points are only
defined
> with ENABLE_RLZ defined.

I would actually be OK with #ifdefing the relevant code.  But another
possibility which would probably be superior is to go ahead and move the access
point list outside the relevant #if.  We do this sort of thing in some other
similar places in Chrome.  Maybe this would also let you get rid of the "#if
!defined(OS_IOS)" in search_terms_data.cc?

[koz (OOO until 15th September), 2014-04-09 05:54:48]

c/b/u/app_list lgtm

[Sam McNally, 2014-04-09 09:13:49]

On 2014/04/09 05:36:50, Peter Kasting wrote:
> On 2014/04/09 05:33:43, Sam McNally wrote:
> > > What if instead of passing a bool around everywhere, you actually passed
> > around
> > > the RLZ access point value?  Not only would this be slightly simpler, it
> would
> > > be more self-documenting -- the type and the value names both would be
more
> > > obvious.
> > > 
> > > And if we're going to store the knowledge of "in app list" for the
purposes
> of
> > > RLZ origins in a few more classes anyway, I don't see any real layering
> > problems
> > > in making those classes refer to RLZ access points directly.
> > > 
> > > Not required, but give it some thought.
> > 
> > I think that approach sounds cleaner, but the RLZ access points are only
> defined
> > with ENABLE_RLZ defined.
> 
> I would actually be OK with #ifdefing the relevant code.  But another
> possibility which would probably be superior is to go ahead and move the
access
> point list outside the relevant #if.  We do this sort of thing in some other
> similar places in Chrome.  Maybe this would also let you get rid of the "#if
> !defined(OS_IOS)" in search_terms_data.cc?

The IDs and their type are defined in the rlz library, which isn't fully
implemented on Linux. I'm going to stick with the current implementation in this
CL and change it in a follow up if I can get it to work in a cleaner way.

[Sam McNally, 2014-04-09 09:14:09]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-04-09 09:26:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/228013002/110001

[commit-bot: I haz the power, 2014-04-09 09:34:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/228013002/110001

[commit-bot: I haz the power, 2014-04-09 09:46:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/228013002/110001

[commit-bot: I haz the power, 2014-04-09 09:55:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/228013002/110001

[commit-bot: I haz the power, 2014-04-09 10:58:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-09 10:58:45]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[xiyuan, 2014-04-09 16:12:08]

https://codereview.chromium.org/228013002/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/omnibox_provider.cc (right):

https://codereview.chromium.org/228013002/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/omnibox_provider.cc:139:
AutocompleteClassifier::kDefaultOmniboxProviders |
Did you mean "& ~AutocompleteProvider::TYPE_ZERO_SUGGEST"?

[Sam McNally, 2014-04-09 23:38:52]

https://codereview.chromium.org/228013002/diff/110001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/search/omnibox_provider.cc (right):

https://codereview.chromium.org/228013002/diff/110001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/search/omnibox_provider.cc:139:
AutocompleteClassifier::kDefaultOmniboxProviders |
On 2014/04/09 16:12:08, xiyuan wrote:
> Did you mean "& ~AutocompleteProvider::TYPE_ZERO_SUGGEST"?

Yes.

[xiyuan, 2014-04-10 01:16:28]

lgtm

[Sam McNally, 2014-04-10 01:55:27]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-04-10 01:55:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sammc@chromium.org/228013002/130001

[commit-bot: I haz the power, 2014-04-10 04:42:13]

Change committed as 262910