[Remote suggestions] Do not fetch before EULA accepted

[Remote suggestions] Do not fetch before EULA accepted

This CL makes sure we never fetch snippets before EULA gets accepted by
the user (if the platforms supports accepting EULA on FRE).

BUG=676662
Review-Url: https://codereview.chromium.org/2759943002
Cr-Commit-Position: refs/heads/master@{#460348}
Committed: https://chromium.googlesource.com/chromium/src/+/f7081f8bb1607bc160ff793fd02cf257d04c6ef8

[jkrcal, 2017-03-20 14:12:11]

jkrcal@chromium.org changed reviewers:
+ tschumann@chromium.org

[jkrcal, 2017-03-20 14:12:11]

Tim, could you PTAL?
(feel free to relay to someone else if you are too busy, pre-vacation)

[jkrcal, 2017-03-20 14:12:41]

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

[commit-bot: I haz the power, 2017-03-20 14:12:59]

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-03-20 14:21:11]

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

[commit-bot: I haz the power, 2017-03-20 14:21:12]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[jkrcal, 2017-03-21 08:46:02]

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

[commit-bot: I haz the power, 2017-03-21 08:46:17]

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-03-21 11:10:45]

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

[commit-bot: I haz the power, 2017-03-21 11:10:46]

Dry run: This issue passed the CQ dry run.

[tschumann, 2017-03-21 20:13:15]

sorry for the delay!

https://codereview.chromium.org/2759943002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h
(right):

