Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2113383002:
More detailed implementation of the RequestPicker (Closed)
|
Created:
4 years, 5 months ago by Pete Williamson 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, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMore detailed implementation of the RequestPicker
BUG=610521
Committed: https://crrev.com/4c1748036810d9f4280dfcb4e53ebeec84abaadc
Cr-Commit-Position: refs/heads/master@{#407281}
Patch Set 1 #Patch Set 2 : merge fixes in the unittests #Patch Set 3 : More detailed implementation of the RequestPicker #Patch Set 4 : Fix tests #Patch Set 5 : Fix time check and MeetsConditions check #
Total comments: 40
Patch Set 6 : CR fixes per DougArnett #
Total comments: 6
Patch Set 7 : second round of CR comments #
Total comments: 6
Patch Set 8 : Simplify picker logic for multiple criteria #
Total comments: 47
Patch Set 9 : FGorski feedback round 1 #Patch Set 10 : Try a different way of checking policy to separate policy from checking. #
Total comments: 17
Patch Set 11 : More FGorski feedback changes #Patch Set 12 : Factor request comparison more cleanly #Patch Set 13 : Fix merge #Messages
Total messages: 48 (24 generated)
The CQ bit was checked by petewil@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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by petewil@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-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by petewil@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-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by petewil@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...
petewil@chromium.org changed reviewers: + dougarnett@chromium.org, romax@chromium.org
https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:107: // Look at policies to decide if we prefer more-tried or less tried requests. Reviewers: Please think about if we got the right criteria. Did we check them in the right order? Are there any other criteria to consider? Should we use last_attempt_time?
The CQ bit was checked by petewil@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_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Some initial comments - mostly some terminology thoughts as I get my head into this topic. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/device_conditions.h (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/device_conditions.h:23: DeviceConditions() why would we want default ctor to have weakest set of constraints? https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:29: int GetMaxRetries() { return 4; } So if retry count instead of try count, this means up to 5 tries. Wonder if we should start smaller - perhaps 2 retries (so up to 3 tries). https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:29: int GetMaxRetries() { return 4; } Consider defining these values in constants (eg, DEFAULT_MAX_RETRIES)? https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:29: int GetMaxRetries() { return 4; } I wonder if we will want to have different values for user-requested and not. If so, do we pass in parameter to this method or have separate OfflinerPolicy instances or something else. [Perhaps not actionable for this CL but something for us to consider] https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:31: // How many seconds to keep trying new pages for, before we call it a day "call it a day" may be bad metaphor for a time in seconds value :-) https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:33: int GetBackgroundTimeBudgetInSeconds() { return 170; } GetBackgroundProcessingTimeBudgetSeconds? https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:36: int GetSinglePageTimeBudgetInSeconds() { return 120; } GetBackgroundLoadTimeBudgetSeconds? [not sure about these name suggestions but wanted to consider tying them more to the operation terminology] https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:40: int GetBatteryRequiredForSpeculativeOfflining() { return 50; } "Speculative" might carry more meaning that just not being user requested (eg, Zine seems more "speculative" than top search result). GetMinimumBatteryLevelForUserRequest() { return 0; } and GetMinimumBatteryLevelForNonUserRequest() or GetMinimumBatteryLevel(bool is_user_request) ? Also, should we hold off on defining Speculative yet? https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:34: // When we get queue contents from the queue, use them to pick the next remove one of the "queue" references on this line https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:48: // Handle each request only once, replacing the best reqeust candidate if it Consider if better: Iterate once through the requests, keeping track of best candidate. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:51: if (!RequestMeetsConditions(requests[i])) { ConditionsSatisyRequest() ? https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:52: DVLOG(0) << "@@@@@@ request did not meet."; clean up these before landing? https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:72: bool RequestPicker::RequestMeetsConditions(const SavePageRequest& request) { might be nicer to pass in conditions rather then use member var (could be simpler to test). https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/request_picker.h (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.h:19: RequestPicker(RequestQueue* requestQueue, OfflinerPolicy* policy); indent https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/save_page_request.h (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/save_page_request.h:34: const bool was_user_reqeusted); spelling https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/save_page_request.h:75: void set_user_reqeusted(bool user_requested) { spelling
CR feedback per DougArnett https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/device_conditions.h (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/device_conditions.h:23: DeviceConditions() On 2016/07/18 21:45:44, dougarnett wrote: > why would we want default ctor to have weakest set of constraints? Ideally, the default ctor is only used when constraints are not available to the code building it, and will be set later, so these values should never actually be seen. However, if you prefer stronger constraints in case one slips through, let me know what you prefer, and I'm happy to change the default ctor. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:29: int GetMaxRetries() { return 4; } On 2016/07/18 21:45:44, dougarnett wrote: > So if retry count instead of try count, this means up to 5 tries. Wonder if we > should start smaller - perhaps 2 retries (so up to 3 tries). Done (changed to 2) https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:29: int GetMaxRetries() { return 4; } On 2016/07/18 21:45:44, dougarnett wrote: > I wonder if we will want to have different values for user-requested and not. If > so, do we pass in parameter to this method or have separate OfflinerPolicy > instances or something else. [Perhaps not actionable for this CL but something > for us to consider] Considering different values for later https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:29: int GetMaxRetries() { return 4; } On 2016/07/18 21:45:44, dougarnett wrote: > Consider defining these values in constants (eg, DEFAULT_MAX_RETRIES)? This is temporary code, we will eventually change to get it from finch, so I planned to keep it simple, which is why I didn't already use constants, but since you asked, I changed the file to use constants everywhere. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:31: // How many seconds to keep trying new pages for, before we call it a day On 2016/07/18 21:45:44, dougarnett wrote: > "call it a day" may be bad metaphor for a time in seconds value :-) Done. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:33: int GetBackgroundTimeBudgetInSeconds() { return 170; } On 2016/07/18 21:45:44, dougarnett wrote: > GetBackgroundProcessingTimeBudgetSeconds? Done. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/offliner_policy.h:40: int GetBatteryRequiredForSpeculativeOfflining() { return 50; } On 2016/07/18 21:45:44, dougarnett wrote: > "Speculative" might carry more meaning that just not being user requested (eg, > Zine seems more "speculative" than top search result). > > GetMinimumBatteryLevelForUserRequest() { return 0; } and > GetMinimumBatteryLevelForNonUserRequest() or > GetMinimumBatteryLevel(bool is_user_request) ? > > Also, should we hold off on defining Speculative yet? Went with GetMinimumBatteryLevelForNonUserRequestOfflining(); While I'm happy to hold off, when I made similar suggestions about deferring parts of the speculative loading path to later, you generally seemed to prefer doing it optimistically. Also, the Now/AGSA stuff is starting to land, so I thought it was maybe time to start adding support. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:34: // When we get queue contents from the queue, use them to pick the next On 2016/07/18 21:45:44, dougarnett wrote: > remove one of the "queue" references on this line Done. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:48: // Handle each request only once, replacing the best reqeust candidate if it On 2016/07/18 21:45:44, dougarnett wrote: > Consider if better: Iterate once through the requests, keeping track of best > candidate. I think that is what we are doing. We iterate once through, asking two questions each time, is this request eligible, and is it better. Is my code formatting confusing somehow? If so, let me know and I'll make it more readable. Is my comment misleading? https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:51: if (!RequestMeetsConditions(requests[i])) { On 2016/07/18 21:45:44, dougarnett wrote: > ConditionsSatisyRequest() ? ConditionsSatisfyRequest seems a bit ambiguous to me. RequestConditionsSatisfied? https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:52: DVLOG(0) << "@@@@@@ request did not meet."; On 2016/07/18 21:45:44, dougarnett wrote: > clean up these before landing? Oops, though they were already gone. Removed now. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:72: bool RequestPicker::RequestMeetsConditions(const SavePageRequest& request) { On 2016/07/18 21:45:44, dougarnett wrote: > might be nicer to pass in conditions rather then use member var (could be > simpler to test). I'm not sure it helps much, we typically make a new picker for each test, setting conditions in the ctor, and it is easy to add a SetConditionsForTest method if we end up needing it. I'm happy to change this if you feel strongly, or if we discover a need for it later while testing. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/request_picker.h (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.h:19: RequestPicker(RequestQueue* requestQueue, OfflinerPolicy* policy); On 2016/07/18 21:45:45, dougarnett wrote: > indent Done. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/save_page_request.h (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/save_page_request.h:75: void set_user_reqeusted(bool user_requested) { On 2016/07/18 21:45:45, dougarnett wrote: > spelling Done.
https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/device_conditions.h (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/device_conditions.h:23: DeviceConditions() On 2016/07/18 22:52:09, Pete Williamson wrote: > On 2016/07/18 21:45:44, dougarnett wrote: > > why would we want default ctor to have weakest set of constraints? > > Ideally, the default ctor is only used when constraints are not available to the > code building it, and will be set later, so these values should never actually > be seen. > > However, if you prefer stronger constraints in case one slips through, let me > know what you prefer, and I'm happy to change the default ctor. Indeed, since it is public it was a concern of default instance slipping through by some maintenance developer down the road. I haven't noticed where it is needed/used yet. My first choice would be to see if it could be protected or private. If not, the power connection true on UNMETERED network would make me less twitchy here. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:48: // Handle each request only once, replacing the best reqeust candidate if it On 2016/07/18 22:52:09, Pete Williamson wrote: > On 2016/07/18 21:45:44, dougarnett wrote: > > Consider if better: Iterate once through the requests, keeping track of best > > candidate. > > I think that is what we are doing. We iterate once through, asking two > questions each time, is this request eligible, and is it better. Is my code > formatting confusing somehow? If so, let me know and I'll make it more > readable. Is my comment misleading? Was proposing different comment wording. I found "handle" wording bit confusing initially but is ok. Fix spelling if you keep your wording. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:51: if (!RequestMeetsConditions(requests[i])) { On 2016/07/18 22:52:09, Pete Williamson wrote: > On 2016/07/18 21:45:44, dougarnett wrote: > > ConditionsSatisyRequest() ? > > ConditionsSatisfyRequest seems a bit ambiguous to me. > RequestConditionsSatisfied? good https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:72: bool RequestPicker::RequestMeetsConditions(const SavePageRequest& request) { On 2016/07/18 22:52:09, Pete Williamson wrote: > On 2016/07/18 21:45:44, dougarnett wrote: > > might be nicer to pass in conditions rather then use member var (could be > > simpler to test). > > I'm not sure it helps much, we typically make a new picker for each test, > setting conditions in the ctor, and it is easy to add a SetConditionsForTest > method if we end up needing it. > > I'm happy to change this if you feel strongly, or if we discover a need for it > later while testing. Just was idea to consider. It seems like pure logic function so if it was also static, then could have very simple suite of tests for it that don't involve setting up picker instance. https://codereview.chromium.org/2113383002/diff/100001/components/offline_pag... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2113383002/diff/100001/components/offline_pag... components/offline_pages/background/offliner_policy.h:52: return kBatteryPercentageForSpeculativeOfflining; please match constant name to newer method name https://codereview.chromium.org/2113383002/diff/100001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/100001/components/offline_pag... components/offline_pages/background/request_picker.cc:128: // If we found that this wasn't as good in the area of request count, I'm not sure I understand this initially. Is this really part of the above check? I wonder if the not equal check should go around the above with fall-through return of false? https://codereview.chromium.org/2113383002/diff/100001/components/offline_pag... components/offline_pages/background/request_picker.cc:138: if (newRequest->creation_time() < oldRequest->creation_time()) { So we make no assumptions about queue ordering which seems safe but I wonder if we should discuss with Filip. Eg, if we decided that the queue would guarantee ordering by creation time or something (since it is named "queue"), then we could consider returning early in some case.
https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:107: // Look at policies to decide if we prefer more-tried or less tried requests. On 2016/07/18 17:41:06, Pete Williamson wrote: > Reviewers: Please think about if we got the right criteria. Did we check them > in the right order? Are there any other criteria to consider? Should we use > last_attempt_time? Down the road, I expect we may want to consider last_attempt_time if we know it failed previously rather than canceled. But we may need to understand failure and cancel scenarios better first. I did not find it immediately obvious that you have the right order of retry count vs. earlier/later as they have some interaction. If you want earlier requests, then I think that consideration has higher priority than retry count but if you don't want earlier requests then I think the retry count has higher priority. So I think what you have works but is a bit hard to follow the retry count logic and understanding that you won't be inserting "earlier" requests that can have a lower retry count than existing entries. Maybe easier to discuss in person today to keep as is or how we might improve it.
https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/device_conditions.h (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/device_conditions.h:23: DeviceConditions() On 2016/07/19 00:20:44, dougarnett wrote: > On 2016/07/18 22:52:09, Pete Williamson wrote: > > On 2016/07/18 21:45:44, dougarnett wrote: > > > why would we want default ctor to have weakest set of constraints? > > > > Ideally, the default ctor is only used when constraints are not available to > the > > code building it, and will be set later, so these values should never actually > > be seen. > > > > However, if you prefer stronger constraints in case one slips through, let me > > know what you prefer, and I'm happy to change the default ctor. > > Indeed, since it is public it was a concern of default instance slipping through > by some maintenance developer down the road. > > I haven't noticed where it is needed/used yet. My first choice would be to see > if it could be protected or private. If not, the power connection true on > UNMETERED network would make me less twitchy here. Changed to power on, 75% battery, and unmetered network. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:48: // Handle each request only once, replacing the best reqeust candidate if it On 2016/07/19 00:20:44, dougarnett wrote: > On 2016/07/18 22:52:09, Pete Williamson wrote: > > On 2016/07/18 21:45:44, dougarnett wrote: > > > Consider if better: Iterate once through the requests, keeping track of best > > > candidate. > > > > I think that is what we are doing. We iterate once through, asking two > > questions each time, is this request eligible, and is it better. Is my code > > formatting confusing somehow? If so, let me know and I'll make it more > > readable. Is my comment misleading? > > Was proposing different comment wording. I found "handle" wording bit confusing > initially but is ok. > > Fix spelling if you keep your wording. Done. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:51: if (!RequestMeetsConditions(requests[i])) { On 2016/07/19 00:20:44, dougarnett wrote: > On 2016/07/18 22:52:09, Pete Williamson wrote: > > On 2016/07/18 21:45:44, dougarnett wrote: > > > ConditionsSatisyRequest() ? > > > > ConditionsSatisfyRequest seems a bit ambiguous to me. > > RequestConditionsSatisfied? > > good Done. https://codereview.chromium.org/2113383002/diff/80001/components/offline_page... components/offline_pages/background/request_picker.cc:107: // Look at policies to decide if we prefer more-tried or less tried requests. On 2016/07/19 15:49:49, dougarnett wrote: > On 2016/07/18 17:41:06, Pete Williamson wrote: > > Reviewers: Please think about if we got the right criteria. Did we check them > > in the right order? Are there any other criteria to consider? Should we use > > last_attempt_time? > > Down the road, I expect we may want to consider last_attempt_time if we know it > failed previously rather than canceled. But we may need to understand failure > and cancel scenarios better first. > > I did not find it immediately obvious that you have the right order of retry > count vs. earlier/later as they have some interaction. If you want earlier > requests, then I think that consideration has higher priority than retry count > but if you don't want earlier requests then I think the retry count has higher > priority. So I think what you have works but is a bit hard to follow the retry > count logic and understanding that you won't be inserting "earlier" requests > that can have a lower retry count than existing entries. Maybe easier to discuss > in person today to keep as is or how we might improve it. Per our discussion, I added a new policy to pick whether retry count or recency should come first. https://codereview.chromium.org/2113383002/diff/100001/components/offline_pag... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2113383002/diff/100001/components/offline_pag... components/offline_pages/background/offliner_policy.h:52: return kBatteryPercentageForSpeculativeOfflining; On 2016/07/19 00:20:44, dougarnett wrote: > please match constant name to newer method name Done. https://codereview.chromium.org/2113383002/diff/100001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/100001/components/offline_pag... components/offline_pages/background/request_picker.cc:128: // If we found that this wasn't as good in the area of request count, On 2016/07/19 00:20:44, dougarnett wrote: > I'm not sure I understand this initially. Is this really part of the above > check? I wonder if the not equal check should go around the above with > fall-through return of false? I was trying to make the logic above smaller by collecting the return cases. This is all different now, but the original intent was that if the retry count matched, we needed a tiebreaker, if it didn't match, this was the common "fail" case where the original request was better. https://codereview.chromium.org/2113383002/diff/100001/components/offline_pag... components/offline_pages/background/request_picker.cc:138: if (newRequest->creation_time() < oldRequest->creation_time()) { On 2016/07/19 00:20:44, dougarnett wrote: > So we make no assumptions about queue ordering which seems safe but I wonder if > we should discuss with Filip. Eg, if we decided that the queue would guarantee > ordering by creation time or something (since it is named "queue"), then we > could consider returning early in some case. The "queue" doesn't do any ordering for us in spite of its name. I did bring that up once in a code review, but the answer ws that it fits the logical description of a queue as a place for pending work.
https://codereview.chromium.org/2113383002/diff/120001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/120001/components/offline_pag... components/offline_pages/background/request_picker.cc:12: const int kEquiavalentRequest = 0; spelling https://codereview.chromium.org/2113383002/diff/120001/components/offline_pag... components/offline_pages/background/request_picker.cc:12: const int kEquiavalentRequest = 0; Consider defining enum https://codereview.chromium.org/2113383002/diff/120001/components/offline_pag... components/offline_pages/background/request_picker.cc:124: if (policy_->RetryCountIsMoreImportantThanRecency()) { As we discussed, still more complicated than we'd like. One other idea is to first collect comparison values and then write algorithm using them. eg, bool betterPriority = ....; TriState betterRecency = ...; TriState betterRetries = ...; if (betterPriority) return true; else if (RetryCountMoreIMportant) return (betterRetries != Same) ? betterRetries == Better : betterRecency == Better; else return (betterRecency != Same) ? betterRecency == Better : betterRetries == Better;
https://codereview.chromium.org/2113383002/diff/120001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/120001/components/offline_pag... components/offline_pages/background/request_picker.cc:12: const int kEquiavalentRequest = 0; On 2016/07/19 20:49:11, dougarnett wrote: > Consider defining enum Done. https://codereview.chromium.org/2113383002/diff/120001/components/offline_pag... components/offline_pages/background/request_picker.cc:12: const int kEquiavalentRequest = 0; On 2016/07/19 20:49:10, dougarnett wrote: > spelling Changed to enum, renamed it. https://codereview.chromium.org/2113383002/diff/120001/components/offline_pag... components/offline_pages/background/request_picker.cc:124: if (policy_->RetryCountIsMoreImportantThanRecency()) { On 2016/07/19 20:49:11, dougarnett wrote: > As we discussed, still more complicated than we'd like. > > One other idea is to first collect comparison values and then write algorithm > using them. eg, > > bool betterPriority = ....; > TriState betterRecency = ...; > TriState betterRetries = ...; > > if (betterPriority) > return true; > else if (RetryCountMoreIMportant) > return (betterRetries != Same) > ? betterRetries == Better > : betterRecency == Better; > else return (betterRecency != Same) > ? betterRecency == Better > : betterRetries == Better; > > Done.
lgtm Would be good to get 2nd second of eyes sign-off on this one. If romax doesn't get to it before his vacation, maybe ask fgorski.
petewil@chromium.org changed reviewers: + fgorski@chromium.org
+FGorski - Could you please take a look?
Mostly nits, but I have some suggestions to the picker. We can chat, if you have questions. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/device_conditions.h (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/device_conditions.h:27: DeviceConditions(const DeviceConditions& other) Given that you are defining the copy constructor, what are your thought on assignment operator? https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/device_conditions.h:29: battery_percentage_(other.battery_percentage_), nit: please align properly https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/offliner_policy.h:11: const int kSinglePageTimeBudgetSeconds = 120; nit: having defaults specified in such way, you will pretty much use your budget on a single failed attempt. Did you consider other combinations, where you would maybe succeed 2 single page attempts, or give more time for a single attempt? https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/offliner_policy.h:43: // return to the scheduler for the time being. nit: can you remove "for the time being"? just "return control to the scheduler" https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/offliner_policy.h:55: int GetMinimumBatteryPercentageForNonUserRequestOfflining() { nit: I think it is safe to drop offlining (given the context of policy name). https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:63: picker_.reset(new RequestPicker(queue_.get(), policy.get())); policy_.get() This is happening after the std::move above, I suspect you are assigning nullptr. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:72: // TODO(petewil): We need a robust scheme for allocating new IDs. Note: We may have to switch for GUIDs for this give downloads home design. You can capture this in a todo. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_coordinator.h:51: const GURL& url, const ClientId& client_id, bool was_user_reqeusted); nit: user_requested is good enough of a passive voice. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_coordinator.h:127: void SetDeviceConditionsForTest(DeviceConditions& current_conditions) { nit: ForTesting seems to be more popular in Chromium codebase. As this file has more, you don't have to fix it, but perhaps something we might want to revisit in future. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_coordinator.h:144: std::unique_ptr<DeviceConditions> current_conditions_; device_conditions_ might be a better name for this field https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.cc:50: if (!RequestConditionsSatisfied(requests[i])) { nit: {} not needed https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.cc:51: continue; nit: indentation https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.cc:125: if (retryCountPreference != Same) { This use of result both here and below (specifically treating same as false through return false), as well as the naming of methods including IsNewRequest...Better tells me that these methods should be returning bool, as they are predicates. (Treat this as an alternative solution to the approach presented below, using STL.) https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.cc:144: ComparisonResult RequestPicker::IsNewRequestRetryCountBetter( I have 2 concerns with these implementations. 1) They don't conform to Compare as defined by STL, therefor you cannot use simple std::sort or std::min (which would be much simpler) 2) They are mixing policy and comparison in a way where policy has to be consulted in every comparison. I see an alternative: a method that produces a Compare object suitable to a given policy/device conditions or anything else. And then throwing that Compare object on std::min. RequestConditionsSatisfied could be applied in a same way, as UnaryPredicate using std::remove_if. A set of simple predicates also removes the need for defining Better/Worse/... enum. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.cc:146: // If the retry counts are equal, we have no preference. This is self evident in the code. (A lot of your comments below are not adding to the understanding. ) https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.cc:147: if (newRequest->attempt_count() == oldRequest->attempt_count()) nit: aligment https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.cc:165: nit: empty line is not needed here. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/request_picker.h (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.h:20: Worse = -1, Same = 0, Better = 1 nit: there are some indentation issues here. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/request_picker_unittest.cc (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker_unittest.cc:112: base::Unretained(this)), nit:alignemnt https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker_unittest.cc:129: base::Unretained(this)), nit: alignment https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/save_page_request.cc (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/save_page_request.cc:34: user_requested_(was_user_requested) {} nit: alignment https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/save_page_request.h (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/save_page_request.h:40: const bool was_user_requested); nit: user_requested https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/save_page_request.h:104: bool user_requested_; I like this name :)
https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/device_conditions.h (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/device_conditions.h:29: battery_percentage_(other.battery_percentage_), On 2016/07/20 16:23:31, fgorski wrote: > nit: please align properly Done. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/offliner_policy.h:11: const int kSinglePageTimeBudgetSeconds = 120; On 2016/07/20 16:23:31, fgorski wrote: > nit: having defaults specified in such way, you will pretty much use your budget > on a single failed attempt. > > Did you consider other combinations, where you would maybe succeed 2 single page > attempts, or give more time for a single attempt? Yep. There is a bug to tune these, and at the moment, these are just guesses. (crbug/625204) The 170 comes from the 3 minute Marshmallow maintenance cycle. The 120 comes from a 95% time for popular websites in India on 2G. I understand that we only have time for a single failed attempt and to start a second attempt, but it seems that we should figure out good numbers for these first before worrying about their relation to eachother. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/offliner_policy.h:43: // return to the scheduler for the time being. On 2016/07/20 16:23:31, fgorski wrote: > nit: can you remove "for the time being"? > just "return control to the scheduler" Done. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/offliner_policy.h:55: int GetMinimumBatteryPercentageForNonUserRequestOfflining() { On 2016/07/20 16:23:31, fgorski wrote: > nit: I think it is safe to drop offlining (given the context of policy name). Not done, I think it is better to be more explict in this case. I want to be explict that the non-user request is an offlining request, not some other thing we decided to do. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:63: picker_.reset(new RequestPicker(queue_.get(), policy.get())); On 2016/07/20 16:23:31, fgorski wrote: > policy_.get() > This is happening after the std::move above, I suspect you are assigning > nullptr. Nice catch! This was supposed to be policy_ https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:72: // TODO(petewil): We need a robust scheme for allocating new IDs. On 2016/07/20 16:23:31, fgorski wrote: > Note: We may have to switch for GUIDs for this give downloads home design. You > can capture this in a todo. Todo elaborated a bit more to mention GUIDS and downloads home. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_coordinator.h:51: const GURL& url, const ClientId& client_id, bool was_user_reqeusted); On 2016/07/20 16:23:31, fgorski wrote: > nit: user_requested is good enough of a passive voice. Done. I thought the was was indicating past tense, not more passive, but user_requested is OK here if you think it is descriptive enough. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_coordinator.h:127: void SetDeviceConditionsForTest(DeviceConditions& current_conditions) { On 2016/07/20 16:23:31, fgorski wrote: > nit: ForTesting seems to be more popular in Chromium codebase. > As this file has more, you don't have to fix it, but perhaps something we might > want to revisit in future. Noted. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_coordinator.h:144: std::unique_ptr<DeviceConditions> current_conditions_; On 2016/07/20 16:23:31, fgorski wrote: > device_conditions_ might be a better name for this field Not changed. We have two types of conditions, the desired conditions and the existing conditions. I wanted to keep them apart naming-wise, and device_conditions_ doesn't explicitly belong to one camp or the other. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.cc:50: if (!RequestConditionsSatisfied(requests[i])) { On 2016/07/20 16:23:31, fgorski wrote: > nit: {} not needed Done. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.cc:51: continue; On 2016/07/20 16:23:31, fgorski wrote: > nit: indentation Done. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.cc:125: if (retryCountPreference != Same) { On 2016/07/20 16:23:31, fgorski wrote: > This use of result both here and below (specifically treating same as false > through return false), as well as the naming of methods including > IsNewRequest...Better tells me that these methods should be returning bool, as > they are predicates. > > (Treat this as an alternative solution to the approach presented below, using > STL.) OK, per our discussion, I will try an alternate approach with the next version of the patch, stay tuned... https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.cc:146: // If the retry counts are equal, we have no preference. On 2016/07/20 16:23:31, fgorski wrote: > This is self evident in the code. (A lot of your comments below are not adding > to the understanding. ) Done. (My thinking was that this is a bit convoluted, and having comments will help the readability of the code as a whole.) https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.cc:147: if (newRequest->attempt_count() == oldRequest->attempt_count()) On 2016/07/20 16:23:31, fgorski wrote: > nit: aligment Done. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.cc:165: On 2016/07/20 16:23:31, fgorski wrote: > nit: empty line is not needed here. Done. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/request_picker.h (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker.h:20: Worse = -1, Same = 0, Better = 1 On 2016/07/20 16:23:31, fgorski wrote: > nit: there are some indentation issues here. Done. (danger of cut and paste coding, their standards may be different, even in Chrome). https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/request_picker_unittest.cc (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker_unittest.cc:112: base::Unretained(this)), On 2016/07/20 16:23:31, fgorski wrote: > nit:alignemnt Done. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/request_picker_unittest.cc:129: base::Unretained(this)), On 2016/07/20 16:23:31, fgorski wrote: > nit: alignment Done. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/save_page_request.cc (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/save_page_request.cc:34: user_requested_(was_user_requested) {} On 2016/07/20 16:23:31, fgorski wrote: > nit: alignment Done. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/save_page_request.h (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/save_page_request.h:40: const bool was_user_requested); On 2016/07/20 16:23:31, fgorski wrote: > nit: user_requested Done. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/save_page_request.h:104: bool user_requested_; On 2016/07/20 16:23:31, fgorski wrote: > I like this name :) Acknowledged.
Revamped the policy checking as suggested by FGorski to separate out policy from checking. Please take a look.
already looks much better. I actually was able to look inside the compare functions this time. They are much cleaner. Thanks. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/device_conditions.h (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/device_conditions.h:27: DeviceConditions(const DeviceConditions& other) On 2016/07/20 16:23:30, fgorski wrote: > Given that you are defining the copy constructor, what are your thought on > assignment operator? I think you missed this one. https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/offliner_policy.h:11: const int kSinglePageTimeBudgetSeconds = 120; On 2016/07/20 19:50:43, Pete Williamson wrote: > On 2016/07/20 16:23:31, fgorski wrote: > > nit: having defaults specified in such way, you will pretty much > use your budget > > on a single failed attempt. > > > > Did you consider other combinations, where you would maybe succeed 2 single > page > > attempts, or give more time for a single attempt? > > Yep. There is a bug to tune these, and at the moment, these are just guesses. > (crbug/625204) > The 170 comes from the 3 minute Marshmallow maintenance cycle. > The 120 comes from a 95% time for popular websites in India on 2G. > I understand that we only have time for a single failed attempt and to start a > second attempt, but it seems that we should figure out good numbers for these > first before worrying about their relation to eachother. Acknowledged. https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:66: } I don't think you need either pair of {} https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:143: // returns true if hte older request is better. s/hte/the/ https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:151: // Check the attempt count. Consider extracting the block from here to the first return as CompareAttemptCount. This will cover sign revert (and its explanation in one place. https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:162: base::TimeDelta difference = left->creation_time() - right->creation_time(); Same for this section. (Extract) And both extracted pieces are reused in the other compare function https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:169: if (result == 1) this whole block to the end is equivalent to: return result > 0; https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:180: base::TimeDelta difference = left->creation_time() - right->creation_time(); nit: indentation https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:197: if (result == 1) same here: return result > 0 https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... File components/offline_pages/background/request_picker.h (right): https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.h:18: typedef bool (RequestPicker::*RequestCompareFunction)( nit: RequestLessThanFunction perhaps?
https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... File components/offline_pages/background/device_conditions.h (right): https://codereview.chromium.org/2113383002/diff/140001/components/offline_pag... components/offline_pages/background/device_conditions.h:27: DeviceConditions(const DeviceConditions& other) On 2016/07/21 17:01:20, fgorski wrote: > On 2016/07/20 16:23:30, fgorski wrote: > > Given that you are defining the copy constructor, what are your thought on > > assignment operator? > > I think you missed this one. Ah, right, sorry. I intentionally left the assignment operator out because it caused an error trying to assign to a const variable in the assignment operator (it is OK for a constructor to assign to a const member variable, but not for an assignment operator). Yes, I know the rule that you should provide both when you have either, but since it didn't compile, I thought I'd try just the copy ctor. Thinking about it a bit more, I can rely on the default, and just not add the "no copy ctor or assignment operator" macro, and I should be fine, so the latest has the copy ctor removed, and a comment added. https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:66: } On 2016/07/21 17:01:20, fgorski wrote: > I don't think you need either pair of {} Done. I misinterpreted our guidelines as anything other than a single line "if" needing braces. This is one line of if and one line of else. https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:143: // returns true if hte older request is better. On 2016/07/21 17:01:20, fgorski wrote: > s/hte/the/ Done. https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:151: // Check the attempt count. On 2016/07/21 17:01:20, fgorski wrote: > Consider extracting the block from here to the first return as > CompareAttemptCount. This will cover sign revert (and its explanation in one > place. After I thought about it, I prefer leaving the code as is for several reasons. First, I can't think of a good name for the new function that will indicate what it really does. Second, subtracting times gives me a time difference that I can't just do a signum on, I need the additional step of converting to milliseconds, which would mean subtracting at the call site something like I have below. Third, the new function would need to be a template instead of a normal function since it would take int64_t for the time difference, and int for the retry count. While there is nothing wrong with a Template per-se, it makes the code just a bit more complidated, and my aim here is to make it more readable. // new function declaration: int GetSignAndMaybeFlip(T difference, boolean maybeFlip); // Function at call site result = GetSignAndMaybeFlip( (left->creation_time() - right->creation_time()).InMilliseconds), earlier_requests_better)); I think this would lead to code that is harder to understand, though a bit better factored. https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:169: if (result == 1) On 2016/07/21 17:01:20, fgorski wrote: > this whole block to the end is equivalent to: > > return result > 0; Done. https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:180: base::TimeDelta difference = left->creation_time() - right->creation_time(); On 2016/07/21 17:01:20, fgorski wrote: > nit: indentation Done. https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:197: if (result == 1) On 2016/07/21 17:01:20, fgorski wrote: > same here: return result > 0 Done. https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... File components/offline_pages/background/request_picker.h (right): https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.h:18: typedef bool (RequestPicker::*RequestCompareFunction)( On 2016/07/21 17:01:20, fgorski wrote: > nit: RequestLessThanFunction perhaps? I'd prefer to leave it as is. It isn't really a less than function.
lgtm https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:151: // Check the attempt count. On 2016/07/21 19:48:45, Pete Williamson wrote: > On 2016/07/21 17:01:20, fgorski wrote: > > Consider extracting the block from here to the first return as > > CompareAttemptCount. This will cover sign revert (and its explanation in one > > place. > > After I thought about it, I prefer leaving the code as is for several reasons. > First, I can't think of a good name for the new function that will indicate what > it really does. > Second, subtracting times gives me a time difference that I can't just do a > signum on, I need the additional step of converting to milliseconds, which would > mean subtracting at the call site something like I have below. > Third, the new function would need to be a template instead of a normal function > since it would take int64_t for the time difference, and int for the retry > count. While there is nothing wrong with a Template per-se, it makes the code > just a bit more complidated, and my aim here is to make it more readable. > > // new function declaration: > int GetSignAndMaybeFlip(T difference, boolean maybeFlip); > > // Function at call site > result = GetSignAndMaybeFlip( > (left->creation_time() - right->creation_time()).InMilliseconds), > earlier_requests_better)); > > I think this would lead to code that is harder to understand, though a bit > better factored. I was thinking of 2 functions that are specific to what they do: CompareAttemptCount CompareCreationTime You don't have to make the change if you don't want to.
https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2113383002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.cc:151: // Check the attempt count. On 2016/07/22 16:02:11, fgorski wrote: > On 2016/07/21 19:48:45, Pete Williamson wrote: > > On 2016/07/21 17:01:20, fgorski wrote: > > > Consider extracting the block from here to the first return as > > > CompareAttemptCount. This will cover sign revert (and its explanation in one > > > place. > > > > After I thought about it, I prefer leaving the code as is for several reasons. > > First, I can't think of a good name for the new function that will indicate > what > > it really does. > > Second, subtracting times gives me a time difference that I can't just do a > > signum on, I need the additional step of converting to milliseconds, which > would > > mean subtracting at the call site something like I have below. > > Third, the new function would need to be a template instead of a normal > function > > since it would take int64_t for the time difference, and int for the retry > > count. While there is nothing wrong with a Template per-se, it makes the code > > just a bit more complidated, and my aim here is to make it more readable. > > > > // new function declaration: > > int GetSignAndMaybeFlip(T difference, boolean maybeFlip); > > > > // Function at call site > > result = GetSignAndMaybeFlip( > > (left->creation_time() - right->creation_time()).InMilliseconds), > > earlier_requests_better)); > > > > I think this would lead to code that is harder to understand, though a bit > > better factored. > > I was thinking of 2 functions that are specific to what they do: > CompareAttemptCount > CompareCreationTime > > You don't have to make the change if you don't want to. Oh, that's different than what I thought you were suggesting, and totally makes sense. Done.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2113383002/#ps220001 (title: "Factor request comparison more cleanly")
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 petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2113383002/#ps240001 (title: "Fix 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.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== More detailed implementation of the RequestPicker BUG=610521 ========== to ========== More detailed implementation of the RequestPicker BUG=610521 Committed: https://crrev.com/4c1748036810d9f4280dfcb4e53ebeec84abaadc Cr-Commit-Position: refs/heads/master@{#407281} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/4c1748036810d9f4280dfcb4e53ebeec84abaadc Cr-Commit-Position: refs/heads/master@{#407281} |
