Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2858893002:
Move final part of initialization tasks to ProcessInitializationHandler (Closed)
DescriptionMove final part of initialization tasks to ProcessInitializationHandler
This CL finishes initialization task movement started by
http://crrev.com/2853513002 and http://crrev.com/2852193003.
BUG=716454
Review-Url: https://codereview.chromium.org/2858893002
Cr-Commit-Position: refs/heads/master@{#469613}
Committed: https://chromium.googlesource.com/chromium/src/+/5b08619bdacb059f9523b14a5ed84dddf33e9275
Patch Set 1 #
Total comments: 9
Patch Set 2 : Addressed comments #
Messages
Total messages: 25 (16 generated)
The CQ bit was checked by bsazonov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bsazonov@chromium.org changed reviewers: + tedchoc@chromium.org
Ted, please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Patchset #1 (id:1) has been deleted
https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (right): https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:120: @SuppressFBWarnings("LI_LAZY_INIT_STATIC") Strangely, this change triggered FindBugs error for getInstance(): https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan..., so I had to add this suppression. Is there any better way to fix this?
https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:133: RecordHistogram.recordLongTimesHistogram("UMA.Debug.EnableCrashUpload.DeferredStartUptime2", I think we should move this into ProcessInitializationHandler as well. And if we do that, should we just remove this method altogether and have the one caller just call directly into ProcessInitializationHandler? https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (right): https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:120: @SuppressFBWarnings("LI_LAZY_INIT_STATIC") On 2017/05/03 13:32:10, bsazonov wrote: > Strangely, this change triggered FindBugs error for getInstance(): > https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan..., > so I had to add this suppression. Is there any better way to fix this? Nah, this should be fine. From: http://findbugs.sourceforge.net/bugDescriptions.html """ This method contains an unsynchronized lazy initialization of a non-volatile static field. Because the compiler or processor may reorder instructions, threads are not guaranteed to see a completely initialized object, if the method can be called by multiple threads. You can make the field volatile to correct the problem. For more information, see the Java Memory Model web site. """ Since we check that this must be called on the UI thread, we don't need to do any synchronization here. https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:398: this looks like a unnecessary blank line https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:400: .executeOnExecutor(AsyncTask.SERIAL_EXECUTOR); sigh...https://bugs.llvm.org/show_bug.cgi?id=32627
https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:133: RecordHistogram.recordLongTimesHistogram("UMA.Debug.EnableCrashUpload.DeferredStartUptime2", On 2017/05/03 15:53:19, Ted C wrote: > I think we should move this into ProcessInitializationHandler as well. > > And if we do that, should we just remove this method altogether and have the one > caller just call directly into ProcessInitializationHandler? I can easily move RecordHistogram.recordLongTimesHistogram to ProcessInitializationHandler. However, I'm not sure what to do with mDeferredStartupInitializedForApp. It is used in queueDeferredTasksOnIdleHandler() to understand when it's time to record deferred startup stats. Managing this flag(s) from ProcessInitializationHandler would look awful as well (especially considering classes derived from ProcessInitializationHandler). https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java (right): https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:398: On 2017/05/03 15:53:20, Ted C wrote: > this looks like a unnecessary blank line Thanks for spotting it! I'll fix it when we decide what to do with mDeferredStartupInitializedForApp, if you don't mind. https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java:400: .executeOnExecutor(AsyncTask.SERIAL_EXECUTOR); On 2017/05/03 15:53:20, Ted C wrote: > sigh...https://bugs.llvm.org/show_bug.cgi?id=32627 Yeah, this is annoying. I can fix this manually, but I don't think it's a good approach. WDYT?
https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2858893002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:133: RecordHistogram.recordLongTimesHistogram("UMA.Debug.EnableCrashUpload.DeferredStartUptime2", On 2017/05/03 19:20:12, bsazonov wrote: > On 2017/05/03 15:53:19, Ted C wrote: > > I think we should move this into ProcessInitializationHandler as well. > > > > And if we do that, should we just remove this method altogether and have the > one > > caller just call directly into ProcessInitializationHandler? > > I can easily move RecordHistogram.recordLongTimesHistogram to > ProcessInitializationHandler. However, I'm not sure what to do with > mDeferredStartupInitializedForApp. It is used in > queueDeferredTasksOnIdleHandler() to understand when it's time to record > deferred startup stats. Managing this flag(s) from ProcessInitializationHandler > would look awful as well (especially considering classes derived from > ProcessInitializationHandler). I think you could just expose and return mInitializedDeferredStartupTasks from ProcessInitializationHandler. That "should" be the same.
The CQ bit was checked by bsazonov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bsazonov@chromium.org to run a CQ dry run
Patchset #2 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hm, it turns out that mDeferredStartupInitializedForApp is redundant - queueDeferredTasksOnIdleHandler() call should be made after adding deferred startup tasks, so this call itself is a signal that startup tasks were added. That's good, because it breaks circular dependency between ProcessInitializationHandler and DeferredStartupHandler.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by bsazonov@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493970219362050,
"parent_rev": "f1ffc8932b93b6dca10055638cb08460f4752fed", "commit_rev":
"5b08619bdacb059f9523b14a5ed84dddf33e9275"}
Message was sent while issue was closed.
Description was changed from ========== Move final part of initialization tasks to ProcessInitializationHandler This CL finishes initialization task movement started by http://crrev.com/2853513002 and http://crrev.com/2852193003. BUG=716454 ========== to ========== Move final part of initialization tasks to ProcessInitializationHandler This CL finishes initialization task movement started by http://crrev.com/2853513002 and http://crrev.com/2852193003. BUG=716454 Review-Url: https://codereview.chromium.org/2858893002 Cr-Commit-Position: refs/heads/master@{#469613} Committed: https://chromium.googlesource.com/chromium/src/+/5b08619bdacb059f9523b14a5ed8... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/5b08619bdacb059f9523b14a5ed8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
