Use server signal to decide whether to notify

Use server signal to decide whether to notify

BUG=675561
Review-Url: https://codereview.chromium.org/2618703004
Cr-Commit-Position: refs/heads/master@{#441944}
Committed: https://chromium.googlesource.com/chromium/src/+/1ed3d555ea644e4648f14fab71a67b45c30d0e15

[sfiera, 2017-01-05 15:17:26]

sfiera@chromium.org changed reviewers:
+ bauerb@chromium.org

[sfiera, 2017-01-05 15:17:26]

Now that the signal exists in ContentSuggestion.

Needs a helper function from https://crrev.com/2613863003; I'll rebase after
that lands.

Thanks!

[Bernhard Bauer, 2017-01-05 17:31:17]

https://codereview.chromium.org/2618703004/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/content_suggestions_notifier_service.cc (right):

https://codereview.chromium.org/2618703004/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/content_suggestions_notifier_service.cc:108: if
(false /* DO_NOT_SUBMIT: always_notify after crrev.com/2613863003 */) {
So, if the feature is enabled, we will always notify about the first suggestion,
but only for ARTICLES, never for any other category, whereas if the feature is
disabled, we will notify for any category that has the notification signal. Is
that intended behavior or should the conditions be jiggled around?

https://codereview.chromium.org/2618703004/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/content_suggestions_notifier_service.cc:110:
suggestions.size()) {
Maybe use !suggestions.empty()?

[sfiera, 2017-01-05 19:14:37]

https://codereview.chromium.org/2618703004/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/content_suggestions_notifier_service.cc (right):

https://codereview.chromium.org/2618703004/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/content_suggestions_notifier_service.cc:108: if
(false /* DO_NOT_SUBMIT: always_notify after crrev.com/2613863003 */) {
On 2017/01/05 17:31:17, Bernhard Bauer wrote:
> So, if the feature is enabled, we will always notify about the first
suggestion,
> but only for ARTICLES, never for any other category, whereas if the feature is
> disabled, we will notify for any category that has the notification signal. Is
> that intended behavior or should the conditions be jiggled around?

This condition will use AlwaysNotifyAboutContentSuggestions(), which checks a
variation parameter. There are three possibilities: the feature is disabled (in
which case this service is never even created), it's enabled and we use the
server's signal (by default), and the experimental behavior of this branch when
always_notify=true.

AlwaysNotifyAboutContentSuggestions() is a simple helper function, so I just
inlined it with a TODO for now.

https://codereview.chromium.org/2618703004/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/content_suggestions_notifier_service.cc:110:
suggestions.size()) {
On 2017/01/05 17:31:17, Bernhard Bauer wrote:
> Maybe use !suggestions.empty()?

Done.

[sfiera, 2017-01-05 19:44:11]

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

[commit-bot: I haz the power, 2017-01-05 19:44:37]

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-01-05 20:26:02]

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

[commit-bot: I haz the power, 2017-01-05 20:26:02]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2017-01-06 13:26:59]

LGTM if the behavior mentioned below is intended.

https://codereview.chromium.org/2618703004/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/content_suggestions_notifier_service.cc (right):

https://codereview.chromium.org/2618703004/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/content_suggestions_notifier_service.cc:108: if
(false /* DO_NOT_SUBMIT: always_notify after crrev.com/2613863003 */) {
On 2017/01/05 19:14:37, sfiera wrote:
> On 2017/01/05 17:31:17, Bernhard Bauer wrote:
> > So, if the feature is enabled, we will always notify about the first
> suggestion,
> > but only for ARTICLES, never for any other category, whereas if the feature
is
> > disabled, we will notify for any category that has the notification signal.
Is
> > that intended behavior or should the conditions be jiggled around?
> 
> This condition will use AlwaysNotifyAboutContentSuggestions(), which checks a
> variation parameter. There are three possibilities: the feature is disabled
(in
> which case this service is never even created), it's enabled and we use the
> server's signal (by default), and the experimental behavior of this branch
when
> always_notify=true.

Yes, but my point was, in the third case (if the always notify experiment is
one), we will always notify about articles, but never about anything else, even
if it has the server's signal. Is that intended?

> AlwaysNotifyAboutContentSuggestions() is a simple helper function, so I just
> inlined it with a TODO for now.

[sfiera, 2017-01-06 15:39:53]

https://codereview.chromium.org/2618703004/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/content_suggestions_notifier_service.cc (right):

https://codereview.chromium.org/2618703004/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/content_suggestions_notifier_service.cc:108: if
(false /* DO_NOT_SUBMIT: always_notify after crrev.com/2613863003 */) {
On 2017/01/06 13:26:59, Bernhard Bauer wrote:
> On 2017/01/05 19:14:37, sfiera wrote:
> > On 2017/01/05 17:31:17, Bernhard Bauer wrote:
> > > So, if the feature is enabled, we will always notify about the first
> > suggestion,
> > > but only for ARTICLES, never for any other category, whereas if the
feature
> is
> > > disabled, we will notify for any category that has the notification
signal.
> Is
> > > that intended behavior or should the conditions be jiggled around?
> > 
> > This condition will use AlwaysNotifyAboutContentSuggestions(), which checks
a
> > variation parameter. There are three possibilities: the feature is disabled
> (in
> > which case this service is never even created), it's enabled and we use the
> > server's signal (by default), and the experimental behavior of this branch
> when
> > always_notify=true.
> 
> Yes, but my point was, in the third case (if the always notify experiment is
> one), we will always notify about articles, but never about anything else,
even
> if it has the server's signal. Is that intended?

Oh, I see. Yes, this is the intended behavior. The intent is to replace the
server's signal entirely.

[sfiera, 2017-01-06 15:40:02]

The CQ bit was checked by sfiera@chromium.org

[commit-bot: I haz the power, 2017-01-06 15:40:20]

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-01-06 15:43:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483717202008000,
"parent_rev": "4c5494ab0c4036e09b174ee9d425ba03465deb31", "commit_rev":
"1ed3d555ea644e4648f14fab71a67b45c30d0e15"}

[commit-bot: I haz the power, 2017-01-06 15:44:29]

Description was changed from

==========
Use server signal to decide whether to notify

BUG=675561
==========

to

==========
Use server signal to decide whether to notify

BUG=675561

Review-Url: https://codereview.chromium.org/2618703004
Cr-Commit-Position: refs/heads/master@{#441944}
Committed:
https://chromium.googlesource.com/chromium/src/+/1ed3d555ea644e4648f14fab71a6...
==========

[commit-bot: I haz the power, 2017-01-06 15:44:30]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/1ed3d555ea644e4648f14fab71a6...