Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2149773002:
PrerenderingOffliner will abort background load if it sees chrome transitioning to foreground on lo… (Closed)
|
Created:
4 years, 5 months ago by dougarnett Modified:
4 years, 5 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, romax+watch_chromium.org, tburkard+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, gavinp+prer_chromium.org, asvitkine+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. |
DescriptionPrerenderingOffliner will abort background load if it sees chrome transitioning to foreground on low-end devices.
Also adds new FOREGROUND_CANCELED request status (and histogram enum value).
BUG=627884, 622508
Committed: https://crrev.com/93ec7def5cee33305155e15131c439d23ddceb0d
Cr-Commit-Position: refs/heads/master@{#406565}
Patch Set 1 #Patch Set 2 : Trying to drop the class files that got uploaded #Patch Set 3 : Nutha try #
Total comments: 15
Patch Set 4 : Addressed first round of feedback #
Total comments: 2
Patch Set 5 : Revise some comments wrt app listener setting/clearing #
Messages
Total messages: 36 (17 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: + fgorski@chromium.org, pasko@chromium.org, petewil@chromium.org
Fyi, I verified this change manually on low-end device. I wasn't able to run the unit tests on that device though so actually exercising new unit test on low-end device should get picked up by Lollipop Low-end Tester after submission.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:91: app_listener_.reset(nullptr); Why do we reset for both SavePage done and LoadPageDone? Is it because we don't always get LoadPageDone? https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:134: app_listener_.reset(new base::android::ApplicationStatusListener( Would it be better to always set up a listener, even on high end devices? I can see us wanting to use several signals, and even cancel background loads on high end devices sometimes. https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc (right): https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:341: if (base::SysInfo::IsLowEndDevice()) { This seems a bit problematic, the build bots may be either all or no low end devices. Instead, is there a way to get base::SysInfo::IsLowEndDevice to return true for your test? Did you test this on both kinds of devices, and verify both paths passed?
https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:91: app_listener_.reset(nullptr); On 2016/07/13 23:17:55, Pete Williamson wrote: > Why do we reset for both SavePage done and LoadPageDone? Is it because we don't > always get LoadPageDone? Paired with clearing pending_request_. Only reset in LoadPageDone for the case that the load did not succeed and there will be no SavePage attempt. https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:134: app_listener_.reset(new base::android::ApplicationStatusListener( On 2016/07/13 23:17:55, Pete Williamson wrote: > Would it be better to always set up a listener, even on high end devices? I can > see us wanting to use several signals, and even cancel background loads on high > end devices sometimes. Had that originally but didn't come up with any use yet in the callback so expected I would get the opposite feedback ;-) I don't have strong opinion either way. https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc (right): https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:341: if (base::SysInfo::IsLowEndDevice()) { On 2016/07/13 23:17:55, Pete Williamson wrote: > This seems a bit problematic, the build bots may be either all or no low end > devices. > > Instead, is there a way to get base::SysInfo::IsLowEndDevice to return true for > your test? > > Did you test this on both kinds of devices, and verify both paths passed? I tried to address this point in advance in original mail message for this CL so I will paste here to see if it answers or was unclear: Fyi, I verified this change manually on low-end device. I wasn't able to run the unit tests on that device though so actually exercising new unit test on low-end device should get picked up by Lollipop Low-end Tester after submission.
https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:134: app_listener_.reset(new base::android::ApplicationStatusListener( On 2016/07/13 23:44:20, dougarnett wrote: > On 2016/07/13 23:17:55, Pete Williamson wrote: > > Would it be better to always set up a listener, even on high end devices? I > can > > see us wanting to use several signals, and even cancel background loads on > high > > end devices sometimes. > > Had that originally but didn't come up with any use yet in the callback so > expected I would get the opposite feedback ;-) I don't have strong opinion > either way. I suppose I would be OK with waiting until we need to use the signal before setting up the app status listener every time. (IE, the code is OK as is). https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc (right): https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:341: if (base::SysInfo::IsLowEndDevice()) { On 2016/07/13 23:44:20, dougarnett wrote: > On 2016/07/13 23:17:55, Pete Williamson wrote: > > This seems a bit problematic, the build bots may be either all or no low end > > devices. > > > > Instead, is there a way to get base::SysInfo::IsLowEndDevice to return true > for > > your test? > > > > Did you test this on both kinds of devices, and verify both paths passed? > > I tried to address this point in advance in original mail message for this CL so > I will paste here to see if it answers or was unclear: > > Fyi, I verified this change manually on low-end device. I wasn't able to run the > unit tests on that device though so actually exercising new unit test on low-end > device should get picked up by Lollipop Low-end Tester after submission. I'd really like to see this tested on both before submission. I have a low end device you may borrow. I'd also like to know if it is possible to mock out the base::SysInfo::IsLowEndDevice() call to return true for our unit test.
https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc (right): https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:341: if (base::SysInfo::IsLowEndDevice()) { On 2016/07/13 23:57:12, Pete Williamson wrote: > On 2016/07/13 23:44:20, dougarnett wrote: > > On 2016/07/13 23:17:55, Pete Williamson wrote: > > > This seems a bit problematic, the build bots may be either all or no low end > > > devices. > > > > > > Instead, is there a way to get base::SysInfo::IsLowEndDevice to return true > > for > > > your test? > > > > > > Did you test this on both kinds of devices, and verify both paths passed? > > > > I tried to address this point in advance in original mail message for this CL > so > > I will paste here to see if it answers or was unclear: > > > > Fyi, I verified this change manually on low-end device. I wasn't able to run > the > > unit tests on that device though so actually exercising new unit test on > low-end > > device should get picked up by Lollipop Low-end Tester after submission. > > I'd really like to see this tested on both before submission. I have a low end > device you may borrow. > > I'd also like to know if it is possible to mock out the > base::SysInfo::IsLowEndDevice() call to return true for our unit test. Acknowledged - I will look to mock the case. Please try to run a unit test on your low-end device and see if it will run it. It would be sweet if we have one that will.
On 2016/07/14 15:29:21, dougarnett wrote: > https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... > File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc > (right): > > https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... > chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:341: if > (base::SysInfo::IsLowEndDevice()) { > On 2016/07/13 23:57:12, Pete Williamson wrote: > > On 2016/07/13 23:44:20, dougarnett wrote: > > > On 2016/07/13 23:17:55, Pete Williamson wrote: > > > > This seems a bit problematic, the build bots may be either all or no low > end > > > > devices. > > > > > > > > Instead, is there a way to get base::SysInfo::IsLowEndDevice to return > true > > > for > > > > your test? > > > > > > > > Did you test this on both kinds of devices, and verify both paths passed? > > > > > > I tried to address this point in advance in original mail message for this > CL > > so > > > I will paste here to see if it answers or was unclear: > > > > > > Fyi, I verified this change manually on low-end device. I wasn't able to run > > the > > > unit tests on that device though so actually exercising new unit test on > > low-end > > > device should get picked up by Lollipop Low-end Tester after submission. > > > > I'd really like to see this tested on both before submission. I have a low > end > > device you may borrow. > > > > I'd also like to know if it is possible to mock out the > > base::SysInfo::IsLowEndDevice() call to return true for our unit test. > > Acknowledged - I will look to mock the case. > > Please try to run a unit test on your low-end device and see if it will run it. > It would be sweet if we have one that will. I have already run unit tests on my low end device, and they worked (I haven't tried this test, though.)
https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:134: app_listener_.reset(new base::android::ApplicationStatusListener( On 2016/07/13 23:57:12, Pete Williamson wrote: > On 2016/07/13 23:44:20, dougarnett wrote: > > On 2016/07/13 23:17:55, Pete Williamson wrote: > > > Would it be better to always set up a listener, even on high end devices? I > > can > > > see us wanting to use several signals, and even cancel background loads on > > high > > > end devices sometimes. > > > > Had that originally but didn't come up with any use yet in the callback so > > expected I would get the opposite feedback ;-) I don't have strong opinion > > either way. > > I suppose I would be OK with waiting until we need to use the signal before > setting up the app status listener every time. (IE, the code is OK as is). I second Pete's initial comment. It is the state change listener that should make checks for low end device or do any other interpretation of the state change information. This code does not need to be conditional on device knowledge. https://codereview.chromium.org/2149773002/diff/40001/components/offline_page... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2149773002/diff/40001/components/offline_page... components/offline_pages/background/offliner.h:31: FOREGROUND_CANCELED = 7, // Foreground transition canceled request. nit: Please align all of the comments.
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...
https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:134: app_listener_.reset(new base::android::ApplicationStatusListener( On 2016/07/14 17:01:45, fgorski wrote: > On 2016/07/13 23:57:12, Pete Williamson wrote: > > On 2016/07/13 23:44:20, dougarnett wrote: > > > On 2016/07/13 23:17:55, Pete Williamson wrote: > > > > Would it be better to always set up a listener, even on high end devices? > I > > > can > > > > see us wanting to use several signals, and even cancel background loads on > > > high > > > > end devices sometimes. > > > > > > Had that originally but didn't come up with any use yet in the callback so > > > expected I would get the opposite feedback ;-) I don't have strong opinion > > > either way. > > > > I suppose I would be OK with waiting until we need to use the signal before > > setting up the app status listener every time. (IE, the code is OK as is). > > I second Pete's initial comment. > It is the state change listener that should make checks for low end device or do > any other interpretation of the state change information. > > This code does not need to be conditional on device knowledge. Done. https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc (right): https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:341: if (base::SysInfo::IsLowEndDevice()) { On 2016/07/14 15:29:21, dougarnett wrote: > On 2016/07/13 23:57:12, Pete Williamson wrote: > > On 2016/07/13 23:44:20, dougarnett wrote: > > > On 2016/07/13 23:17:55, Pete Williamson wrote: > > > > This seems a bit problematic, the build bots may be either all or no low > end > > > > devices. > > > > > > > > Instead, is there a way to get base::SysInfo::IsLowEndDevice to return > true > > > for > > > > your test? > > > > > > > > Did you test this on both kinds of devices, and verify both paths passed? > > > > > > I tried to address this point in advance in original mail message for this > CL > > so > > > I will paste here to see if it answers or was unclear: > > > > > > Fyi, I verified this change manually on low-end device. I wasn't able to run > > the > > > unit tests on that device though so actually exercising new unit test on > > low-end > > > device should get picked up by Lollipop Low-end Tester after submission. > > > > I'd really like to see this tested on both before submission. I have a low > end > > device you may borrow. > > > > I'd also like to know if it is possible to mock out the > > base::SysInfo::IsLowEndDevice() call to return true for our unit test. > > Acknowledged - I will look to mock the case. > > Please try to run a unit test on your low-end device and see if it will run it. > It would be sweet if we have one that will. Done. https://codereview.chromium.org/2149773002/diff/40001/components/offline_page... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2149773002/diff/40001/components/offline_page... components/offline_pages/background/offliner.h:31: FOREGROUND_CANCELED = 7, // Foreground transition canceled request. On 2016/07/14 17:01:45, fgorski wrote: > nit: Please align all of the comments. Unfortunately, this is what 'git cl format' gave me so will be fragile to try to maintain by hand when updated and format run again. Trying another approach - putting comments on preceding line - check if that works better for you?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc (right): https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:341: if (base::SysInfo::IsLowEndDevice()) { On 2016/07/15 16:00:25, dougarnett wrote: > On 2016/07/14 15:29:21, dougarnett wrote: > > On 2016/07/13 23:57:12, Pete Williamson wrote: > > > On 2016/07/13 23:44:20, dougarnett wrote: > > > > On 2016/07/13 23:17:55, Pete Williamson wrote: > > > > > This seems a bit problematic, the build bots may be either all or no low > > end > > > > > devices. > > > > > > > > > > Instead, is there a way to get base::SysInfo::IsLowEndDevice to return > > true > > > > for > > > > > your test? > > > > > > > > > > Did you test this on both kinds of devices, and verify both paths > passed? > > > > > > > > I tried to address this point in advance in original mail message for this > > CL > > > so > > > > I will paste here to see if it answers or was unclear: > > > > > > > > Fyi, I verified this change manually on low-end device. I wasn't able to > run > > > the > > > > unit tests on that device though so actually exercising new unit test on > > > low-end > > > > device should get picked up by Lollipop Low-end Tester after submission. > > > > > > I'd really like to see this tested on both before submission. I have a low > > end > > > device you may borrow. > > > > > > I'd also like to know if it is possible to mock out the > > > base::SysInfo::IsLowEndDevice() call to return true for our unit test. > > > > Acknowledged - I will look to mock the case. > > > > Please try to run a unit test on your low-end device and see if it will run > it. > > It would be sweet if we have one that will. > > Done. I don't see the change to mock, is it uploaded yet? In a different file? Also, once we have the change, we can get rid of the if statement, since we will always be testing on a (simulated) low end device.
On 2016/07/16 00:57:20, Pete Williamson wrote: > https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... > File chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc > (right): > > https://codereview.chromium.org/2149773002/diff/40001/chrome/browser/android/... > chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc:341: if > (base::SysInfo::IsLowEndDevice()) { > On 2016/07/15 16:00:25, dougarnett wrote: > > On 2016/07/14 15:29:21, dougarnett wrote: > > > On 2016/07/13 23:57:12, Pete Williamson wrote: > > > > On 2016/07/13 23:44:20, dougarnett wrote: > > > > > On 2016/07/13 23:17:55, Pete Williamson wrote: > > > > > > This seems a bit problematic, the build bots may be either all or no > low > > > end > > > > > > devices. > > > > > > > > > > > > Instead, is there a way to get base::SysInfo::IsLowEndDevice to return > > > true > > > > > for > > > > > > your test? > > > > > > > > > > > > Did you test this on both kinds of devices, and verify both paths > > passed? > > > > > > > > > > I tried to address this point in advance in original mail message for > this > > > CL > > > > so > > > > > I will paste here to see if it answers or was unclear: > > > > > > > > > > Fyi, I verified this change manually on low-end device. I wasn't able to > > run > > > > the > > > > > unit tests on that device though so actually exercising new unit test on > > > > low-end > > > > > device should get picked up by Lollipop Low-end Tester after submission. > > > > > > > > I'd really like to see this tested on both before submission. I have a > low > > > end > > > > device you may borrow. > > > > > > > > I'd also like to know if it is possible to mock out the > > > > base::SysInfo::IsLowEndDevice() call to return true for our unit test. > > > > > > Acknowledged - I will look to mock the case. > > > > > > Please try to run a unit test on your low-end device and see if it will run > > it. > > > It would be sweet if we have one that will. > > > > Done. > > I don't see the change to mock, is it uploaded yet? In a different file? > Also, once we have the change, we can get rid of the if statement, since we will > always be testing on a (simulated) low end device. It was uploaded. The if was replaced by two separate test cases in that upload. Do you see 3 deltas from patch set?
lgtm. I think I mistakenly clicked on the link for the comment expecting to see the new code. When I went back and clicked on the "3" to see changes since version 3, I did see the code, sorry about that.
lgtm with one small nit. https://codereview.chromium.org/2149773002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2149773002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:70: app_listener_.reset(nullptr); nit: this does not match the comment and is in between commented code. It could happen before the commented block or after.
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
https://codereview.chromium.org/2149773002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2149773002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/prerendering_offliner.cc:70: app_listener_.reset(nullptr); On 2016/07/19 16:09:44, fgorski wrote: > nit: this does not match the comment and is in between commented code. It could > happen before the commented block or after. Revised comments to include.
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: + isherman@chromium.org
Ilya, would you be able to review histograms.xml in this patch?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
histograms.xml lgtm
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, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2149773002/#ps80001 (title: "Revise some comments wrt app listener setting/clearing")
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 #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== PrerenderingOffliner will abort background load if it sees chrome transitioning to foreground on low-end devices. Also adds new FOREGROUND_CANCELED request status (and histogram enum value). BUG=627884,622508 ========== to ========== PrerenderingOffliner will abort background load if it sees chrome transitioning to foreground on low-end devices. Also adds new FOREGROUND_CANCELED request status (and histogram enum value). BUG=627884,622508 Committed: https://crrev.com/93ec7def5cee33305155e15131c439d23ddceb0d Cr-Commit-Position: refs/heads/master@{#406565} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/93ec7def5cee33305155e15131c439d23ddceb0d Cr-Commit-Position: refs/heads/master@{#406565} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
