Allow direct and indirect navigation values to be varied via field trial.

Allow direct and indirect navigation values to be varied via field trial.

This CL allows the engagement scores assigned to direct and indirect
navigations to be varied via a field trial. It also makes method names
related to the setting of values in AppBannerSettingsHelper more
consistent.

BUG=487519
Committed: https://crrev.com/633bbe04d2525d7272a78d78f8e3fdd0abe4d5a2
Cr-Commit-Position: refs/heads/master@{#345729}

[dominickn, 2015-08-25 07:17:26]

dominickn@chromium.org changed reviewers:
+ benwells@chromium.org

[dominickn, 2015-08-26 02:57:19]

PTAL, and advise whether this is too complex to be merged. The CL size is a
little larger because I made method names consistent, which required a few
renames in Android code.

[dominickn, 2015-08-26 03:00:10]

Patchset #2 (id:20001) has been deleted

[benwells, 2015-08-26 06:50:06]

lgtm, but some tiny things you can take or leave.

https://codereview.chromium.org/1309803005/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/1309803005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:477: void
AppBannerSettingsHelper::UpdateEngagementWeights() {
UpdateEngagementWeights and UpdateMinutesBetweenVisits don't need to be exposed.
They could be in the anon namespace.

https://codereview.chromium.org/1309803005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:481:
base::FieldTrialList::FindFullName("AppBannersEngagementWeights");
Nit: do you think it is worthwhile adding something like:

if (weights.empty())
  return;

?

(and in UpdateMinutesBetweenVisits).

https://codereview.chromium.org/1309803005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:482:
std::vector<base::StringPiece> tokens = base::SplitStringPiece(
Could you just use SplitString? Since you are calling as_string below I'm not
sure if there is an advantage to using StringPiece (correct me if I'm wrong).

[benwells, 2015-08-26 06:50:29]

On 2015/08/26 06:50:06, benwells wrote:
> lgtm, but some tiny things you can take or leave.
> 
>
https://codereview.chromium.org/1309803005/diff/40001/chrome/browser/banners/...
> File chrome/browser/banners/app_banner_settings_helper.cc (right):
> 
>
https://codereview.chromium.org/1309803005/diff/40001/chrome/browser/banners/...
> chrome/browser/banners/app_banner_settings_helper.cc:477: void
> AppBannerSettingsHelper::UpdateEngagementWeights() {
> UpdateEngagementWeights and UpdateMinutesBetweenVisits don't need to be
exposed.
> They could be in the anon namespace.
> 
>
https://codereview.chromium.org/1309803005/diff/40001/chrome/browser/banners/...
> chrome/browser/banners/app_banner_settings_helper.cc:481:
> base::FieldTrialList::FindFullName("AppBannersEngagementWeights");
> Nit: do you think it is worthwhile adding something like:
> 
> if (weights.empty())
>   return;
> 
> ?
> 
> (and in UpdateMinutesBetweenVisits).
> 
>
https://codereview.chromium.org/1309803005/diff/40001/chrome/browser/banners/...
> chrome/browser/banners/app_banner_settings_helper.cc:482:
> std::vector<base::StringPiece> tokens = base::SplitStringPiece(
> Could you just use SplitString? Since you are calling as_string below I'm not
> sure if there is an advantage to using StringPiece (correct me if I'm wrong).

Also as this is just to M46, this should be fine to merge as is.

[dominickn, 2015-08-26 07:01:49]

dominickn@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[dominickn, 2015-08-26 07:04:23]

@benwells: Thanks, nits addressed. : )

@dfalcantara: please review Android code. This will be merged to M46, along with
https://codereview.chromium.org/1306693005/.

https://codereview.chromium.org/1309803005/diff/40001/chrome/browser/banners/...
File chrome/browser/banners/app_banner_settings_helper.cc (right):

https://codereview.chromium.org/1309803005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:477: void
AppBannerSettingsHelper::UpdateEngagementWeights() {
On 2015/08/26 06:50:06, benwells wrote:
> UpdateEngagementWeights and UpdateMinutesBetweenVisits don't need to be
exposed.
> They could be in the anon namespace.

Done.

https://codereview.chromium.org/1309803005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:481:
base::FieldTrialList::FindFullName("AppBannersEngagementWeights");
On 2015/08/26 06:50:06, benwells wrote:
> Nit: do you think it is worthwhile adding something like:
> 
> if (weights.empty())
>   return;
> 
> ?
> 
> (and in UpdateMinutesBetweenVisits).

Done.

https://codereview.chromium.org/1309803005/diff/40001/chrome/browser/banners/...
chrome/browser/banners/app_banner_settings_helper.cc:482:
std::vector<base::StringPiece> tokens = base::SplitStringPiece(
On 2015/08/26 06:50:06, benwells wrote:
> Could you just use SplitString? Since you are calling as_string below I'm not
> sure if there is an advantage to using StringPiece (correct me if I'm wrong).

I believe there's a memory advantage to SplitStringPiece, but that may be
nullified by as_string(), so done.

[gone, 2015-08-26 17:12:36]

lgtm

[dominickn, 2015-08-26 23:56:45]

The CQ bit was checked by dominickn@chromium.org

[dominickn, 2015-08-26 23:56:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/1309803005/#ps60001
(title: "Addressing reviewer feedback")

[commit-bot: I haz the power, 2015-08-26 23:58:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1309803005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1309803005/60001

[commit-bot: I haz the power, 2015-08-27 00:04:54]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-08-27 00:05:37]

Patchset 3 (id:??) landed as
https://crrev.com/633bbe04d2525d7272a78d78f8e3fdd0abe4d5a2
Cr-Commit-Position: refs/heads/master@{#345729}

[Nico, 2015-08-27 04:23:36]

A revert of this CL (patchset #3 id:60001) has been created in
https://codereview.chromium.org/1322503002/ by thakis@chromium.org.

The reason for reverting is: Looks like this broke
org.chromium.chrome.browser.banners.AppBannerManagerTest :
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/21895.

[dominickn, 2015-08-27 04:57:11]

On 2015/08/27 04:23:36, Nico wrote:
> A revert of this CL (patchset #3 id:60001) has been created in
> https://codereview.chromium.org/1322503002/ by mailto:thakis@chromium.org.
> 
> The reason for reverting is: Looks like this broke
> org.chromium.chrome.browser.banners.AppBannerManagerTest :
>
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...
>
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/21895.

Reopening. Irritatingly, the trybots didn't catch a fairly glaring test error on
Android (running tests locally is broken for me at the moment).

@dfalcantara, PTAL before I send it back to the CQ?

[dominickn, 2015-08-27 06:27:08]

On 2015/08/27 04:57:11, dominickn wrote:
> On 2015/08/27 04:23:36, Nico wrote:
> > A revert of this CL (patchset #3 id:60001) has been created in
> > https://codereview.chromium.org/1322503002/ by mailto:thakis@chromium.org.
> > 
> > The reason for reverting is: Looks like this broke
> > org.chromium.chrome.browser.banners.AppBannerManagerTest :
> >
>
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...
> >
>
http://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/21895.
> 
> Reopening. Irritatingly, the trybots didn't catch a fairly glaring test error
on
> Android (running tests locally is broken for me at the moment).
> 
> @dfalcantara, PTAL before I send it back to the CQ?

Re-closing: it's the other CL I landed that caused the issue.