[NTP Snippets] Schedule periodic fetching

[NTP Snippets] Schedule periodic fetching

BUG=586452
Committed: https://crrev.com/e6aa5c3359e84aea9a4be25dff250a4ca3fef35e
Cr-Commit-Position: refs/heads/master@{#377581}

[Marc Treib, 2016-02-16 15:03:24]

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

[Marc Treib, 2016-02-16 15:03:25]

PTApreliminaryL!

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java
(right):

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java:25:
public class SnippetsLauncherService extends GcmTaskService {
So, as you can see, this class is currently almost identical to
BackgroundSyncLauncherService. It would be possible to merge the two into a
generic "ChromeGcmTaskService" which would have some sort of dispatch mechanism.
But given how little code this actually is, I find it simpler to just have
separate services. WDYT?

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java:26:
private static final String TAG = "SnippetsLauncherSvc";
Weirdly abbreviated because tags apparently must be <= 20 chars.

[Bernhard Bauer, 2016-02-16 16:19:09]

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java
(right):

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java:93:
.setRequiredNetwork(Task.NETWORK_STATE_UNMETERED)
Do you want to also require being on a charger?

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java
(right):

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java:25:
public class SnippetsLauncherService extends GcmTaskService {
On 2016/02/16 15:03:25, Marc Treib wrote:
> So, as you can see, this class is currently almost identical to
> BackgroundSyncLauncherService. It would be possible to merge the two into a
> generic "ChromeGcmTaskService" which would have some sort of dispatch
mechanism.
> But given how little code this actually is, I find it simpler to just have
> separate services. WDYT?

Hm... I think there are a couple of things in here that are non-trivial enough
that I wouldn't really want to just copy and paste them... Also, it might be
good to look into bundling these, i.e. if we are starting the browser anyway for
Sync, we could just fetch snippets as well.

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java:26:
private static final String TAG = "SnippetsLauncherSvc";
On 2016/02/16 15:03:25, Marc Treib wrote:
> Weirdly abbreviated because tags apparently must be <= 20 chars.

Eh, I'd be fine with just "SnippetsLauncher".

https://codereview.chromium.org/1699143002/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1699143002/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:128: #if defined(OS_ANDROID)
Move this before the OS_IOS section, so the sections themselves are ordered
alphabetically?

https://codereview.chromium.org/1699143002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1699143002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:20: const int
kDefaultFetchingIntervalSeconds = 30 * 60;
TBH, while we are testing this feature, I would reduce the interval even more,
maybe to 5 minutes, but for production 30 minutes seems very frequently (keep in
mind this unconditionally starts the whole browser process every thirty
minutes).

Longer term, it would be cool to e.g. be more aware of if the browser process is
already running, or to integrate more with history analysis to anticipate times
when the user is likely to actually look at snippets.

https://codereview.chromium.org/1699143002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:50: if (scheduler_) {
When would the scheduler be null?

[Marc Treib, 2016-02-16 16:54:27]

Thanks!

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java
(right):

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java:93:
.setRequiredNetwork(Task.NETWORK_STATE_UNMETERED)
On 2016/02/16 16:19:08, Bernhard Bauer wrote:
> Do you want to also require being on a charger?

I don't think so, at least for now? But since this is only a first shot without
any sophisticated logic, I'm fine with requiring charging as well.

Longer-term, I think we should adjust the frequency based on charging state:
E.g. while charging, fetch every 30 mins, otherwise every 2 hours, something
like that.

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java
(right):

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java:25:
public class SnippetsLauncherService extends GcmTaskService {
On 2016/02/16 16:19:08, Bernhard Bauer wrote:
> On 2016/02/16 15:03:25, Marc Treib wrote:
> > So, as you can see, this class is currently almost identical to
> > BackgroundSyncLauncherService. It would be possible to merge the two into a
> > generic "ChromeGcmTaskService" which would have some sort of dispatch
> mechanism.
> > But given how little code this actually is, I find it simpler to just have
> > separate services. WDYT?
> 
> Hm... I think there are a couple of things in here that are non-trivial enough
> that I wouldn't really want to just copy and paste them... Also, it might be
> good to look into bundling these, i.e. if we are starting the browser anyway
for
> Sync, we could just fetch snippets as well.

Hm, I guess that might make sense longer-term. It does seem like a recipe for
confusion though, due to different requirements of jobs. E.g. we want an
unmetered connection and (maybe) charging; background sync is fine with any
connection.
I'll look into combining the two.

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java:26:
private static final String TAG = "SnippetsLauncherSvc";
On 2016/02/16 16:19:08, Bernhard Bauer wrote:
> On 2016/02/16 15:03:25, Marc Treib wrote:
> > Weirdly abbreviated because tags apparently must be <= 20 chars.
> 
> Eh, I'd be fine with just "SnippetsLauncher".

There is another class called SnippetsLauncher, I'd like to have different tags.

https://codereview.chromium.org/1699143002/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1699143002/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_manager.cc:128: #if defined(OS_ANDROID)
On 2016/02/16 16:19:08, Bernhard Bauer wrote:
> Move this before the OS_IOS section, so the sections themselves are ordered
> alphabetically?

Sure, done. Also moved WIN behind CHROMEOS so it's actually alphabetic ;)

https://codereview.chromium.org/1699143002/diff/1/components/ntp_snippets/ntp...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1699143002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:20: const int
kDefaultFetchingIntervalSeconds = 30 * 60;
On 2016/02/16 16:19:08, Bernhard Bauer wrote:
> TBH, while we are testing this feature, I would reduce the interval even more,
> maybe to 5 minutes, but for production 30 minutes seems very frequently (keep
in
> mind this unconditionally starts the whole browser process every thirty
> minutes).

Eh, IMO the value doesn't really matter yet - I was assuming we'd have the
cmdline flag soon. But sure, reduced to 1 minute for now. (Why not go all the
way :) )

> Longer term, it would be cool to e.g. be more aware of if the browser process
is
> already running, or to integrate more with history analysis to anticipate
times
> when the user is likely to actually look at snippets.

Yup. Something kinda sorta like that is on the roadmap.

https://codereview.chromium.org/1699143002/diff/1/components/ntp_snippets/ntp...
components/ntp_snippets/ntp_snippets_service.cc:50: if (scheduler_) {
On 2016/02/16 16:19:08, Bernhard Bauer wrote:
> When would the scheduler be null?

I was vaguely thinking of iOS, which has an NTPSnippetsServiceFactory
implementation (that I haven't updated yet) and not much else. Really no point
in that though - removed the null check.

[Bernhard Bauer, 2016-02-17 16:53:00]

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java
(right):

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java:93:
.setRequiredNetwork(Task.NETWORK_STATE_UNMETERED)
On 2016/02/16 16:54:27, Marc Treib wrote:
> On 2016/02/16 16:19:08, Bernhard Bauer wrote:
> > Do you want to also require being on a charger?
> 
> I don't think so, at least for now? But since this is only a first shot
without
> any sophisticated logic, I'm fine with requiring charging as well.
> 
> Longer-term, I think we should adjust the frequency based on charging state:
> E.g. while charging, fetch every 30 mins, otherwise every 2 hours, something
> like that.

Acknowledged.

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java
(right):

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java:25:
public class SnippetsLauncherService extends GcmTaskService {
On 2016/02/16 16:54:27, Marc Treib wrote:
> I'll look into combining the two.

OK, thanks! Even if we don't want to do coalescing of these jobs, it would at
least be good to share some code.

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java:26:
private static final String TAG = "SnippetsLauncherSvc";
On 2016/02/16 16:54:27, Marc Treib wrote:
> On 2016/02/16 16:19:08, Bernhard Bauer wrote:
> > On 2016/02/16 15:03:25, Marc Treib wrote:
> > > Weirdly abbreviated because tags apparently must be <= 20 chars.
> > 
> > Eh, I'd be fine with just "SnippetsLauncher".
> 
> There is another class called SnippetsLauncher, I'd like to have different
tags.

So, in general that is not a requirement; it's totally fine to use tags just to
group related areas together, not to identify exact classes.

[Marc Treib, 2016-02-18 10:21:47]

GcmTaskServices merged. PTAL!

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java
(right):

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java:25:
public class SnippetsLauncherService extends GcmTaskService {
On 2016/02/17 16:53:00, Bernhard Bauer wrote:
> On 2016/02/16 16:54:27, Marc Treib wrote:
> > I'll look into combining the two.
> 
> OK, thanks! Even if we don't want to do coalescing of these jobs, it would at
> least be good to share some code.

Done. Also added some tests.

https://codereview.chromium.org/1699143002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncherService.java:26:
private static final String TAG = "SnippetsLauncherSvc";
On 2016/02/17 16:53:00, Bernhard Bauer wrote:
> On 2016/02/16 16:54:27, Marc Treib wrote:
> > On 2016/02/16 16:19:08, Bernhard Bauer wrote:
> > > On 2016/02/16 15:03:25, Marc Treib wrote:
> > > > Weirdly abbreviated because tags apparently must be <= 20 chars.
> > > 
> > > Eh, I'd be fine with just "SnippetsLauncher".
> > 
> > There is another class called SnippetsLauncher, I'd like to have different
> tags.
> 
> So, in general that is not a requirement; it's totally fine to use tags just
to
> group related areas together, not to identify exact classes.

Acknowledged (but obsolete now that this has been merged with the other
GcmTaskService implementation).

[Bernhard Bauer, 2016-02-18 10:32:40]

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeGcmTaskService.java
(right):

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeGcmTaskService.java:28:
public class ChromeGcmTaskService extends GcmTaskService {
Should we just name this ChromeBackgroundService? Using GcmTaskService is an
implementation detail.

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsController.java
(right):

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsController.java:24:
// applicationContext can be null in tests.
Nit: Use pipe symbols for variable names, otherwise it reads a bit strange.

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeGcmTaskServiceTest.java
(right):

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeGcmTaskServiceTest.java:23:
public class ChromeGcmTaskServiceTest extends InstrumentationTestCase {
Thanks for writing a test! I think it might be a bit easier though to do this as
a JUnit test. You can look around //chrome/android/junit for existing tests to
model it after.

[Marc Treib, 2016-02-18 10:52:40]

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeGcmTaskService.java
(right):

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeGcmTaskService.java:28:
public class ChromeGcmTaskService extends GcmTaskService {
On 2016/02/18 10:32:40, Bernhard Bauer wrote:
> Should we just name this ChromeBackgroundService? Using GcmTaskService is an
> implementation detail.

Done.

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsController.java
(right):

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsController.java:24:
// applicationContext can be null in tests.
On 2016/02/18 10:32:40, Bernhard Bauer wrote:
> Nit: Use pipe symbols for variable names, otherwise it reads a bit strange.

Done.

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeGcmTaskServiceTest.java
(right):

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeGcmTaskServiceTest.java:23:
public class ChromeGcmTaskServiceTest extends InstrumentationTestCase {
On 2016/02/18 10:32:40, Bernhard Bauer wrote:
> Thanks for writing a test! I think it might be a bit easier though to do this
as
> a JUnit test. You can look around //chrome/android/junit for existing tests to
> model it after.

Ah, the diff is weird - I didn't actually write most of this, it was already
there as BackgroundSyncLauncherServiceTest. I just extended/generalized it a
bit. I'll defer to the BackgroundSync folks on why they chose an
InstrumentationTest (once May's stuff has landed).

[Bernhard Bauer, 2016-02-18 11:43:12]

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeGcmTaskServiceTest.java
(right):

https://codereview.chromium.org/1699143002/diff/40001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeGcmTaskServiceTest.java:23:
public class ChromeGcmTaskServiceTest extends InstrumentationTestCase {
On 2016/02/18 10:52:40, Marc Treib wrote:
> On 2016/02/18 10:32:40, Bernhard Bauer wrote:
> > Thanks for writing a test! I think it might be a bit easier though to do
this
> as
> > a JUnit test. You can look around //chrome/android/junit for existing tests
to
> > model it after.
> 
> Ah, the diff is weird - I didn't actually write most of this, it was already
> there as BackgroundSyncLauncherServiceTest. I just extended/generalized it a
> bit. I'll defer to the BackgroundSync folks on why they chose an
> InstrumentationTest (once May's stuff has landed).

Acknowledged.

[Bernhard Bauer, 2016-02-18 11:43:45]

Oh, and LGTM I meant :)

[Marc Treib, 2016-02-19 11:07:29]

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

[Marc Treib, 2016-02-19 11:07:29]

+anthonyvd for profile_manager.cc - PTAL!

[Marc Treib, 2016-02-19 11:25:05]

Patchset #7 (id:120001) has been deleted

[Marc Treib, 2016-02-19 11:25:11]

Patchset #6 (id:100001) has been deleted

[Marc Treib, 2016-02-19 11:27:06]

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

[Marc Treib, 2016-02-19 11:27:06]

+iclelland FYI: I've generalized the BackgroundSyncLauncherService to a
ChromeBackgroundService, since we now have a second customer for a
GcmTaskService. Also, one question to you below regarding the tests. Thanks!

https://codereview.chromium.org/1699143002/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java
(right):

https://codereview.chromium.org/1699143002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java:23:
public class ChromeBackgroundServiceTest extends InstrumentationTestCase {
iclelland: bauerb suggested this kind of test might be easier as a JUnit test
rather than an instrumentation test - do you know why this was written as an
instrumentation test?

[Bernhard Bauer, 2016-02-19 11:31:40]

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

[Bernhard Bauer, 2016-02-19 11:31:41]

+jkarlin

[iclelland, 2016-02-19 13:59:09]

Cool -- the BackgroundSync code merge LGTM.

https://codereview.chromium.org/1699143002/diff/140001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java
(right):

https://codereview.chromium.org/1699143002/diff/140001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java:23:
public class ChromeBackgroundServiceTest extends InstrumentationTestCase {
On 2016/02/19 11:27:06, Marc Treib wrote:
> iclelland: bauerb suggested this kind of test might be easier as a JUnit test
> rather than an instrumentation test - do you know why this was written as an
> instrumentation test?

I think the only reason I looked for something a bit more Android-specific than
a junit.TestCase was to be able to access the Context in the setUp method. I'm
not otherwise opposed to changing it, if you think it'll be clearer as a plain
TestCase.

[anthonyvd, 2016-02-19 15:04:28]

profile_manager.cc lgtm % small nit

https://codereview.chromium.org/1699143002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1699143002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:131: #include
"chrome/installer/util/browser_distribution.h"
I don't see why you would need this include.

[Marc Treib, 2016-02-19 15:34:33]

https://codereview.chromium.org/1699143002/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1699143002/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:131: #include
"chrome/installer/util/browser_distribution.h"
On 2016/02/19 15:04:28, anthonyvd wrote:
> I don't see why you would need this include.

Ah, I just moved this block so that the "defined(..)"s would be alphabetic. But
looks like this is actually not required, so removed. Thanks!

[Marc Treib, 2016-02-19 16:40:17]

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

[Marc Treib, 2016-02-19 16:40:18]

+noyau for iOS. PTAL!

[noyau (Ping after 24h), 2016-02-22 11:58:50]

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_scheduler.h (right):

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_scheduler.h:11: class NTPSnippetsScheduler
{
I'm confused about the connection between this class and the snippet service.
What method does the scheduler call back on the service to do the fetch ? Can it
be documented on a Scheduler observer of some sort? How will the scheduler deal
with backing up when the download fails, or if the download takes a long time?
Who own instance of these classes, as apparently the service is not?

My problem is that I'm looking at this code and I don't understand what I have
to do to implement a concrete class on iOS. I'm also worried that the concrete
implementation for this is all in java, and as such completely unusable on iOS.
What is the reason for not implementing this in C++? It seems to me that having
common code between the platform would be a plus, keeping only the UI different.

Is there a scheduler code that can be reused from component updater for example?

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:15: namespace ntp_snippets {
nit: I would avoid nesting the namespaces, this line can go after the anonymous
namespace.

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:19: // TODO(treib): This is an
extremely small value, for ease of development.
Please use the format TODO(crbug/XXXX): and add a bug to track this. This is too
easy to let slip…

[Bernhard Bauer, 2016-02-22 12:05:12]

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_scheduler.h (right):

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_scheduler.h:11: class NTPSnippetsScheduler
{
On 2016/02/22 11:58:49, noyau wrote:
> I'm confused about the connection between this class and the snippet service.
> What method does the scheduler call back on the service to do the fetch ? Can
it
> be documented on a Scheduler observer of some sort? How will the scheduler
deal
> with backing up when the download fails, or if the download takes a long time?
> Who own instance of these classes, as apparently the service is not?
> 
> My problem is that I'm looking at this code and I don't understand what I have
> to do to implement a concrete class on iOS. I'm also worried that the concrete
> implementation for this is all in java, and as such completely unusable on
iOS.
> What is the reason for not implementing this in C++? It seems to me that
having
> common code between the platform would be a plus, keeping only the UI
different.
> 
> Is there a scheduler code that can be reused from component updater for
example?

The component updater does not use the Android scheduling mechanism ATM, so it
will only schedule tasks while Chrome is running (which is a known issue). If
anything, we could eventually start using the Android implementation here for
the component updater as well, but we will need an implementation in Java.

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:19: // TODO(treib): This is an
extremely small value, for ease of development.
On 2016/02/22 11:58:49, noyau wrote:
> Please use the format TODO(crbug/XXXX): and add a bug to track this. This is
too
> easy to let slip…

Nit: `TODO(crbug.com/1234)`, to make it clickable everywhere.

[Marc Treib, 2016-02-22 12:44:32]

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_scheduler.h (right):

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_scheduler.h:11: class NTPSnippetsScheduler
{
On 2016/02/22 12:05:12, Bernhard Bauer wrote:
> On 2016/02/22 11:58:49, noyau wrote:
> > I'm confused about the connection between this class and the snippet
service.
> > What method does the scheduler call back on the service to do the fetch ?
Can
> it
> > be documented on a Scheduler observer of some sort? How will the scheduler
> deal
> > with backing up when the download fails, or if the download takes a long
time?

Currently it just calls FetchSnippets on the service. I've added a comment on
the Scheduler interface saying that. IMO an extra observer is overkill at this
point, particularly since the specifics are likely to change soon.

Any logic to handle failed downloads etc will live in the service (so far, we
don't have any), so it'll be shared between platforms.

> > Who own instance of these classes, as apparently the service is not?

It's a singleton, because that's what makes sense on the Java side, and as long
as we only have an Android implementation there's no need to complicate things
here. If necessary, it'll be easy to add a wrapper around the singleton, so that
the service can own that.

> > My problem is that I'm looking at this code and I don't understand what I
have
> > to do to implement a concrete class on iOS. I'm also worried that the
concrete
> > implementation for this is all in java, and as such completely unusable on
> iOS.
> > What is the reason for not implementing this in C++? It seems to me that
> having
> > common code between the platform would be a plus, keeping only the UI
> different.

Re common code, absolutely. But all the scheduler implementation does is call
some Android OS functions, so there's no way to share it.
All the actual logic for scheduling will live in the service.

> > Is there a scheduler code that can be reused from component updater for
> example?
> 
> The component updater does not use the Android scheduling mechanism ATM, so it
> will only schedule tasks while Chrome is running (which is a known issue). If
> anything, we could eventually start using the Android implementation here for
> the component updater as well, but we will need an implementation in Java.

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_service.cc (right):

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:15: namespace ntp_snippets {
On 2016/02/22 11:58:49, noyau wrote:
> nit: I would avoid nesting the namespaces, this line can go after the
anonymous
> namespace.

Done.
Nesting is a somewhat common pattern though. If we ever need anything from
ntp_snippets:: in the anonymous namespace, then I'd prefer nesting over
repeating the namespace everywhere.

https://codereview.chromium.org/1699143002/diff/200001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_service.cc:19: // TODO(treib): This is an
extremely small value, for ease of development.
On 2016/02/22 12:05:12, Bernhard Bauer wrote:
> On 2016/02/22 11:58:49, noyau wrote:
> > Please use the format TODO(crbug/XXXX): and add a bug to track this. This is
> too
> > easy to let slip…
> 
> Nit: `TODO(crbug.com/1234)`, to make it clickable everywhere.

Done.

[noyau (Ping after 24h), 2016-02-22 15:02:31]

lgtm

https://codereview.chromium.org/1699143002/diff/220001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_scheduler.h (right):

https://codereview.chromium.org/1699143002/diff/220001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_scheduler.h:19: virtual bool Unschedule() =
0;
Add DISALLOW_COPY_AND_ASSIGN()

[Marc Treib, 2016-02-22 17:13:08]

https://codereview.chromium.org/1699143002/diff/220001/components/ntp_snippet...
File components/ntp_snippets/ntp_snippets_scheduler.h (right):

https://codereview.chromium.org/1699143002/diff/220001/components/ntp_snippet...
components/ntp_snippets/ntp_snippets_scheduler.h:19: virtual bool Unschedule() =
0;
On 2016/02/22 15:02:31, noyau wrote:
> Add DISALLOW_COPY_AND_ASSIGN()

Done. I also had to add a default constructor :-/

[Marc Treib, 2016-02-24 14:14:50]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-02-24 14:14:50]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
anthonyvd@chromium.org, iclelland@chromium.org, noyau@chromium.org
Link to the patchset: https://codereview.chromium.org/1699143002/#ps260001
(title: "rebase")

[commit-bot: I haz the power, 2016-02-24 14:16:32]

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

[commit-bot: I haz the power, 2016-02-24 15:07:11]

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

[commit-bot: I haz the power, 2016-02-24 15:07:12]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Marc Treib, 2016-02-25 14:51:07]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-02-25 14:51:08]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
noyau@chromium.org, anthonyvd@chromium.org, iclelland@chromium.org
Link to the patchset: https://codereview.chromium.org/1699143002/#ps300001
(title: "fix bots")

[commit-bot: I haz the power, 2016-02-25 14:51:45]

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

[commit-bot: I haz the power, 2016-02-25 15:44:52]

Committed patchset #14 (id:300001)

[commit-bot: I haz the power, 2016-02-25 15:46:03]

Description was changed from

==========
[NTP Snippets] Schedule periodic fetching

BUG=586452
==========

to

==========
[NTP Snippets] Schedule periodic fetching

BUG=586452

Committed: https://crrev.com/e6aa5c3359e84aea9a4be25dff250a4ca3fef35e
Cr-Commit-Position: refs/heads/master@{#377581}
==========

[commit-bot: I haz the power, 2016-02-25 15:46:04]

Patchset 14 (id:??) landed as
https://crrev.com/e6aa5c3359e84aea9a4be25dff250a4ca3fef35e
Cr-Commit-Position: refs/heads/master@{#377581}