Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2164503003:
[OfflinePages] Do not start background loading on low-end devices if app has any visible Activities. (Closed)
|
Created:
4 years, 5 months ago by dougarnett Modified:
4 years, 4 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[OfflinePages] Do not start background loading on low-end devices if app has any visible Activities.
BUG=627884, 622508
Committed: https://crrev.com/6dd8c1ec9f9c9339676e83374eb78c627fc4aa1e
Cr-Commit-Position: refs/heads/master@{#406864}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added comment for setting up fake running activity #Patch Set 3 : Resets the SysUtils and ApplicationStatus static state so it does not leak to other robolectric tes… #
Total comments: 4
Messages
Total messages: 29 (15 generated)
The CQ bit was checked by dougarnett@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...
dougarnett@chromium.org changed reviewers: + petewil@chromium.org
LGTM with suggestion https://codereview.chromium.org/2164503003/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java (right): https://codereview.chromium.org/2164503003/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:156: Activity testActivity = new Activity(); Maybe add a comment here: // Create another activity on the device, and use test methods to simulate it being in a started state.
https://codereview.chromium.org/2164503003/diff/1/chrome/android/junit/src/or... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java (right): https://codereview.chromium.org/2164503003/diff/1/chrome/android/junit/src/or... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:156: Activity testActivity = new Activity(); On 2016/07/19 21:15:43, Pete Williamson wrote: > Maybe add a comment here: > > // Create another activity on the device, and use test methods to simulate it > being in a started state. Done.
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org Link to the patchset: https://codereview.chromium.org/2164503003/#ps20001 (title: "Added comment for setting up fake running activity")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by dougarnett@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...
Pete, please take another look.
dougarnett@chromium.org changed reviewers: + yfriedman@chromium.org
Yaron, can you review simple change here to reset SysUtils cached state for low-end device? Also, happy to have you look at rest of CL.
https://codereview.chromium.org/2164503003/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/SysUtils.java (right): https://codereview.chromium.org/2164503003/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/SysUtils.java:107: sLowEndDevice = null; Should reset be calling detectLowEndDevice() instead of just assuming it isn't? It seems that after our test, we want to restore the state that existed before our test, not just clear the Low End Device state.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2164503003/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/SysUtils.java (right): https://codereview.chromium.org/2164503003/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/SysUtils.java:107: sLowEndDevice = null; On 2016/07/20 18:29:59, Pete Williamson wrote: > Should reset be calling detectLowEndDevice() instead of just assuming it isn't? > It seems that after our test, we want to restore the state that existed before > our test, not just clear the Low End Device state. true, but it's sorta moot. The next call to isLowEndDevice will ensure it's set
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org Link to the patchset: https://codereview.chromium.org/2164503003/#ps40001 (title: "Resets the SysUtils and ApplicationStatus static state so it does not leak to other robolectric tes…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [OfflinePages] Do not start background loading on low-end devices if app has any visible Activities. BUG=627884, 622508 ========== to ========== [OfflinePages] Do not start background loading on low-end devices if app has any visible Activities. BUG=627884, 622508 Committed: https://crrev.com/6dd8c1ec9f9c9339676e83374eb78c627fc4aa1e Cr-Commit-Position: refs/heads/master@{#406864} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/6dd8c1ec9f9c9339676e83374eb78c627fc4aa1e Cr-Commit-Position: refs/heads/master@{#406864}
Message was sent while issue was closed.
pasko@chromium.org changed reviewers: + pasko@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2164503003/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java (right): https://codereview.chromium.org/2164503003/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:86: CommandLine.init(new String[] {"testcommand", IS_LOW_END_DEVICE_SWITCH}); did you not choose @CommandLineFlags.Add(BaseSwitches.ENABLE_LOW_END_DEVICE_MODE) because you explicitly wanted to run all tests as low end device?
Message was sent while issue was closed.
https://codereview.chromium.org/2164503003/diff/40001/chrome/android/junit/sr... File chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java (right): https://codereview.chromium.org/2164503003/diff/40001/chrome/android/junit/sr... chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/BackgroundOfflinerTaskTest.java:86: CommandLine.init(new String[] {"testcommand", IS_LOW_END_DEVICE_SWITCH}); On 2016/07/29 09:58:17, pasko wrote: > did you not choose > @CommandLineFlags.Add(BaseSwitches.ENABLE_LOW_END_DEVICE_MODE) because you > explicitly wanted to run all tests as low end device? This was a bit interesting. I did try that annotation initially without getting the desired effect. I did get it to work with init() but later learned there is an issue with SysUtils caching the value it first calculates - so whichever test runs first set the flag and subsequent tests don't actually look at the command switch, just use the cached value. I added a SysUtils.reset() method and now call it it tearDown to isolate the command switch interpretation. Since figuring that out I have not gone back to try the annotation instead of init(). I should do that and see if it looks better. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