https://codereview.chromium.org/2759943002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:62:
public web_resource::EulaAcceptedNotifier::Observer {
as you know, I'm trying to avoid multiple inheritance.
In this case, we can actually quite nicely use aggregation as the observer is
using the RemoteSuggestionsScheduler's public interface.

So why not have a small class like EulaState with a constructor taking the
local_state_prefs and remote suggestions scheduler. EulaState would also
encapsulate the notifier (so no doubt if it's null or not)
The SchedulingRemoteSuggestionsProvider's constructor can instantiate build it
and simply ask that state for all info it needs. WDYT?

[jkrcal, 2017-03-22 09:36:05]

PTAL again!

https://codereview.chromium.org/2759943002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h
(right):

https://codereview.chromium.org/2759943002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:62:
public web_resource::EulaAcceptedNotifier::Observer {
On 2017/03/21 20:13:15, tschumann wrote:
> as you know, I'm trying to avoid multiple inheritance.
> In this case, we can actually quite nicely use aggregation as the observer is
> using the RemoteSuggestionsScheduler's public interface.
> 
> So why not have a small class like EulaState with a constructor taking the
> local_state_prefs and remote suggestions scheduler. EulaState would also
> encapsulate the notifier (so no doubt if it's null or not)
> The SchedulingRemoteSuggestionsProvider's constructor can instantiate build it
> and simply ask that state for all info it needs. WDYT?

Done. WDYT?

[tschumann, 2017-03-22 09:47:02]

https://codereview.chromium.org/2759943002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h
(right):

https://codereview.chromium.org/2759943002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:62:
public web_resource::EulaAcceptedNotifier::Observer {
On 2017/03/22 09:36:04, jkrcal wrote:
> On 2017/03/21 20:13:15, tschumann wrote:
> > as you know, I'm trying to avoid multiple inheritance.
> > In this case, we can actually quite nicely use aggregation as the observer
is
> > using the RemoteSuggestionsScheduler's public interface.
> > 
> > So why not have a small class like EulaState with a constructor taking the
> > local_state_prefs and remote suggestions scheduler. EulaState would also
> > encapsulate the notifier (so no doubt if it's null or not)
> > The SchedulingRemoteSuggestionsProvider's constructor can instantiate build
it
> > and simply ask that state for all info it needs. WDYT?
> 
> Done. WDYT?

Thanks! The cyclic dependency is still a bit of a smell but I don't see and easy
way to improve this.

https://codereview.chromium.org/2759943002/diff/40001/components/ntp_snippets...
File
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2759943002/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:151:

(I could have sworn I wrote this comment yesterday already, but apparently
not...)
Please add a unit-test making sure we're not fetching before the EULA got
accepted.
(https://codesearch.chromium.org/chromium/src/components/web_resource/eula_acc...
has some hints how to do this easily).

[jkrcal, 2017-03-22 16:48:01]

Added the unit-test. It is a bit messy because on some platforms the
EulaNotifier is not constructed and I want the tests to pass on all platforms.

PTAL.

https://codereview.chromium.org/2759943002/diff/20001/components/ntp_snippets...
File components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h
(right):

https://codereview.chromium.org/2759943002/diff/20001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider.h:62:
public web_resource::EulaAcceptedNotifier::Observer {
On 2017/03/22 09:47:01, tschumann wrote:
> On 2017/03/22 09:36:04, jkrcal wrote:
> > On 2017/03/21 20:13:15, tschumann wrote:
> > > as you know, I'm trying to avoid multiple inheritance.
> > > In this case, we can actually quite nicely use aggregation as the observer
> is
> > > using the RemoteSuggestionsScheduler's public interface.
> > > 
> > > So why not have a small class like EulaState with a constructor taking the
> > > local_state_prefs and remote suggestions scheduler. EulaState would also
> > > encapsulate the notifier (so no doubt if it's null or not)
> > > The SchedulingRemoteSuggestionsProvider's constructor can instantiate
build
> it
> > > and simply ask that state for all info it needs. WDYT?
> > 
> > Done. WDYT?
> 
> Thanks! The cyclic dependency is still a bit of a smell but I don't see and
easy
> way to improve this.

Acknowledged.

https://codereview.chromium.org/2759943002/diff/40001/components/ntp_snippets...
File
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2759943002/diff/40001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:151:

On 2017/03/22 09:47:02, tschumann wrote:
> (I could have sworn I wrote this comment yesterday already, but apparently
> not...)
> Please add a unit-test making sure we're not fetching before the EULA got
> accepted.
>
(https://codesearch.chromium.org/chromium/src/components/web_resource/eula_acc...
> has some hints how to do this easily).

Done.

[tschumann, 2017-03-22 17:56:41]

lgtm

some nits left. Thanks!

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
File
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:172:
return web_resource::EulaAcceptedNotifier::Create(&local_state_);
let's make this more specific and add != nullptr
you can then reduce the comment to:
// Create() returns a unique_ptr, so this is no leak.

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:203:
void ActivateUnderlyingProviderWithoutEula() {
I'd get rid of this function as it's misleading. This function has no control
over whether Eula was accepted or not.

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:229:
// Do not run the test if Eula notifier is available
// Only run this tests on platforms that don't support Eula.
?

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:234:
// First enable the scheduler.
together with the comment about inlining
ActivateUnderlyingProviderWithoutEula().
// Activating the provider should schedule the background fetch.
EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
scheduling_provider_->OnProviderActivated();
// Verify fetches get triggered.
EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
scheduling_provider_->OnPersistentSchedulerWakeUp();

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:246:
// Do not run the test if Eula notifier is not available.
// Only run this tests on platforms supporting Eula.

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:250:
// First enable the scheduler.
// Activating the provider should schedule the background fetch.
?

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:263:
// Do not run the test if Eula notifier is not available.
Only run this tests on platforms supporting Eula.

[jkrcal, 2017-03-23 09:29:41]

jkrcal@chromium.org changed reviewers:
+ achuith@chromium.org, noyau@chromium.org

[jkrcal, 2017-03-23 09:29:44]

Thanks, Tim!

Achuith, could you PTAL at +components/web_resource?
Éric, could you PTAL at ios factory?

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
File
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc
(right):

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:172:
return web_resource::EulaAcceptedNotifier::Create(&local_state_);
On 2017/03/22 17:56:41, tschumann wrote:
> let's make this more specific and add != nullptr
> you can then reduce the comment to:
> // Create() returns a unique_ptr, so this is no leak. 

Done.

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:203:
void ActivateUnderlyingProviderWithoutEula() {
On 2017/03/22 17:56:41, tschumann wrote:
> I'd get rid of this function as it's misleading. This function has no control
> over whether Eula was accepted or not.

Done.

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:229:
// Do not run the test if Eula notifier is available
On 2017/03/22 17:56:41, tschumann wrote:
> // Only run this tests on platforms that don't support Eula.
> ?

Done.

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:234:
// First enable the scheduler.
On 2017/03/22 17:56:41, tschumann wrote:
> together with the comment about inlining
> ActivateUnderlyingProviderWithoutEula().
> // Activating the provider should schedule the background fetch.
> EXPECT_CALL(persistent_scheduler_, Schedule(_, _));
> scheduling_provider_->OnProviderActivated();
> // Verify fetches get triggered.
> EXPECT_CALL(*underlying_provider_, RefetchInTheBackground(_));
> scheduling_provider_->OnPersistentSchedulerWakeUp();

Done.

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:246:
// Do not run the test if Eula notifier is not available.
On 2017/03/22 17:56:41, tschumann wrote:
> // Only run this tests on platforms supporting Eula.

Done.

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:250:
// First enable the scheduler.
On 2017/03/22 17:56:41, tschumann wrote:
> // Activating the provider should schedule the background fetch.
> ?

Done.

https://codereview.chromium.org/2759943002/diff/60001/components/ntp_snippets...
components/ntp_snippets/remote/scheduling_remote_suggestions_provider_unittest.cc:263:
// Do not run the test if Eula notifier is not available.
On 2017/03/22 17:56:41, tschumann wrote:
> Only run this tests on platforms supporting Eula.

Done.

[achuithb, 2017-03-23 18:00:18]

On 2017/03/23 09:29:44, jkrcal wrote:
> Thanks, Tim!
> 
> Achuith, could you PTAL at +components/web_resource?

There're no components/web_resource files in this CL

[jkrcal, 2017-03-23 18:10:11]

On 2017/03/23 18:00:18, achuithb wrote:
> On 2017/03/23 09:29:44, jkrcal wrote:
> > Thanks, Tim!
> > 
> > Achuith, could you PTAL at +components/web_resource?
> 
> There're no components/web_resource files in this CL

I am sorry, I was not verbose enough. It is an added DEPS entry.

[achuithb, 2017-03-23 18:16:54]

On 2017/03/23 18:10:11, jkrcal wrote:
> On 2017/03/23 18:00:18, achuithb wrote:
> > On 2017/03/23 09:29:44, jkrcal wrote:
> > > Thanks, Tim!
> > > 
> > > Achuith, could you PTAL at +components/web_resource?
> > 
> > There're no components/web_resource files in this CL
> 
> I am sorry, I was not verbose enough. It is an added DEPS entry.

lgtm DEPS

[jkrcal, 2017-03-27 09:23:38]

Thanks!

Éric, friendly ping!

[noyau (Ping after 24h), 2017-03-28 15:16:58]

ios lgtm.

[jkrcal, 2017-03-28 15:20:11]

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

[commit-bot: I haz the power, 2017-03-28 15:20:59]

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-03-28 15:24:12]

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

[commit-bot: I haz the power, 2017-03-28 15:24:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jkrcal, 2017-03-28 15:33:03]

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

[commit-bot: I haz the power, 2017-03-28 15:33:50]

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-03-28 17:14:14]

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

[commit-bot: I haz the power, 2017-03-28 17:14:15]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jkrcal, 2017-03-29 09:59:14]

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

[commit-bot: I haz the power, 2017-03-29 09:59:39]

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-03-29 11:02:29]

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

[commit-bot: I haz the power, 2017-03-29 11:02:30]

Dry run: This issue passed the CQ dry run.

[jkrcal, 2017-03-29 11:04:27]

The CQ bit was checked by jkrcal@chromium.org

[jkrcal, 2017-03-29 11:04:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
tschumann@chromium.org, achuith@chromium.org, noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/2759943002/#ps140001
(title: "Fix compilation")

[commit-bot: I haz the power, 2017-03-29 11:04:41]

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-03-29 11:08:46]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490785467564660,
"parent_rev": "69fd1b5798090316e9969c007aaf88cd5da6ef76", "commit_rev":
"f7081f8bb1607bc160ff793fd02cf257d04c6ef8"}

[commit-bot: I haz the power, 2017-03-29 11:09:35]

Description was changed from

==========
[Remote suggestions] Do not fetch before EULA accepted

This CL makes sure we never fetch snippets before EULA gets accepted by
the user (if the platforms supports accepting EULA on FRE).

BUG=676662
==========

to

==========
[Remote suggestions] Do not fetch before EULA accepted

This CL makes sure we never fetch snippets before EULA gets accepted by
the user (if the platforms supports accepting EULA on FRE).

BUG=676662

Review-Url: https://codereview.chromium.org/2759943002
Cr-Commit-Position: refs/heads/master@{#460348}
Committed:
https://chromium.googlesource.com/chromium/src/+/f7081f8bb1607bc160ff793fd02c...
==========

[commit-bot: I haz the power, 2017-03-29 11:09:36]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/f7081f8bb1607bc160ff793fd02c...