Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2209813002:
[Offline Pages] Moves Coordinator to using MarkAttemptStarted/MarkAttemptCompleted API. (Closed)
|
Created:
4 years, 4 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[Offline Pages] Moves Coordinator to using MarkAttemptStarted/MarkAttemptCompleted API.
This does fix the basic issue with the internals page showing STARTED status incorrectly
although we will be removing this Status separately.
Also adds new MarkAttemptAborted method for dealing with attempts we cancel because
chrome transitions to the foreground. [I imaging adding Pause support might also be reason to MarkAttemptAborted.]
BUG=632143
Committed: https://crrev.com/34ad8d1edc48ea257af035c270432475bac09755
Cr-Commit-Position: refs/heads/master@{#409881}
Patch Set 1 #Patch Set 2 : Merge #Patch Set 3 : Fixes status string send to event logger (was hardwired to "Saved" without checking actual status) #
Total comments: 26
Patch Set 4 : Address Pete's feedback #Patch Set 5 : Merge plus backed off a DCHECK that fails a test (rather have the test) #Patch Set 6 : Fixed merge issue of multi-clientid remove with separate callback. #Patch Set 7 : Fix trybot complaint for no return value on string mapping methods #
Total comments: 13
Patch Set 8 : Move enum2string functions to event logger and addressed some other Filip feedback #
Total comments: 2
Patch Set 9 : Fix paste error #Patch Set 10 : Merge #Messages
Total messages: 53 (35 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...
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...)
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...
Description was changed from ========== [Offline Pages] Moves Coordinator to using MarkAttemptStarted/MarkAttemptCompleted API. This does fix the basic issue with the internals page showing STARTED status incorrectly although we will be removing this Status separately. Also adds new MarkAttemptAborted method for dealing with attempts we cancel because chrome transitions to the foreground. BUG=632143 ========== to ========== [Offline Pages] Moves Coordinator to using MarkAttemptStarted/MarkAttemptCompleted API. This does fix the basic issue with the internals page showing STARTED status incorrectly although we will be removing this Status separately. Also adds new MarkAttemptAborted method for dealing with attempts we cancel because chrome transitions to the foreground. [I imaging adding Pause support might also be reason to MarkAttemptAborted.] BUG=632143 ==========
dougarnett@chromium.org changed reviewers: + fgorski@chromium.org, petewil@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/offliner.h:52: case STATUS_COUNT: return "STATUS_COUNT"; SHould we really be returning a string for the count? Any code that takes advantage of this feature will be in for an ugly surprise. https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:88: DLOG(WARNING) << "Failed to update request attempt details. " Now that more is hanging off the update request, we may need to do more if an attempted update fails. What happens to the new cases where an update fails? https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:203: StopProcessing(); So do we always need to call StopProcessing if the prerender returns failure here? How and when will it get started again? https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:225: // Update the request for the canceled attempt. Aborted cases include: * StopProcessing called by an outside caller of our API * Chrome being swapped out by android * Exceeding our maintenance window time slice https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:404: // Now just one request in the queue since failed request removed Huh? I didnt' see code to remove the failed request in this change. https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/save_page_request_unittest.cc (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/save_page_request_unittest.cc:95: // Last attempt time and status are updated. Should we have kept attempt count instead of status? https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/save_page_request_unittest.cc:103: base::Time activation_time = creation_time + base::TimeDelta::FromHours(6); Why are we setting activation_time to anything other than creation_time? It seems orthogonal to this particular test.
No CL update yet - just some response thoughts. https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/offliner.h:52: case STATUS_COUNT: return "STATUS_COUNT"; On 2016/08/03 20:44:55, Pete Williamson wrote: > SHould we really be returning a string for the count? Any code that takes > advantage of this feature will be in for an ugly surprise. update to DCHECK and empty string for non-Debug? https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:88: DLOG(WARNING) << "Failed to update request attempt details. " On 2016/08/03 20:44:55, Pete Williamson wrote: > Now that more is hanging off the update request, we may need to do more if an > attempted update fails. What happens to the new cases where an update fails? What can we do if persisting fails? Diagnostic things we could do would be some UMA and log to event_logger I suppose. For LoadAndSave, it will carry the MarkAttemptStarted state now. Otherwise, we will see the previous state next Get. https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:203: StopProcessing(); On 2016/08/03 20:44:55, Pete Williamson wrote: > So do we always need to call StopProcessing if the prerender returns failure > here? How and when will it get started again? This was a hole before - LoadAndSave will never call callback if returns false - so I guess watchdog timeout is what we would see. 3 cases where false returned: 1. already have pending request 2. prerender manager says that it can't/won't prerender at all (per configured mode and control group check) 3. can't save the url Maybe we should treat the last two cases as failures and delete the request. https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:225: // Update the request for the canceled attempt. On 2016/08/03 20:44:55, Pete Williamson wrote: > Aborted cases include: > * StopProcessing called by an outside caller of our API > * Chrome being swapped out by android > * Exceeding our maintenance window time slice Yeah, we need to add these - none of them in OfflinerDoneCallback. Specifically thinking about Render Process Killed (possible prelude to being swapped out completely) and Prerendering Canceled here. https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:404: // Now just one request in the queue since failed request removed On 2016/08/03 20:44:56, Pete Williamson wrote: > Huh? I didnt' see code to remove the failed request in this change. Greater Than or Equal check in Coordinater line 239: request.attempt_count() >= policy_->GetMaxTries() instead of previous strictly Greater Than check.
https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/offliner.h:52: case STATUS_COUNT: return "STATUS_COUNT"; On 2016/08/03 21:40:01, dougarnett wrote: > On 2016/08/03 20:44:55, Pete Williamson wrote: > > SHould we really be returning a string for the count? Any code that takes > > advantage of this feature will be in for an ugly surprise. > > update to DCHECK and empty string for non-Debug? sounds good https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:203: StopProcessing(); On 2016/08/03 21:40:01, dougarnett wrote: > On 2016/08/03 20:44:55, Pete Williamson wrote: > > So do we always need to call StopProcessing if the prerender returns failure > > here? How and when will it get started again? > > This was a hole before - LoadAndSave will never call callback if returns false - > so I guess watchdog timeout is what we would see. > > 3 cases where false returned: > 1. already have pending request > 2. prerender manager says that it can't/won't prerender at all (per configured > mode and control group check) > 3. can't save the url > > Maybe we should treat the last two cases as failures and delete the request. Case 1 - Should never happen (I'm happy to add a DCHECK somewhere) Case 2 - Sure it makes sense to StopProcessing, and I don't care if it ever gets started again, but we should probably surface this to the caller somehow so they know to stop calling, and that our virtual feature is disabled Case 3 - We shouldn't stop processing here, but just move on to the next item. https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:225: // Update the request for the canceled attempt. On 2016/08/03 21:40:01, dougarnett wrote: > On 2016/08/03 20:44:55, Pete Williamson wrote: > > Aborted cases include: > > * StopProcessing called by an outside caller of our API > > * Chrome being swapped out by android > > * Exceeding our maintenance window time slice > > Yeah, we need to add these - none of them in OfflinerDoneCallback. > Specifically thinking about Render Process Killed (possible prelude to being > swapped out completely) and Prerendering Canceled here. Acknowledged.
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/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/offliner.h (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/offliner.h:52: case STATUS_COUNT: return "STATUS_COUNT"; On 2016/08/03 22:29:12, Pete Williamson wrote: > On 2016/08/03 21:40:01, dougarnett wrote: > > On 2016/08/03 20:44:55, Pete Williamson wrote: > > > SHould we really be returning a string for the count? Any code that takes > > > advantage of this feature will be in for an ugly surprise. > > > > update to DCHECK and empty string for non-Debug? > > sounds good Done. https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:88: DLOG(WARNING) << "Failed to update request attempt details. " On 2016/08/03 21:40:01, dougarnett wrote: > On 2016/08/03 20:44:55, Pete Williamson wrote: > > Now that more is hanging off the update request, we may need to do more if an > > attempted update fails. What happens to the new cases where an update fails? > > What can we do if persisting fails? Diagnostic things we could do would be some > UMA and log to event_logger I suppose. > > For LoadAndSave, it will carry the MarkAttemptStarted state now. > Otherwise, we will see the previous state next Get. Added event logger event https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:203: StopProcessing(); On 2016/08/03 22:29:12, Pete Williamson wrote: > On 2016/08/03 21:40:01, dougarnett wrote: > > On 2016/08/03 20:44:55, Pete Williamson wrote: > > > So do we always need to call StopProcessing if the prerender returns failure > > > here? How and when will it get started again? > > > > This was a hole before - LoadAndSave will never call callback if returns false > - > > so I guess watchdog timeout is what we would see. > > > > 3 cases where false returned: > > 1. already have pending request > > 2. prerender manager says that it can't/won't prerender at all (per > configured > > mode and control group check) > > 3. can't save the url > > > > Maybe we should treat the last two cases as failures and delete the request. > > Case 1 - Should never happen (I'm happy to add a DCHECK somewhere) > Case 2 - Sure it makes sense to StopProcessing, and I don't care if it ever gets > started again, but we should probably surface this to the caller somehow so they > know to stop calling, and that our virtual feature is disabled > Case 3 - We shouldn't stop processing here, but just move on to the next item. Added CanSaveURL check in SavePageLater API to not accept such a request in first place; and added DCHECKs in offliner to catch logic errors in Debug builds. https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:225: // Update the request for the canceled attempt. On 2016/08/03 22:29:12, Pete Williamson wrote: > On 2016/08/03 21:40:01, dougarnett wrote: > > On 2016/08/03 20:44:55, Pete Williamson wrote: > > > Aborted cases include: > > > * StopProcessing called by an outside caller of our API > > > * Chrome being swapped out by android > > > * Exceeding our maintenance window time slice > > > > Yeah, we need to add these - none of them in OfflinerDoneCallback. > > Specifically thinking about Render Process Killed (possible prelude to being > > swapped out completely) and Prerendering Canceled here. > > Acknowledged. Added a TODO in StopProcessing for is_busy_ case to find current request and mark aborted https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/save_page_request_unittest.cc (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/save_page_request_unittest.cc:95: // Last attempt time and status are updated. On 2016/08/03 20:44:56, Pete Williamson wrote: > Should we have kept attempt count instead of status? Not sure what you mean. Also, my change here is only making comment more accurate for existing code. https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/save_page_request_unittest.cc:103: base::Time activation_time = creation_time + base::TimeDelta::FromHours(6); On 2016/08/03 20:44:56, Pete Williamson wrote: > Why are we setting activation_time to anything other than creation_time? It > seems orthogonal to this particular test. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_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...
LGTM with nit - comment could be improved. https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:191: weak_ptr_factory_.GetWeakPtr()); Good catch to delete this, looks like leftover code from a debugging session. I wonder how the presubmit let the unused variable through... https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/save_page_request_unittest.cc (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/save_page_request_unittest.cc:95: // Last attempt time and status are updated. On 2016/08/03 23:33:51, dougarnett wrote: > On 2016/08/03 20:44:56, Pete Williamson wrote: > > Should we have kept attempt count instead of status? > > Not sure what you mean. Also, my change here is only making comment more > accurate for existing code. Right, the comment says "last attempt time and status" I think maybe it should say "last attempt time and count"
https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator.cc:191: weak_ptr_factory_.GetWeakPtr()); On 2016/08/04 00:17:14, Pete Williamson wrote: > Good catch to delete this, looks like leftover code from a debugging session. I > wonder how the presubmit let the unused variable through... I am curious as well. https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/save_page_request_unittest.cc (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/save_page_request_unittest.cc:95: // Last attempt time and status are updated. On 2016/08/04 00:17:14, Pete Williamson wrote: > On 2016/08/03 23:33:51, dougarnett wrote: > > On 2016/08/03 20:44:56, Pete Williamson wrote: > > > Should we have kept attempt count instead of status? > > > > Not sure what you mean. Also, my change here is only making comment more > > accurate for existing code. > > Right, the comment says "last attempt time and status" I think maybe it should > say "last attempt time and count" but the count is not updated here - still 1 as in line 82
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...)
https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... File components/offline_pages/background/save_page_request_unittest.cc (right): https://codereview.chromium.org/2209813002/diff/40001/components/offline_page... components/offline_pages/background/save_page_request_unittest.cc:95: // Last attempt time and status are updated. On 2016/08/04 00:23:35, dougarnett wrote: > On 2016/08/04 00:17:14, Pete Williamson wrote: > > On 2016/08/03 23:33:51, dougarnett wrote: > > > On 2016/08/03 20:44:56, Pete Williamson wrote: > > > > Should we have kept attempt count instead of status? > > > > > > Not sure what you mean. Also, my change here is only making comment more > > > accurate for existing code. > > > > Right, the comment says "last attempt time and status" I think maybe it > should > > say "last attempt time and count" > > but the count is not updated here - still 1 as in line 82 Ah, nevermind, I saw the line checking the count, and thought that it had been updated.
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
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.
mostly looks good, but please remove static methods from .h files. https://codereview.chromium.org/2209813002/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2209813002/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:112: DCHECK(false); nit: this should be DCHECK(!pending_request_.get()); before if I believe. https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:96: const ClientId& client_id, RequestQueue::UpdateRequestResult result) { please run git cl format. https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:100: DLOG(WARNING) << "Failed to update request attempt details. " nit: use DVLOG here too https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:214: base::Bind(&RequestCoordinator::OfflinerDoneCallback, nit: alignment https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... File components/offline_pages/background/request_queue.h (right): https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... components/offline_pages/background/request_queue.h:46: static std::string UpdateRequestResultToString(UpdateRequestResult result) { Please consider defining this and the method in offliner.h outside of these files, as they do not belong with these classes. Perhaps the recorder could have a set of helper methods. Conversion of enum to string feels like a purely diagnostic function and does not belong to this or offliner interface. https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... File components/offline_pages/background/save_page_request.cc (right): https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... components/offline_pages/background/save_page_request.cc:86: --attempt_count_; based on discussion we had today with pete, dimich and justin, we should probably be incrementing attempt count upon unsuccessful completion that is retriable, rather than before making an attempt. That prevents us from decrementing on the abort.
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/2209813002/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/prerendering_offliner.cc (right): https://codereview.chromium.org/2209813002/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/prerendering_offliner.cc:112: DCHECK(false); On 2016/08/04 06:01:34, fgorski wrote: > nit: this should be DCHECK(!pending_request_.get()); before if I believe. Done. https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:96: const ClientId& client_id, RequestQueue::UpdateRequestResult result) { On 2016/08/04 06:01:34, fgorski wrote: > please run git cl format. Done. https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:100: DLOG(WARNING) << "Failed to update request attempt details. " On 2016/08/04 06:01:34, fgorski wrote: > nit: use DVLOG here too Done. https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:214: base::Bind(&RequestCoordinator::OfflinerDoneCallback, On 2016/08/04 06:01:34, fgorski wrote: > nit: alignment Done. https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... File components/offline_pages/background/request_queue.h (right): https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... components/offline_pages/background/request_queue.h:46: static std::string UpdateRequestResultToString(UpdateRequestResult result) { On 2016/08/04 06:01:34, fgorski wrote: > Please consider defining this and the method in offliner.h outside of these > files, as they do not belong with these classes. Perhaps the recorder could have > a set of helper methods. > Conversion of enum to string feels like a purely diagnostic function and does > not belong to this or offliner interface. Done. https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... File components/offline_pages/background/save_page_request.cc (right): https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... components/offline_pages/background/save_page_request.cc:86: --attempt_count_; On 2016/08/04 06:01:34, fgorski wrote: > based on discussion we had today with pete, dimich and justin, we should > probably be incrementing attempt count upon unsuccessful completion that is > retriable, rather than before making an attempt. That prevents us from > decrementing on the abort. So change MarkAttemptStarted and MarkAttemptCompleted then - in this CL? Btw, I'm concerned that we have some way to stop processing requests that never complete - that is, after they start, we always get swapped out or chrome crashes. Did you guys discuss this type of situation? I had raised it with Pete on Tuesday as a flaw with how he was currently doing the increment after attempt completed.
https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... File components/offline_pages/background/save_page_request.cc (right): https://codereview.chromium.org/2209813002/diff/120001/components/offline_pag... components/offline_pages/background/save_page_request.cc:86: --attempt_count_; On 2016/08/04 16:24:38, dougarnett wrote: > On 2016/08/04 06:01:34, fgorski wrote: > > based on discussion we had today with pete, dimich and justin, we should > > probably be incrementing attempt count upon unsuccessful completion that is > > retriable, rather than before making an attempt. That prevents us from > > decrementing on the abort. > > So change MarkAttemptStarted and MarkAttemptCompleted then - in this CL? > > Btw, I'm concerned that we have some way to stop processing requests that never > complete - that is, after they start, we always get swapped out or chrome > crashes. Did you guys discuss this type of situation? I had raised it with Pete > on Tuesday as a flaw with how he was currently doing the increment after attempt > completed. Pondering a bit more, I think we should keep this as it is for this iteration. I think it is better approach prior to add state variable or doing more work to uses last attempt time to imply the in-progress state. I could add more text to the above TODO but doesn't really seem necessary as another iteration planned in near term.
lgtm. please address the last comment. https://codereview.chromium.org/2209813002/diff/140001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2209813002/diff/140001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:33: } // namespaceconst std::string& fix the accidental paste, please.
https://codereview.chromium.org/2209813002/diff/140001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2209813002/diff/140001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:33: } // namespaceconst std::string& On 2016/08/04 16:51:50, fgorski wrote: > fix the accidental paste, please. ug, thanks!
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/2209813002/#ps160001 (title: "Fix paste error")
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
Failed to apply patch for
components/offline_pages/background/request_coordinator.h:
While running git apply --index -3 -p1;
error: patch failed:
components/offline_pages/background/request_coordinator.h:103
Falling back to three-way merge...
Applied patch to 'components/offline_pages/background/request_coordinator.h'
with conflicts.
U components/offline_pages/background/request_coordinator.h
Patch: components/offline_pages/background/request_coordinator.h
Index: components/offline_pages/background/request_coordinator.h
diff --git a/components/offline_pages/background/request_coordinator.h
b/components/offline_pages/background/request_coordinator.h
index
24c2183a34de46243d3ccd1c2ca418580abd7636..d1fbef39ecb183cf3375960ab6b7900301a30edc
100644
--- a/components/offline_pages/background/request_coordinator.h
+++ b/components/offline_pages/background/request_coordinator.h
@@ -103,7 +103,10 @@ class RequestCoordinator : public KeyedService {
void AddRequestResultCallback(RequestQueue::AddRequestResult result,
const SavePageRequest& request);
- void UpdateRequestCallback(RequestQueue::UpdateRequestResult result);
+ void UpdateRequestCallback(const ClientId& client_id,
+ RequestQueue::UpdateRequestResult result);
+
+ void UpdateMultipleRequestCallback(RequestQueue::UpdateRequestResult result);
// Callback from the request picker when it has chosen our next request.
void RequestPicked(const SavePageRequest& request);
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/2209813002/#ps180001 (title: "Merge")
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.
Description was changed from ========== [Offline Pages] Moves Coordinator to using MarkAttemptStarted/MarkAttemptCompleted API. This does fix the basic issue with the internals page showing STARTED status incorrectly although we will be removing this Status separately. Also adds new MarkAttemptAborted method for dealing with attempts we cancel because chrome transitions to the foreground. [I imaging adding Pause support might also be reason to MarkAttemptAborted.] BUG=632143 ========== to ========== [Offline Pages] Moves Coordinator to using MarkAttemptStarted/MarkAttemptCompleted API. This does fix the basic issue with the internals page showing STARTED status incorrectly although we will be removing this Status separately. Also adds new MarkAttemptAborted method for dealing with attempts we cancel because chrome transitions to the foreground. [I imaging adding Pause support might also be reason to MarkAttemptAborted.] BUG=632143 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Moves Coordinator to using MarkAttemptStarted/MarkAttemptCompleted API. This does fix the basic issue with the internals page showing STARTED status incorrectly although we will be removing this Status separately. Also adds new MarkAttemptAborted method for dealing with attempts we cancel because chrome transitions to the foreground. [I imaging adding Pause support might also be reason to MarkAttemptAborted.] BUG=632143 ========== to ========== [Offline Pages] Moves Coordinator to using MarkAttemptStarted/MarkAttemptCompleted API. This does fix the basic issue with the internals page showing STARTED status incorrectly although we will be removing this Status separately. Also adds new MarkAttemptAborted method for dealing with attempts we cancel because chrome transitions to the foreground. [I imaging adding Pause support might also be reason to MarkAttemptAborted.] BUG=632143 Committed: https://crrev.com/34ad8d1edc48ea257af035c270432475bac09755 Cr-Commit-Position: refs/heads/master@{#409881} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/34ad8d1edc48ea257af035c270432475bac09755 Cr-Commit-Position: refs/heads/master@{#409881} |
