Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1288593003:
[BackgroundSync] Move read and write of shared preferences to an AsyncTask (Closed)
Description[BackgroundSync] Move read and write of shared preferences to an AsyncTask
BUG=520105
Committed: https://crrev.com/3fd1c36bf4c6cf03586c74bf95d051787d7c86b2
Cr-Commit-Position: refs/heads/master@{#345118}
Patch Set 1 #Patch Set 2 : Clean up #
Total comments: 8
Patch Set 3 : Early return #
Total comments: 2
Patch Set 4 : Sync as suffix instead of prefix #
Messages
Total messages: 34 (12 generated)
jkarlin@chromium.org changed reviewers: + jdduke@chromium.org
jdduke: PTAL at everything, thanks!
jdduke@chromium.org changed reviewers: + yfriedman@chromium.org
Looks quite reasonable, though yriedman@ is probably a better reviewer here.
On 2015/08/19 17:29:05, jdduke wrote: > Looks quite reasonable, though yriedman@ is probably a better reviewer here. Yaron is OOO, do you suggest any other reviewers? Thanks!
Just a couple questions. https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java (right): https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:74: SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context); Does getting the SharedPreferences cause a violation? Or getting/setting values via the SharedPreferences? Or both? https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/BackgroundSyncLauncherTest.java (right): https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/BackgroundSyncLauncherTest.java:115: mLauncher.setLaunchWhenNextOnline(mContext, true); Are AsyncTasks all serviced by the same worker thread? Or is there otherwise any guarantees about ordering when launched from the same thread?
https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java (right): https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncher.java:74: SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context); On 2015/08/19 17:32:42, jdduke wrote: > Does getting the SharedPreferences cause a violation? Or getting/setting values > via the SharedPreferences? Or both? It turns out both getting and setting can block. https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/BackgroundSyncLauncherTest.java (right): https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/BackgroundSyncLauncherTest.java:115: mLauncher.setLaunchWhenNextOnline(mContext, true); On 2015/08/19 17:32:42, jdduke wrote: > Are AsyncTasks all serviced by the same worker thread? Or is there otherwise any > guarantees about ordering when launched from the same thread? Yep! "When first introduced, AsyncTasks were executed serially on a single background thread. Starting with DONUT, this was changed to a pool of threads allowing multiple tasks to operate in parallel. Starting with HONEYCOMB, tasks are executed on a single thread to avoid common application errors caused by parallel execution."
https://codereview.chromium.org/1288593003/diff/20001/content/browser/android... File content/browser/android/background_sync_launcher_android.cc (right): https://codereview.chromium.org/1288593003/diff/20001/content/browser/android... content/browser/android/background_sync_launcher_android.cc:49: env, java_launcher_.obj(), base::android::GetApplicationContext(), Hmm, is this code executed on WebView? I don't think we want to be using the global context here.
https://codereview.chromium.org/1288593003/diff/20001/content/browser/android... File content/browser/android/background_sync_launcher_android.cc (right): https://codereview.chromium.org/1288593003/diff/20001/content/browser/android... content/browser/android/background_sync_launcher_android.cc:49: env, java_launcher_.obj(), base::android::GetApplicationContext(), On 2015/08/19 17:45:14, jdduke wrote: > Hmm, is this code executed on WebView? I don't think we want to be using the > global context here. I'm not sure, so I've created a bug. Either way that's not new to this CL as the BackgroundSyncLauncherAndroid() call below uses the app context as well. https://code.google.com/p/chromium/issues/detail?id=522596
jdduke@chromium.org changed reviewers: + peter@chromium.org - yfriedman@chromium.org
Looks good but technically I'm only an owner for input-related changes. I'll rubberstamp with peter@'s approval.
Peter, PTAL, many thanks!
jkarlin@chromium.org changed reviewers: + mlamouri@chromium.org - peter@chromium.org
On 2015/08/20 11:18:29, jkarlin wrote: > Peter, PTAL, many thanks! Ah, Peter is out as well. Adding Mounir. PTAL Mounir, thanks!
lgtm https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncherService.java (right): https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncherService.java:87: if (shouldLaunch) { nit: if (!shouldLaunch) return;
Thanks Mounir. jdduke: What do you think? https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncherService.java (right): https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncherService.java:87: if (shouldLaunch) { On 2015/08/21 09:40:54, Mounir Lamouri wrote: > nit: > if (!shouldLaunch) return; Done.
On 2015/08/21 12:03:12, jkarlin wrote: > Thanks Mounir. jdduke: What do you think? > > https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncherService.java > (right): > > https://codereview.chromium.org/1288593003/diff/20001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/BackgroundSyncLauncherService.java:87: > if (shouldLaunch) { > On 2015/08/21 09:40:54, Mounir Lamouri wrote: > > nit: > > if (!shouldLaunch) return; > > Done. Lgtm
The CQ bit was checked by jkarlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1288593003/#ps40001 (title: "Early return")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288593003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288593003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by jkarlin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288593003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288593003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
peter@chromium.org changed reviewers: + peter@chromium.org
lgtm with an optional comment https://codereview.chromium.org/1288593003/diff/40001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/BackgroundSyncLauncherTest.java (right): https://codereview.chromium.org/1288593003/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/BackgroundSyncLauncherTest.java:72: private Boolean syncShouldLaunchWhenNextOnline() { micro nit: We usually have *Sync as a suffix. When using it as a prefix, like here, it might tell the reader that it's synchronizing the flag rather than getting it synchronously.
Thanks! https://codereview.chromium.org/1288593003/diff/40001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/BackgroundSyncLauncherTest.java (right): https://codereview.chromium.org/1288593003/diff/40001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/BackgroundSyncLauncherTest.java:72: private Boolean syncShouldLaunchWhenNextOnline() { On 2015/08/24 13:21:20, Peter Beverloo wrote: > micro nit: We usually have *Sync as a suffix. When using it as a prefix, like > here, it might tell the reader that it's synchronizing the flag rather than > getting it synchronously. Done.
The CQ bit was checked by jkarlin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, jdduke@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1288593003/#ps60001 (title: "Sync as suffix instead of prefix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288593003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288593003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3fd1c36bf4c6cf03586c74bf95d051787d7c86b2 Cr-Commit-Position: refs/heads/master@{#345118} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
