Add RLZ Access points for the app launcher.

Add RLZ Access points for the app launcher.

Currently, searches using the app launcher search box use the RLZ access
point for the omnibox. We want to be able to distinguish between search
requests from these two sources. This change adds new RLZ access points
to be used for searches via the app launcher.

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

[Sam McNally, 2014-04-01 07:54:42]

rogerta: Please review rlz/* and chrome/browser/rlz/*

grt: Please review chrome/installer

[grt (UTC plus 2), 2014-04-01 13:38:28]

rubberstamp lgtm for chrome/installer.

[Roger Tawa OOO till Jul 10th, 2014-04-01 14:20:18]

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz.cc#ne...
chrome/browser/rlz/rlz.cc:192: rlz_lib::NO_ACCESS_POINT;
Hmm, this is brittle.  Makes the order of the access points in the |points|
array above (and |points| array in uninstall code) significant.

Also, some rlz APIs like rlz_lib::GetAccessPointRlz() don't handle
NO_ACCESS_POINT correctly on all platforms.  So you would need to audit all
rlz_lib calls to make sure they can safely handle NO_ACCESS_POINT (and add the
required unit tests).

It might be better to wrap the uses of RLZTracker::CHROME_APP_LIST in an #if
!defined(OS_IOS) anyway.

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz_unitt...
File chrome/browser/rlz/rlz_unittest.cc (right):

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz_unitt...
chrome/browser/rlz/rlz_unittest.cc:280: //  Y: the user performs a search using
the home page
I think we need a comment like:

Z: the user performs a search using the applist

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz_unitt...
chrome/browser/rlz/rlz_unittest.cc:341: #endif
I think we need tests specific to iOS.  This is important if the access point
constant is defined to "no access point".

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz_unitt...
chrome/browser/rlz/rlz_unittest.cc:555: TEST_F(RlzLibTest,
UsageBeforeDelayedInit) {
Please add a new test called ApplistUsageOnly.

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz_unitt...
chrome/browser/rlz/rlz_unittest.cc:669: TEST_F(RlzLibTest,
GetAccessPointRlzOnIoThread) {
Please add a new test ApplistUsageDoesNotSendPingWhenSendPingImmediately,
assuming that is the desired behaviour.

[Sam McNally, 2014-04-02 01:27:27]

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz.cc#ne...
chrome/browser/rlz/rlz.cc:192: rlz_lib::NO_ACCESS_POINT;
On 2014/04/01 14:20:18, Roger Tawa wrote:
> Hmm, this is brittle.  Makes the order of the access points in the |points|
> array above (and |points| array in uninstall code) significant.
> 
> Also, some rlz APIs like rlz_lib::GetAccessPointRlz() don't handle
> NO_ACCESS_POINT correctly on all platforms.  So you would need to audit all
> rlz_lib calls to make sure they can safely handle NO_ACCESS_POINT (and add the
> required unit tests).
> 
> It might be better to wrap the uses of RLZTracker::CHROME_APP_LIST in an #if
> !defined(OS_IOS) anyway.

Done.

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz_unitt...
File chrome/browser/rlz/rlz_unittest.cc (right):

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz_unitt...
chrome/browser/rlz/rlz_unittest.cc:280: //  Y: the user performs a search using
the home page
On 2014/04/01 14:20:18, Roger Tawa wrote:
> I think we need a comment like:
> 
> Z: the user performs a search using the applist

Done.

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz_unitt...
chrome/browser/rlz/rlz_unittest.cc:341: #endif
On 2014/04/01 14:20:18, Roger Tawa wrote:
> I think we need tests specific to iOS.  This is important if the access point
> constant is defined to "no access point".

Changed to use #ifdefs for iOS instead.

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz_unitt...
chrome/browser/rlz/rlz_unittest.cc:555: TEST_F(RlzLibTest,
UsageBeforeDelayedInit) {
On 2014/04/01 14:20:18, Roger Tawa wrote:
> Please add a new test called ApplistUsageOnly.

Done.

https://codereview.chromium.org/220193005/diff/1/chrome/browser/rlz/rlz_unitt...
chrome/browser/rlz/rlz_unittest.cc:669: TEST_F(RlzLibTest,
GetAccessPointRlzOnIoThread) {
On 2014/04/01 14:20:18, Roger Tawa wrote:
> Please add a new test ApplistUsageDoesNotSendPingWhenSendPingImmediately,
> assuming that is the desired behaviour.

Done.

[Roger Tawa OOO till Jul 10th, 2014-04-02 14:24:44]

lgtm

Thanks Sam for fixing up the tests.  In production code, who is going to call
RLZTracker::RecordAppListSearch() ?  Is it possible to setup an observer for
that event, like is done for omnibox and home page, so that all the code remains
in rlz.cc?

[Sam McNally, 2014-04-02 23:08:00]

On 2014/04/02 14:24:44, Roger Tawa wrote:
> lgtm
> 
> Thanks Sam for fixing up the tests.  In production code, who is going to call
> RLZTracker::RecordAppListSearch() ?  Is it possible to setup an observer for
> that event, like is done for omnibox and home page, so that all the code
remains
> in rlz.cc?

I'm not sure. I haven't looked into how the plumbing for that is going to work
yet.

[Sam McNally, 2014-04-02 23:11:59]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-04-02 23:12:55]

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

[commit-bot: I haz the power, 2014-04-03 07:29:39]

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

[commit-bot: I haz the power, 2014-04-03 07:29:39]

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

[Sam McNally, 2014-04-03 07:39:18]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-04-03 07:40:18]

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

[commit-bot: I haz the power, 2014-04-03 08:32:46]

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

[commit-bot: I haz the power, 2014-04-03 08:32:46]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Sam McNally, 2014-04-03 08:38:56]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-04-03 08:39:41]

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

[commit-bot: I haz the power, 2014-04-03 10:52:52]

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

[commit-bot: I haz the power, 2014-04-03 10:52:53]

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

[Sam McNally, 2014-04-03 22:26:53]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-04-03 22:27:33]

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

[commit-bot: I haz the power, 2014-04-03 22:29:51]

Change committed as 261550