Add flag for disabling minimum of ntp tile suggestions

Add flag for disabling minimum of ntp tile suggestions

This flag will cause the the client to use history-based suggestions for
NTP tiles even if there are less than 8 suggestions available.

This is particularily useful for Chrome Home and the condensed NTP where
only 4 tiles are used and which does not contain any suggestions until
there would be enough data for 8 suggestions.

BUG=709959
Review-Url: https://codereview.chromium.org/2858883002
Cr-Commit-Position: refs/heads/master@{#469620}
Committed: https://chromium.googlesource.com/chromium/src/+/122c6655af011e18e58f4384e327174dd6212874

[fhorschig, 2017-05-03 09:28:29]

fhorschig@chromium.org changed reviewers:
+ treib@chromium.org

[fhorschig, 2017-05-03 09:28:29]

Hi Marc,
would you please take a look at this new flag?

It will enable me to move the "use suggestions only,
if there are 8 or more" condition to the client
and finally disable it there.

[Marc Treib, 2017-05-03 09:39:48]

Any reason why this is a "raw" cmdline flag, rather than a base::Feature? Then
we could control it remotely via the variations server.

[fhorschig, 2017-05-03 10:31:30]

Thank you, it's a feature now.

(It was a switch because I haven't paid close attention.)

[Marc Treib, 2017-05-03 11:43:57]

https://codereview.chromium.org/2858883002/diff/20001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2858883002/diff/20001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:3097: "Request suggestions from history even
if there are only very few of them "
"from history" is IMO misleading; the suggestions come from history anyway.

https://codereview.chromium.org/2858883002/diff/20001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2858883002/diff/20001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.h:3359: // suggestions in NTP tiles.
Server-side suggestions. Client-side ones are used anyway, even if there are
few.

https://codereview.chromium.org/2858883002/diff/20001/components/suggestions/...
File components/suggestions/features.cc (right):

https://codereview.chromium.org/2858883002/diff/20001/components/suggestions/...
components/suggestions/features.cc:10: // only very few of them.
s/only very few/less than 8/ ? (That's really what it means, right?)

Also, should this comment be in the header?

https://codereview.chromium.org/2858883002/diff/20001/components/suggestions/...
components/suggestions/features.cc:12: "DisableSuggestionsMinimum",
base::FEATURE_DISABLED_BY_DEFAULT};
Hm, not a fan of a feature that starts with "Disable". I find it confusing to
figure out what the actual state is when the feature is disabled...

How about something like "SuggestionsNoMinimum"?

[fhorschig, 2017-05-03 14:19:43]

https://codereview.chromium.org/2858883002/diff/20001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.cc (right):

https://codereview.chromium.org/2858883002/diff/20001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.cc:3097: "Request suggestions from history even
if there are only very few of them "
On 2017/05/03 11:43:56, Marc Treib wrote:
> "from history" is IMO misleading; the suggestions come from history anyway.

Gone.

https://codereview.chromium.org/2858883002/diff/20001/chrome/browser/flag_des...
File chrome/browser/flag_descriptions.h (right):

https://codereview.chromium.org/2858883002/diff/20001/chrome/browser/flag_des...
chrome/browser/flag_descriptions.h:3359: // suggestions in NTP tiles.
On 2017/05/03 11:43:56, Marc Treib wrote:
> Server-side suggestions. Client-side ones are used anyway, even if there are
> few.

Done.

https://codereview.chromium.org/2858883002/diff/20001/components/suggestions/...
File components/suggestions/features.cc (right):

https://codereview.chromium.org/2858883002/diff/20001/components/suggestions/...
components/suggestions/features.cc:10: // only very few of them.
On 2017/05/03 11:43:56, Marc Treib wrote:
> s/only very few/less than 8/ ? (That's really what it means, right?)
Hmm, actually the server-side config differentiates between mobile 
with 8 suggestions and desktop with 4. 
Currently, we would only see the effect for mobile but technically
there is a difference.

> Also, should this comment be in the header?

Moved. Although I am not sure. 

(We do this for ntp_{tiles,snippets} but large lists don't:
content/public/common/content_features.cc
https://cs.chromium.org/search/?q=f:%5C.cc+%5Cs*const%5Cs*base::Feature
Is there a standard? Is it important enough to think about deeply?)

https://codereview.chromium.org/2858883002/diff/20001/components/suggestions/...
components/suggestions/features.cc:12: "DisableSuggestionsMinimum",
base::FEATURE_DISABLED_BY_DEFAULT};
On 2017/05/03 11:43:56, Marc Treib wrote:
> Hm, not a fan of a feature that starts with "Disable". I find it confusing to
> figure out what the actual state is when the feature is disabled...
> 
> How about something like "SuggestionsNoMinimum"?

Renamed.

[Marc Treib, 2017-05-03 14:28:42]

lgtm

https://codereview.chromium.org/2858883002/diff/20001/components/suggestions/...
File components/suggestions/features.cc (right):

https://codereview.chromium.org/2858883002/diff/20001/components/suggestions/...
components/suggestions/features.cc:10: // only very few of them.
On 2017/05/03 14:19:42, fhorschig wrote:
> On 2017/05/03 11:43:56, Marc Treib wrote:
> > s/only very few/less than 8/ ? (That's really what it means, right?)
> Hmm, actually the server-side config differentiates between mobile 
> with 8 suggestions and desktop with 4. 
> Currently, we would only see the effect for mobile but technically
> there is a difference.

...wait, really? If anything, shouldn't it be the other way around?

> > Also, should this comment be in the header?
> 
> Moved. Although I am not sure. 
> 
> (We do this for ntp_{tiles,snippets} but large lists don't:
> content/public/common/content_features.cc
> https://cs.chromium.org/search/?q=f:%5C.cc+%5Cs*const%5Cs*base::Feature
> Is there a standard? Is it important enough to think about deeply?)

Personally, I'm more likely to look in the header for a description, but I don't
know if there's a standard. Nope, not important enough to worry over :)

https://codereview.chromium.org/2858883002/diff/40001/components/suggestions/...
File components/suggestions/features.h (right):

https://codereview.chromium.org/2858883002/diff/40001/components/suggestions/...
components/suggestions/features.h:14: extern const base::Feature
kSuggestionsWithoutMinimumFeature;
Now it kinda sounds like each suggestions could have some sort of "minimum"...
as is well known, naming things is one of the hard problems in computer science
(the others being caching, and off-by-one errors).
We could go explicit: kUseSuggestionsEvenIfFewFeature? Or you can just leave it
as is; I'll leave the decision to you.

[fhorschig, 2017-05-04 07:39:59]

https://codereview.chromium.org/2858883002/diff/20001/components/suggestions/...
File components/suggestions/features.cc (right):

https://codereview.chromium.org/2858883002/diff/20001/components/suggestions/...
components/suggestions/features.cc:10: // only very few of them.
On 2017/05/03 14:28:41, Marc Treib wrote:
> On 2017/05/03 14:19:42, fhorschig wrote:
> > On 2017/05/03 11:43:56, Marc Treib wrote:
> > > s/only very few/less than 8/ ? (That's really what it means, right?)
> > Hmm, actually the server-side config differentiates between mobile 
> > with 8 suggestions and desktop with 4. 
> > Currently, we would only see the effect for mobile but technically
> > there is a difference.
> 
> ...wait, really? If anything, shouldn't it be the other way around?

As discussed, this is the default config that might be overridden.

> 
> > > Also, should this comment be in the header?
> > 
> > Moved. Although I am not sure. 
> > 
> > (We do this for ntp_{tiles,snippets} but large lists don't:
> > content/public/common/content_features.cc
> > https://cs.chromium.org/search/?q=f:%5C.cc+%5Cs*const%5Cs*base::Feature
> > Is there a standard? Is it important enough to think about deeply?)
> 
> Personally, I'm more likely to look in the header for a description, but I
don't
> know if there's a standard. Nope, not important enough to worry over :)

Thanks!

https://codereview.chromium.org/2858883002/diff/40001/components/suggestions/...
File components/suggestions/features.h (right):

https://codereview.chromium.org/2858883002/diff/40001/components/suggestions/...
components/suggestions/features.h:14: extern const base::Feature
kSuggestionsWithoutMinimumFeature;
On 2017/05/03 14:28:42, Marc Treib wrote:
> Now it kinda sounds like each suggestions could have some sort of "minimum"...
> as is well known, naming things is one of the hard problems in computer
science
> (the others being caching, and off-by-one errors).
> We could go explicit: kUseSuggestionsEvenIfFewFeature? Or you can just leave
it
> as is; I'll leave the decision to you.

It's kUseSuggestionsEvenIfFewFeature now.

[fhorschig, 2017-05-04 07:44:13]

fhorschig@chromium.org changed reviewers:
+ jwd@chromium.org

[fhorschig, 2017-05-04 07:44:14]

Hi Jesse,
would you please take a look at the histograms.xml?

[jwd, 2017-05-04 19:51:39]

lgtm

[fhorschig, 2017-05-05 07:32:28]

The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 07:32:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 07:35:53]

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

[commit-bot: I haz the power, 2017-05-05 07:35:54]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[fhorschig, 2017-05-05 08:27:21]

The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 08:27:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 09:21:29]

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

[commit-bot: I haz the power, 2017-05-05 09:21:30]

Dry run: This issue passed the CQ dry run.

[fhorschig, 2017-05-05 09:29:16]

The CQ bit was checked by fhorschig@chromium.org

[fhorschig, 2017-05-05 09:29:16]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
jwd@chromium.org
Link to the patchset: https://codereview.chromium.org/2858883002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-05 09:29:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 10:21:34]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493976556345140,
"parent_rev": "9db74aa87f97c709baf5d38b82fb987713a40951", "commit_rev":
"122c6655af011e18e58f4384e327174dd6212874"}

[commit-bot: I haz the power, 2017-05-05 10:21:49]

Description was changed from

==========
Add flag for disabling minimum of ntp tile suggestions

This flag will cause the the client to use history-based suggestions for
NTP tiles even if there are less than 8 suggestions available.

This is particularily useful for Chrome Home and the condensed NTP where
only 4 tiles are used and which does not contain any suggestions until
there would be enough data for 8 suggestions.

BUG=709959
==========

to

==========
Add flag for disabling minimum of ntp tile suggestions

This flag will cause the the client to use history-based suggestions for
NTP tiles even if there are less than 8 suggestions available.

This is particularily useful for Chrome Home and the condensed NTP where
only 4 tiles are used and which does not contain any suggestions until
there would be enough data for 8 suggestions.

BUG=709959

Review-Url: https://codereview.chromium.org/2858883002
Cr-Commit-Position: refs/heads/master@{#469620}
Committed:
https://chromium.googlesource.com/chromium/src/+/122c6655af011e18e58f4384e327...
==========

[commit-bot: I haz the power, 2017-05-05 10:21:50]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/122c6655af011e18e58f4384e327...