[NTP Snippets] Reschedule background fetching on app upgrade if it was scheduled before

[NTP Snippets] Reschedule background fetching on app upgrade if it was scheduled before

BUG=646842
Committed: https://crrev.com/65d2499b345e37fd9d46e348ae986f6793629622
Cr-Commit-Position: refs/heads/master@{#421243}

[Marc Treib, 2016-09-23 17:56:46]

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

[Marc Treib, 2016-09-23 17:56:47]

PTAL! (I realize neither of us owns anything here, but since you've already
looked at everything else in this series, might as well look at this too :) )

[jkrcal, 2016-09-26 16:24:05]

lgtm / comments below (feel free to address / ignore them).

https://codereview.chromium.org/2362323002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
(right):

https://codereview.chromium.org/2362323002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:223:
launchBrowser(this, /*tag=*/""); // The |tag| doesn't matter here.
Two space before comment, please.

https://codereview.chromium.org/2362323002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java
(right):

https://codereview.chromium.org/2362323002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java:213:
}
I am a bit sad about the combinatorial explosion of these unit tests :)

I know that tests should be as locally readable as possible. Here, the
repetition feels a bit stupid. 
Why not one common test function with two boolean arguments? (And 4 one line
tests)

[Marc Treib, 2016-09-27 09:30:06]

https://codereview.chromium.org/2362323002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java
(right):

https://codereview.chromium.org/2362323002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:223:
launchBrowser(this, /*tag=*/""); // The |tag| doesn't matter here.
On 2016/09/26 16:24:05, jkrcal wrote:
> Two space before comment, please.

That applies to C++, but not to Java (yay consistency...). If I add that space,
"git cl format" removes it again.

https://codereview.chromium.org/2362323002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java
(right):

https://codereview.chromium.org/2362323002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/ChromeBackgroundServiceTest.java:213:
}
On 2016/09/26 16:24:05, jkrcal wrote:
> I am a bit sad about the combinatorial explosion of these unit tests :)
> 
> I know that tests should be as locally readable as possible. Here, the
> repetition feels a bit stupid. 
> Why not one common test function with two boolean arguments? (And 4 one line
> tests)

Done. (More or less - I made a method to wrap the two mTaskService calls. Is
that what you meant? Anyway, I think that's a reasonable trade-off between
locality and duplication.)

[Marc Treib, 2016-09-27 10:22:33]

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

[Marc Treib, 2016-09-27 10:22:33]

+bauerb for OWNERS approval. PTAL!

[Marc Treib, 2016-09-27 15:15:55]

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

[commit-bot: I haz the power, 2016-09-27 15:16:18]

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, 2016-09-27 15:19:00]

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

[commit-bot: I haz the power, 2016-09-27 15:19:01]

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_...)

[Marc Treib, 2016-09-27 15:20:19]

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

[commit-bot: I haz the power, 2016-09-27 15:20:35]

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, 2016-09-27 16:00:38]

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

[commit-bot: I haz the power, 2016-09-27 16:00:39]

Dry run: This issue passed the CQ dry run.

[Bernhard Bauer, 2016-09-27 16:12:50]

lgtm

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

https://codereview.chromium.org/2362323002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java:147:
boolean isScheduled = periodWifiSeconds != 0 || periodFallbackSeconds != 0;
Move this out of the try block so that it's clear that this is not the part that
can throw the exception?

[Marc Treib, 2016-09-27 16:43:04]

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

https://codereview.chromium.org/2362323002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/snippets/SnippetsLauncher.java:147:
boolean isScheduled = periodWifiSeconds != 0 || periodFallbackSeconds != 0;
On 2016/09/27 16:12:50, Bernhard Bauer wrote:
> Move this out of the try block so that it's clear that this is not the part
that
> can throw the exception?

Done.

[Marc Treib, 2016-09-27 16:43:32]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-09-27 16:43:32]

The patchset sent to the CQ was uploaded after l-g-t-m from jkrcal@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2362323002/#ps60001
(title: "bauerb review")

[commit-bot: I haz the power, 2016-09-27 16:44:01]

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, 2016-09-27 17:14:45]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-27 17:18:25]

Description was changed from

==========
[NTP Snippets] Reschedule background fetching on app upgrade if it was scheduled
before

BUG=646842
==========

to

==========
[NTP Snippets] Reschedule background fetching on app upgrade if it was scheduled
before

BUG=646842

Committed: https://crrev.com/65d2499b345e37fd9d46e348ae986f6793629622
Cr-Commit-Position: refs/heads/master@{#421243}
==========

[commit-bot: I haz the power, 2016-09-27 17:18:26]

Patchset 4 (id:??) landed as
https://crrev.com/65d2499b345e37fd9d46e348ae986f6793629622
Cr-Commit-Position: refs/heads/master@{#421243}