Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2420503002:
[Offline Pages] Define separate watchdog timeout for concurrent bg loads (Closed)
|
Created:
4 years, 2 months ago by dougarnett Modified:
4 years, 2 months ago Reviewers:
Pete Williamson 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Define separate watchdog timeout for concurrent bg loads
BUG=655341
Committed: https://crrev.com/8bc69612087b58e7a679996a2f3e2f60f4f52519
Cr-Commit-Position: refs/heads/master@{#425426}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Trying enum state instead of bool parm #Patch Set 3 : Merge #
Total comments: 6
Patch Set 4 : Added unit test case and dropped test method for setting timeout value #Patch Set 5 : Cleaned up 16 lint errors from "git cl lint" #
Messages
Total messages: 26 (18 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
Pete, interested in an initial pass on the approach here (still need to add tests).
https://codereview.chromium.org/2420503002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_scheduler_bridge.cc (right): https://codereview.chromium.org/2420503002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_scheduler_bridge.cc:55: true /* is_background_scheduled */, device_conditions, Let's not use a binary argument, they are hard to understand at the point of the function call. Perhaps an enum, or make const values indicating the meaning and assign the values to true or false, using the value instead of true here. https://codereview.chromium.org/2420503002/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2420503002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:13: const int kSinglePageTimeLimitForConcurrentLoadSeconds = 300; I wonder if we can improve the naming here a bit more. It isn't really concurrent (at least not once Dimich's change goes in). It isn't really foreground, it is technically a background load when chrome is in the foreground. Maybe kSinglePageTimeLimitForImmediateBackgroundLoadSeconds ? https://codereview.chromium.org/2420503002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:91: int GetSinglePageTimeLimitInSeconds(bool is_background_scheduled) const { maybe rename the variable to indicate scheduled_from_gcm or scheduled_from_tab_helper https://codereview.chromium.org/2420503002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2420503002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:418: offliner_timeout_ = base::TimeDelta::FromSeconds( It's a bit fragile to have this changing constantly. I wonder if there is a way to tie it to the currently executing request. (I can't think of any offhand, but if you can, it would be great).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Backed out the bool arg and trying internal enum processing state that replaces is_stopped and identifies the non stopped processing types. https://codereview.chromium.org/2420503002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/background_scheduler_bridge.cc (right): https://codereview.chromium.org/2420503002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/background_scheduler_bridge.cc:55: true /* is_background_scheduled */, device_conditions, On 2016/10/12 22:33:24, Pete Williamson wrote: > Let's not use a binary argument, they are hard to understand at the point of the > function call. Perhaps an enum, or make const values indicating the meaning and > assign the values to true or false, using the value instead of true here. ok, trying enum internally instead and reverting public api https://codereview.chromium.org/2420503002/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2420503002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:13: const int kSinglePageTimeLimitForConcurrentLoadSeconds = 300; On 2016/10/12 22:33:24, Pete Williamson wrote: > I wonder if we can improve the naming here a bit more. > > It isn't really concurrent (at least not once Dimich's change goes in). > It isn't really foreground, it is technically a background load when chrome is > in the foreground. > > Maybe kSinglePageTimeLimitForImmediateBackgroundLoadSeconds ? Done. https://codereview.chromium.org/2420503002/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:91: int GetSinglePageTimeLimitInSeconds(bool is_background_scheduled) const { On 2016/10/12 22:33:24, Pete Williamson wrote: > maybe rename the variable to indicate scheduled_from_gcm or > scheduled_from_tab_helper trying separate methods https://codereview.chromium.org/2420503002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2420503002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:418: offliner_timeout_ = base::TimeDelta::FromSeconds( On 2016/10/12 22:33:24, Pete Williamson wrote: > It's a bit fragile to have this changing constantly. I wonder if there is a way > to tie it to the currently executing request. (I can't think of any offhand, > but if you can, it would be great). It is really tied to the service window though - not a single request - unless we change somehow to make immediate triggering only apply to one request (which might be an interesting propsect wrt user control over ordering). Another option would be to replace this timeout member with a processing state and then call the policy object when setting the watchdog. Trying that.
lgtm with nit. https://codereview.chromium.org/2420503002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2420503002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:562: // policy_->GetSinglePageTimeLimitInSeconds(is_background_scheduled)); Remove commented out code before checkin https://codereview.chromium.org/2420503002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:572: watchdog_timer_.Start(FROM_HERE, timeout, this, Nice, much less fragile, I like it. https://codereview.chromium.org/2420503002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2420503002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.h:334: ProcessingWindowState processing_state_; Good change, I like this a lot better.
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...
Added a unit test and did some clean up (got rid of test method for setting timeout and also some lint I found running git cl lint). https://codereview.chromium.org/2420503002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2420503002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:562: // policy_->GetSinglePageTimeLimitInSeconds(is_background_scheduled)); On 2016/10/13 22:58:17, Pete Williamson wrote: > Remove commented out code before checkin Done. https://codereview.chromium.org/2420503002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:572: watchdog_timer_.Start(FROM_HERE, timeout, this, On 2016/10/13 22:58:18, Pete Williamson wrote: > Nice, much less fragile, I like it. Acknowledged. https://codereview.chromium.org/2420503002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2420503002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.h:334: ProcessingWindowState processing_state_; On 2016/10/13 22:58:18, Pete Williamson wrote: > Good change, I like this a lot better. Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/2420503002/#ps80001 (title: "Cleaned up 16 lint errors from "git cl lint"")
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.
Description was changed from ========== [Offline Pages] Define separate watchdog timeout for concurrent bg loads BUG=655341 ========== to ========== [Offline Pages] Define separate watchdog timeout for concurrent bg loads BUG=655341 Committed: https://crrev.com/8bc69612087b58e7a679996a2f3e2f60f4f52519 Cr-Commit-Position: refs/heads/master@{#425426} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8bc69612087b58e7a679996a2f3e2f60f4f52519 Cr-Commit-Position: refs/heads/master@{#425426} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
