Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
854843002:
Upstream WarmupManager. (Closed)
DescriptionUpstream WarmupManager.
BUG=449163
Committed: https://crrev.com/7db4b2e2e665aa7cab01c685cc3b69a588ba9e51
Cr-Commit-Position: refs/heads/master@{#315300}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Change the copyright year (address dtrainor's comment). #
Total comments: 10
Patch Set 3 : Address nyquist's comments. #Patch Set 4 : . #Messages
Total messages: 14 (3 generated)
lizeb@chromium.org changed reviewers: + yusufo@chromium.org
Hello, A simple CL upstreaming WarmupManager.
lgtm
lizeb@chromium.org changed reviewers: + dtrainor@chromium.org, nyquist@chromium.org
Hello, Simply upstreaming a small class. dtrainor@chromium.org: Please review changes in nyquist@chromium.org: Please review changes in
lgtm https://chromiumcodereview.appspot.com/854843002/diff/1/chrome/android/java/s... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://chromiumcodereview.appspot.com/854843002/diff/1/chrome/android/java/s... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2015
https://chromiumcodereview.appspot.com/854843002/diff/1/chrome/android/java/s... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://chromiumcodereview.appspot.com/854843002/diff/1/chrome/android/java/s... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/01/21 08:13:26, David Trainor wrote: > 2015 Done. Thank you.
https://codereview.chromium.org/854843002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/854843002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:21: public class WarmupManager { Nit: Since this class has a private constructor, should this class be final? https://codereview.chromium.org/854843002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:33: if (sWarmupManager == null) sWarmupManager = new WarmupManager(); Which thread should this be called from? This does not seem thread safe, so maybe add an assert that it is only accessed from the main thread? And if so, document that behavior. https://codereview.chromium.org/854843002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:46: public boolean hasPrerenderedUrl(String url) { Nit: Is there a reason why this does not take in the profile as a parameter? And if so, could you mention that the last used profile is used for this? https://codereview.chromium.org/854843002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:76: public void prerenderUrl(final String url, final String referrer, Nit: Same as above regarding profile. https://codereview.chromium.org/854843002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:91: * @param themeId These documentation lines don't really help.
Thank you for the review. All done. https://codereview.chromium.org/854843002/diff/20001/chrome/android/java/src/... File chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java (right): https://codereview.chromium.org/854843002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:21: public class WarmupManager { On 2015/01/21 22:15:21, nyquist wrote: > Nit: Since this class has a private constructor, should this class be final? Done. https://codereview.chromium.org/854843002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:33: if (sWarmupManager == null) sWarmupManager = new WarmupManager(); On 2015/01/21 22:15:20, nyquist wrote: > Which thread should this be called from? This does not seem thread safe, so > maybe add an assert that it is only accessed from the main thread? And if so, > document that behavior. Done. https://codereview.chromium.org/854843002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:46: public boolean hasPrerenderedUrl(String url) { On 2015/01/21 22:15:21, nyquist wrote: > Nit: Is there a reason why this does not take in the profile as a parameter? And > if so, could you mention that the last used profile is used for this? Done. https://codereview.chromium.org/854843002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:76: public void prerenderUrl(final String url, final String referrer, On 2015/01/21 22:15:20, nyquist wrote: > Nit: Same as above regarding profile. Done. https://codereview.chromium.org/854843002/diff/20001/chrome/android/java/src/... chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java:91: * @param themeId On 2015/01/21 22:15:21, nyquist wrote: > These documentation lines don't really help. Done.
lgtm
The CQ bit was checked by lizeb@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/854843002/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/7db4b2e2e665aa7cab01c685cc3b69a588ba9e51 Cr-Commit-Position: refs/heads/master@{#315300} |
