Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
933343002:
Add a callback mechanism for resource extraction completion on Android. (Closed)
DescriptionAdd a callback mechanism for resource extraction completion on Android.
BUG=459642
Committed: https://crrev.com/eb4e9ac588294e6b88c63b3bd015e92ce34006d9
Cr-Commit-Position: refs/heads/master@{#316972}
Patch Set 1 #Patch Set 2 : Fix null callback case for prepareToStartBrowserProcess #
Total comments: 15
Patch Set 3 : Address nyquist concerns #Patch Set 4 : Fix tests, tweak logic slightly. #
Messages
Total messages: 18 (7 generated)
aruslan@chromium.org changed reviewers: + aruslan@chromium.org
lgtm with comments. https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java (right): https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:292: ContentMain.initApplicationContext(appContext); I, for one, welcome our failing/crashing dependencies. https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:296: if (completionCallback != null) completionCallback.run(); you don't need one of these "if"s, either here https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:300: if (completionCallback == null) { or here. Also, let's never do this synchronously!
tedchoc@chromium.org changed reviewers: + nyquist@chromium.org
@nyquist - owners PTAL https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java (right): https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:300: if (completionCallback == null) { On 2015/02/18 18:40:25, aruslan wrote: > or here. > Also, let's never do this synchronously! Sadly, there are still too many places that call startBrowserProcessSync above. So until we can switch everyone to supporting an async approach I think we are stuck with it. The if in the runnable above also allows us to share the initialization logic with the two paths below. And I did have it initially like: if (completionCallback == null) resourceExtractor.waitForCompletion(); resourceExtractor.addCompletionCallback(postResourceExtraction); But waitForCompletion doesn't actually result in the callback == finished in ResourceExtractor, so the completion callback becomes async by one UI task therefore breaking the sync case.
https://codereview.chromium.org/933343002/diff/20001/base/android/java/src/or... File base/android/java/src/org/chromium/base/ResourceExtractor.java (right): https://codereview.chromium.org/933343002/diff/20001/base/android/java/src/or... base/android/java/src/org/chromium/base/ResourceExtractor.java:60: private List<Runnable> mCompletionCallbacks = new ArrayList<Runnable>(); Nit: This reference is never changed, so add |final|. https://codereview.chromium.org/933343002/diff/20001/base/android/java/src/or... base/android/java/src/org/chromium/base/ResourceExtractor.java:398: if (shouldSkipPakExtraction()) { This implies that all parts of the embedders would know whether pak extraction should be executed or not. Is there a specific reason why this doesn't just post a callback that it's finished? If not; maybe add this early bail-out to the JavaDoc? https://codereview.chromium.org/933343002/diff/20001/base/android/java/src/or... base/android/java/src/org/chromium/base/ResourceExtractor.java:405: callback.run(); Is there a reason why this callback is run directly instead of being posted back? I find that an odd interaction model. In the other class you're editing, BrowserStartupController, we do similar things, but all callbacks are posted (postStartupCompleted()). https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java (right): https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:291: Context appContext = mContext.getApplicationContext(); Nit: Only in tests is this not the application context already. For this class, maybe we can do this in the private constructor instead of in the factory. That way you could remove this awkward line. https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:292: ContentMain.initApplicationContext(appContext); On 2015/02/18 18:40:25, aruslan wrote: > I, for one, welcome our failing/crashing dependencies. To me it looks like the ordering of these operations would be the same both with and without the completionCallback? https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:296: if (completionCallback != null) completionCallback.run(); On 2015/02/18 18:40:25, aruslan wrote: > you don't need one of these "if"s, either here The way the code is organized now at least, this Runnable will be called in both cases (null and !=null); because ContentMain needs to be initialized and the command line stuffs. Were you proposing a different organization?
https://codereview.chromium.org/933343002/diff/20001/base/android/java/src/or... File base/android/java/src/org/chromium/base/ResourceExtractor.java (right): https://codereview.chromium.org/933343002/diff/20001/base/android/java/src/or... base/android/java/src/org/chromium/base/ResourceExtractor.java:60: private List<Runnable> mCompletionCallbacks = new ArrayList<Runnable>(); On 2015/02/18 20:25:51, nyquist wrote: > Nit: This reference is never changed, so add |final|. Done. https://codereview.chromium.org/933343002/diff/20001/base/android/java/src/or... base/android/java/src/org/chromium/base/ResourceExtractor.java:398: if (shouldSkipPakExtraction()) { On 2015/02/18 20:25:52, nyquist wrote: > This implies that all parts of the embedders would know whether pak extraction > should be executed or not. Is there a specific reason why this doesn't just post > a callback that it's finished? > > If not; maybe add this early bail-out to the JavaDoc? Changed it to post the task in this case. https://codereview.chromium.org/933343002/diff/20001/base/android/java/src/or... base/android/java/src/org/chromium/base/ResourceExtractor.java:405: callback.run(); On 2015/02/18 20:25:51, nyquist wrote: > Is there a reason why this callback is run directly instead of being posted > back? I find that an odd interaction model. In the other class you're editing, > BrowserStartupController, we do similar things, but all callbacks are posted > (postStartupCompleted()). Done...I was on the fence and didn't particular care either way. https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java (right): https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:291: Context appContext = mContext.getApplicationContext(); On 2015/02/18 20:25:52, nyquist wrote: > Nit: Only in tests is this not the application context already. For this class, > maybe we can do this in the private constructor instead of in the factory. That > way you could remove this awkward line. I actually just changed it so that mContext is always the application context in the constructor of this class instead of changing the test...that way it will always be the application. This "seems" to address your concern?
New patchsets have been uploaded after l-g-t-m from aruslan@chromium.org
lgtm thanks! https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java (right): https://codereview.chromium.org/933343002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:291: Context appContext = mContext.getApplicationContext(); On 2015/02/18 21:27:51, Ted C wrote: > On 2015/02/18 20:25:52, nyquist wrote: > > Nit: Only in tests is this not the application context already. For this > class, > > maybe we can do this in the private constructor instead of in the factory. > That > > way you could remove this awkward line. > > I actually just changed it so that mContext is always the application context in > the constructor of this class instead of changing the test...that way it will > always be the application. > > This "seems" to address your concern? Yes. Sorry if I was unclear. Doing this in the constructor is great.
The CQ bit was checked by tedchoc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/933343002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_rel_tests_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_rel_tes...)
New patchsets have been uploaded after l-g-t-m from nyquist@chromium.org
still lgtm for patch set 4
The CQ bit was checked by tedchoc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/933343002/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/eb4e9ac588294e6b88c63b3bd015e92ce34006d9 Cr-Commit-Position: refs/heads/master@{#316972} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
