Android: Switch to thread pool executor

Android: Switch to thread pool executor.

Switches BackgroundSyncLauncher to the thread pool executor and add
comments explaining why AsyncTask is necessary.

BUG=601053
Committed: https://crrev.com/ad3ee42510afda09e00cb73c694bfa4626980d1d
Cr-Commit-Position: refs/heads/master@{#438573}

[Peter Wen, 2016-12-08 21:03:51]

Description was changed from

==========
Android: Switch to thread pool executor

Switches BackgroundSyncLauncher to use AsyncTask.THREAD_POOL_EXECUTOR.

BUG=601053
==========

to

==========
Android: Remove AsyncTask from sync launcher

Switches BackgroundSyncLauncher to edit preferences directly. Since
Preferences#apply is used there is no performance impact.

BUG=601053
==========

[Peter Wen, 2016-12-08 21:04:34]

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

[Peter Wen, 2016-12-08 21:04:35]

Hi Ian,

I don't think the two AsyncTasks are necessary. It's only .commit that write to
disk, .apply makes the edits in memory and schedules a task to commit to disk.

Thanks,

Peter

[Peter Wen, 2016-12-13 16:35:15]

Friendly ping.

[iclelland, 2016-12-13 16:54:29]

On 2016/12/13 16:35:15, Peter Wen wrote:
> Friendly ping.

Sorry for the delay. I'm not certain what you mean when you say you "don't think
the two AsyncTaks are necessary". This CL doesn't actually remove them, just
puts them into the parallel thread pool, if I understand it.

There don't seem to be any possible races here (they're run at very different
times) so I don't see a problem with the change; it just doesn't match the
comment or the CL description, I think.

If you mean that we could get rid of the AsyncTask entirely, and just call apply
directly -- it was added here: https://codereview.chromium.org/1288593003
because, apparently, writing them directly violated strict mode. Is that no
longer an issue?

[Peter Wen, 2016-12-13 17:52:42]

Description was changed from

==========
Android: Remove AsyncTask from sync launcher

Switches BackgroundSyncLauncher to edit preferences directly. Since
Preferences#apply is used there is no performance impact.

BUG=601053
==========

to

==========
Android: Switch to thread pool executor.

Switches BackgroundSyncLauncher to the thread pool executor and add
comments explaining why AsyncTask is necessary.

BUG=601053
==========

[Peter Wen, 2016-12-13 17:56:33]

On 2016/12/13 16:54:29, iclelland wrote:
> On 2016/12/13 16:35:15, Peter Wen wrote:
> > Friendly ping.
> 
> Sorry for the delay. I'm not certain what you mean when you say you "don't
think
> the two AsyncTaks are necessary". This CL doesn't actually remove them, just
> puts them into the parallel thread pool, if I understand it.
> 
> There don't seem to be any possible races here (they're run at very different
> times) so I don't see a problem with the change; it just doesn't match the
> comment or the CL description, I think.
> 
> If you mean that we could get rid of the AsyncTask entirely, and just call
apply
> directly -- it was added here: https://codereview.chromium.org/1288593003
> because, apparently, writing them directly violated strict mode. Is that no
> longer an issue?

Sorry for the mix-up, forgot to upload patch 2. Based on that bug I missed the
fact that this would be run without a browser process, so I doubt
ContextUtils.getAppSharedPreferences will have been cached earlier, which means
that these accesses will trigger disk read on the first fetch, so AsyncTask is
appropriate here due to it being on the UI thread.

Thanks for checking possible races, PTAL.

[Peter Wen, 2016-12-13 17:57:18]

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

[commit-bot: I haz the power, 2016-12-13 17:57:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[iclelland, 2016-12-13 18:26:18]

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

https://codereview.chromium.org/2562643004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:29:
* {@link AsyncTask} is necessary as the browser process will not have warmed up
the
Should this comment be on the methods that use AsyncTask? Or is the point to
justify the usage within this class?

[iclelland, 2016-12-13 18:26:47]

On 2016/12/13 17:56:33, Peter Wen wrote:
> On 2016/12/13 16:54:29, iclelland wrote:
> > On 2016/12/13 16:35:15, Peter Wen wrote:
> > > Friendly ping.
> > 
> > Sorry for the delay. I'm not certain what you mean when you say you "don't
> think
> > the two AsyncTaks are necessary". This CL doesn't actually remove them, just
> > puts them into the parallel thread pool, if I understand it.
> > 
> > There don't seem to be any possible races here (they're run at very
different
> > times) so I don't see a problem with the change; it just doesn't match the
> > comment or the CL description, I think.
> > 
> > If you mean that we could get rid of the AsyncTask entirely, and just call
> apply
> > directly -- it was added here: https://codereview.chromium.org/1288593003
> > because, apparently, writing them directly violated strict mode. Is that no
> > longer an issue?
> 
> Sorry for the mix-up, forgot to upload patch 2. Based on that bug I missed the
> fact that this would be run without a browser process, so I doubt
> ContextUtils.getAppSharedPreferences will have been cached earlier, which
means
> that these accesses will trigger disk read on the first fetch, so AsyncTask is
> appropriate here due to it being on the UI thread.
> 
> Thanks for checking possible races, PTAL.

Thanks, that makes sense now :)

LGTM

[commit-bot: I haz the power, 2016-12-13 18:27:42]

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

[commit-bot: I haz the power, 2016-12-13 18:27:43]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Peter Wen, 2016-12-13 19:07:47]

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

[commit-bot: I haz the power, 2016-12-13 19:08:28]

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-12-13 19:46:33]

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

