Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2775223006:
[Offline Pages] Improve RequestCoordinator state tracking. (Closed)
|
Created:
3 years, 9 months ago by romax Modified:
3 years, 8 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/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Improve RequestCoordinator state tracking.
Replace the several boolean values used for state tracking with single
state variable.
BUG=644519
Review-Url: https://codereview.chromium.org/2775223006
Cr-Commit-Position: refs/heads/master@{#460820}
Committed: https://chromium.googlesource.com/chromium/src/+/7be44a4d4b5fbbd8787a57a4ffb705dae78bf7e1
Patch Set 1 #
Total comments: 10
Patch Set 2 : comments. #
Total comments: 6
Patch Set 3 : comments. #Patch Set 4 : minor comment fix. #
Messages
Total messages: 28 (18 generated)
romax@chromium.org changed reviewers: + petewil@chromium.org
Closed the gap mentioned in the doc linked in the bug by moving the state change to OFFLINING from SendRequestToOffliner to RequestPicked. PTAL, thanks!
Looks good, thanks for taking care of this! A few comments https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... components/offline_pages/core/background/request_coordinator.cc:289: if (offliner_ && state_ == RequestCoordinatorState::OFFLINING) { What should we do if the state is STARTING? We probably don't want to go on with the request. Maybe we should check in RequestPicked() to see if a cancel happened while we were picking, and if so, to not then proceed to try the request. https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... components/offline_pages/core/background/request_coordinator.cc:780: if (!SendRequestToOffliner(request)) Why check the return value, and then do the same thing either way? Maybe the else clause should be dropped. https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... File components/offline_pages/core/background/request_coordinator.h (right): https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... components/offline_pages/core/background/request_coordinator.h:67: STARTING, PICKING might be more descriptive than STARTING. https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... File components/offline_pages/core/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... components/offline_pages/core/background/request_coordinator_unittest.cc:132: bool is_busy() { Better to get these to return the state, and check the state in the tests. Sure, it is a bigger change, but makes the test clearer.
addressed comments, happy to talk offline about the state during StopPrerendering() / StopProcessing(). PTAL! https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... components/offline_pages/core/background/request_coordinator.cc:289: if (offliner_ && state_ == RequestCoordinatorState::OFFLINING) { On 2017/03/28 00:35:13, Pete Williamson wrote: > What should we do if the state is STARTING? We probably don't want to go on > with the request. Maybe we should check in RequestPicked() to see if a cancel > happened while we were picking, and if so, to not then proceed to try the > request. I think this is guarded by ProcessingWindowState::STOPPED in StopProcessing(). The processing_state_ is checked in RequestPicked. Are you worried about StopPrerendering()? https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... components/offline_pages/core/background/request_coordinator.cc:780: if (!SendRequestToOffliner(request)) On 2017/03/28 00:35:13, Pete Williamson wrote: > Why check the return value, and then do the same thing either way? Maybe the > else clause should be dropped. Seems like you mismatched the braces? :) I've pulled a little bit more logic out of SendRequestToOffliner and now it should look better. Also in order to keep the TODO there I kept the if check inside the case which we should reset the state. https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... File components/offline_pages/core/background/request_coordinator.h (right): https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... components/offline_pages/core/background/request_coordinator.h:67: STARTING, On 2017/03/28 00:35:13, Pete Williamson wrote: > PICKING might be more descriptive than STARTING. Done. https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... File components/offline_pages/core/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... components/offline_pages/core/background/request_coordinator_unittest.cc:132: bool is_busy() { On 2017/03/28 00:35:13, Pete Williamson wrote: > Better to get these to return the state, and check the state in the tests. > Sure, it is a bigger change, but makes the test clearer. Done.
The CQ bit was checked by romax@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... components/offline_pages/core/background/request_coordinator.cc:289: if (offliner_ && state_ == RequestCoordinatorState::OFFLINING) { On 2017/03/28 01:09:56, romax wrote: > On 2017/03/28 00:35:13, Pete Williamson wrote: > > What should we do if the state is STARTING? We probably don't want to go on > > with the request. Maybe we should check in RequestPicked() to see if a cancel > > happened while we were picking, and if so, to not then proceed to try the > > request. > > I think this is guarded by ProcessingWindowState::STOPPED in StopProcessing(). > The processing_state_ is checked in RequestPicked. > Are you worried about StopPrerendering()? My concern is that we might have a call to StopPrerendering come in when the request picker is active. StopPrerendering can be called from several places. It can be called if the user cancels a request, or from a call to StopProcessing (which can itself be called externally since we expose the API), or from a call to the watchdog timeout. Calling from the watchdog timeout should only happen if the state is offlining, so that is not an issue. It could be that StopProcessing() is called after the processing_state_ check in PickRequestTask. While most of the code in request coordinator is single threaded, the PickRequestTask does happen on a request queue. That means it might interleave unpredictably, or we might have a worker thread someday. ProcessingState is really intended for us to know if we are doing background or foreground rendering, not to be a guard if we are active, this state variable should be the code guard for whether the renderer is active. https://codereview.chromium.org/2775223006/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2775223006/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:780: if (!offliner_) { Let's remove this if !offliner check from both if statements, and the TODO. We were originally concerned that the offliner pointer could be null if the offliner crashed, but that doesn't happen (it just points at garbage), which makes this check useless. That will make this if statement easier to read. https://codereview.chromium.org/2775223006/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:787: SendRequestToOffliner(request); Since we aren't checking the return value anymore, should we put the code back to what it used to be? https://codereview.chromium.org/2775223006/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2775223006/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator_unittest.cc:134: RequestCoordinatorState state() { return coordinator()->state(); } Looks good, thanks for doing this!
The CQ bit was checked by romax@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
updated per offline discussion. ptal https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2775223006/diff/1/components/offline_pages/co... components/offline_pages/core/background/request_coordinator.cc:289: if (offliner_ && state_ == RequestCoordinatorState::OFFLINING) { On 2017/03/28 17:12:43, Pete Williamson wrote: > On 2017/03/28 01:09:56, romax wrote: > > On 2017/03/28 00:35:13, Pete Williamson wrote: > > > What should we do if the state is STARTING? We probably don't want to go on > > > with the request. Maybe we should check in RequestPicked() to see if a > cancel > > > happened while we were picking, and if so, to not then proceed to try the > > > request. > > > > I think this is guarded by ProcessingWindowState::STOPPED in StopProcessing(). > > The processing_state_ is checked in RequestPicked. > > Are you worried about StopPrerendering()? > > My concern is that we might have a call to StopPrerendering come in when the > request picker is active. > > StopPrerendering can be called from several places. It can be called if the > user cancels a request, or from a call to StopProcessing (which can itself be > called externally since we expose the API), or from a call to the watchdog > timeout. Calling from the watchdog timeout should only happen if the state is > offlining, so that is not an issue. > > It could be that StopProcessing() is called after the processing_state_ check in > PickRequestTask. While most of the code in request coordinator is single > threaded, the PickRequestTask does happen on a request queue. That means it > might interleave unpredictably, or we might have a worker thread someday. > > ProcessingState is really intended for us to know if we are doing background or > foreground rendering, not to be a guard if we are active, this state variable > should be the code guard for whether the renderer is active. Done. https://codereview.chromium.org/2775223006/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2775223006/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:780: if (!offliner_) { On 2017/03/28 17:12:43, Pete Williamson wrote: > Let's remove this if !offliner check from both if statements, and the TODO. We > were originally concerned that the offliner pointer could be null if the > offliner crashed, but that doesn't happen (it just points at garbage), which > makes this check useless. That will make this if statement easier to read. Done. https://codereview.chromium.org/2775223006/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:787: SendRequestToOffliner(request); On 2017/03/28 17:12:43, Pete Williamson wrote: > Since we aren't checking the return value anymore, should we put the code back > to what it used to be? Done. https://codereview.chromium.org/2775223006/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2775223006/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator_unittest.cc:134: RequestCoordinatorState state() { return coordinator()->state(); } On 2017/03/28 17:12:43, Pete Williamson wrote: > Looks good, thanks for doing this! :)
lgtm. Make sure you test thoroughly on the device.
The CQ bit was checked by romax@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 romax@chromium.org
The CQ bit was checked by romax@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/2775223006/#ps60001 (title: "minor comment fix.")
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: ios-simulator on master.tryserver.chromium.mac (JOB_TIMED_OUT, build hasn't started yet, builder probably lacks capacity)
The CQ bit was checked by romax@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490895902147590,
"parent_rev": "921b599d1775ae5ea19a51a4df7bbe70241beb1b", "commit_rev":
"7be44a4d4b5fbbd8787a57a4ffb705dae78bf7e1"}
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Improve RequestCoordinator state tracking. Replace the several boolean values used for state tracking with single state variable. BUG=644519 ========== to ========== [Offline Pages] Improve RequestCoordinator state tracking. Replace the several boolean values used for state tracking with single state variable. BUG=644519 Review-Url: https://codereview.chromium.org/2775223006 Cr-Commit-Position: refs/heads/master@{#460820} Committed: https://chromium.googlesource.com/chromium/src/+/7be44a4d4b5fbbd8787a57a4ffb7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7be44a4d4b5fbbd8787a57a4ffb7... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