[commit-bot: I haz the power, 2016-12-13 19:46:34]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Peter Wen, 2016-12-13 20:55:39]

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

[commit-bot: I haz the power, 2016-12-13 20:56:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Wen, 2016-12-13 20:56:31]

wnwen@chromium.org changed reviewers:
+ mariakhomenko@chromium.org

[Peter Wen, 2016-12-13 20:56:32]

Thanks Ian!

+mariakhomenko@ for OWNERS.

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

https://codereview.chromium.org/2562643004/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:29:
* {@link AsyncTask} is necessary as the browser process will not have warmed up
the
On 2016/12/13 18:26:18, iclelland wrote:
> Should this comment be on the methods that use AsyncTask? Or is the point to
> justify the usage within this class?
I'd like to justify usage in this class as I think other methods in this class
fall under the same umbrella.

[Maria, 2016-12-13 21:06:23]

lgtm

https://codereview.chromium.org/2562643004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/2562643004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:30:
* {@link SharedPreferences} before it is used here.
I think you should put this comment in shouldLaunchBrowserIfStopped. This is
implementation detail for that method, rather than for the class.

[commit-bot: I haz the power, 2016-12-13 21:34:42]

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

[commit-bot: I haz the power, 2016-12-13 21:34:42]

Dry run: This issue passed the CQ dry run.

[Peter Wen, 2016-12-13 21:46:47]

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

[commit-bot: I haz the power, 2016-12-13 21:47:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Wen, 2016-12-13 21:49:08]

The CQ bit was unchecked by wnwen@chromium.org

[Peter Wen, 2016-12-13 21:49:12]

The CQ bit was checked by wnwen@chromium.org

[Peter Wen, 2016-12-13 21:49:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2562643004/#ps100001
(title: "Fix comments.")

[Peter Wen, 2016-12-13 21:49:43]

Thanks for the reviews, Ian and Maria!

https://codereview.chromium.org/2562643004/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java
(right):

https://codereview.chromium.org/2562643004/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/BackgroundSyncLauncher.java:30:
* {@link SharedPreferences} before it is used here.
On 2016/12/13 21:06:23, Maria wrote:
> I think you should put this comment in shouldLaunchBrowserIfStopped. This is
> implementation detail for that method, rather than for the class.

Done.

[commit-bot: I haz the power, 2016-12-13 21:49:49]

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-12-13 23:54:49]

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

[commit-bot: I haz the power, 2016-12-13 23:54:50]

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

[Peter Wen, 2016-12-14 17:41:03]

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

[commit-bot: I haz the power, 2016-12-14 17:41:25]

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-12-14 18:13:41]

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

[commit-bot: I haz the power, 2016-12-14 18:13:42]

Dry run: This issue passed the CQ dry run.

[Peter Wen, 2016-12-14 18:56:59]

The CQ bit was checked by wnwen@chromium.org

[Peter Wen, 2016-12-14 18:56:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
iclelland@chromium.org, mariakhomenko@chromium.org
Link to the patchset: https://codereview.chromium.org/2562643004/#ps120001
(title: "Fix tests relying on serial executor.")

[commit-bot: I haz the power, 2016-12-14 18:57:51]

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-12-14 19:11:17]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1481741819126620,
"parent_rev": "7c494023b2caa9a7aff33d9bb5b456a49c85fe21", "commit_rev":
"e7da7097d84f3b7ae9c4bfaccd193c283f01c8fa"}

[commit-bot: I haz the power, 2016-12-14 19:11:45]

Description was changed from

==========
Android: Switch to thread pool executor.

Switches BackgroundSyncLauncher to the thread pool executor and add
comments explaining why AsyncTask is necessary.

BUG=601053
==========

to

==========
Android: Switch to thread pool executor.

Switches BackgroundSyncLauncher to the thread pool executor and add
comments explaining why AsyncTask is necessary.

BUG=601053

Review-Url: https://codereview.chromium.org/2562643004
==========

[commit-bot: I haz the power, 2016-12-14 19:11:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-14 19:14:14]

Description was changed from

==========
Android: Switch to thread pool executor.

Switches BackgroundSyncLauncher to the thread pool executor and add
comments explaining why AsyncTask is necessary.

BUG=601053

Review-Url: https://codereview.chromium.org/2562643004
==========

to

==========
Android: Switch to thread pool executor.

Switches BackgroundSyncLauncher to the thread pool executor and add
comments explaining why AsyncTask is necessary.

BUG=601053

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

[commit-bot: I haz the power, 2016-12-14 19:14:15]

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

[Ted C, 2016-12-14 21:37:52]

On 2016/12/14 19:14:15, commit-bot: I haz the power wrote:
> Patchset 7 (id:??) landed as
> https://crrev.com/ad3ee42510afda09e00cb73c694bfa4626980d1d
> Cr-Commit-Position: refs/heads/master@{#438573}

testNewLauncherDisablesNextOnline just failed for me on the try jobs:

https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...

[Peter Wen, 2016-12-14 21:54:06]

On 2016/12/14 21:37:52, Ted C wrote:
> On 2016/12/14 19:14:15, commit-bot: I haz the power wrote:
> > Patchset 7 (id:??) landed as
> > https://crrev.com/ad3ee42510afda09e00cb73c694bfa4626980d1d
> > Cr-Commit-Position: refs/heads/master@{#438573}
> 
> testNewLauncherDisablesNextOnline just failed for me on the try jobs:
> 
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...

Testing it now.

[Peter Wen, 2016-12-14 22:00:06]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2580503002/ by wnwen@chromium.org.

The reason for reverting is:
BackgroundSyncLauncherTest#testNewLauncherDisablesNextOnline fails..