Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2729763002:
[Offline Pages] Pick correct request when resuming. (Closed)
|
Created:
3 years, 9 months ago by romax Modified:
3 years, 9 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Pick correct request when resuming.
Fixed the bug where if user resumes a webpage download in notification,
the request queue would not honor the priority of the tapped request and
pick from all requests using normal logic.
BUG=674653
Review-Url: https://codereview.chromium.org/2729763002
Cr-Commit-Position: refs/heads/master@{#455898}
Committed: https://chromium.googlesource.com/chromium/src/+/50952e8e2031ad1f9d59b4f9bcf0682dd08e6f1b
Patch Set 1 #Patch Set 2 : Moving logic to pick_request_task. #
Total comments: 15
Patch Set 3 : comments. #
Total comments: 10
Patch Set 4 : comments from Pete. #
Total comments: 1
Patch Set 5 : After offline discussion. #
Total comments: 1
Patch Set 6 : update comments. #
Total comments: 28
Patch Set 7 : more comments. #
Total comments: 1
Patch Set 8 : nit #Messages
Total messages: 30 (10 generated)
romax@chromium.org changed reviewers: + petewil@chromium.org
petewil@ PTAL! Also happy to chat offline if this doesn't seem like a good way. Thanks!
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
I have a feeling this could go in the picker task if possible. WDYT, Pete?
On 2017/03/02 17:09:48, fgorski wrote: > I have a feeling this could go in the picker task if possible. WDYT, Pete? I agree with Filip, since we are doing picking here, it should be the responsibility of the PickerTask. If a request has been resumed, we can call the picker with a new parameter, which is either -i or the ID of the resumed task.
Moved the logic to pick_request_task. PTAL, thanks!
On 2017/03/04 03:21:45, romax wrote: > Moved the logic to pick_request_task. PTAL, thanks! I have an impression that if 3 requests are prioritized, we choose one and drop the other 2. How can we ensure that we remember the other 2 on the next picking?
And comments to the patch. https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... File components/offline_pages/core/background/pick_request_task.cc (right): https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:107: // priortized request has been chosen. if statement should probably happen outside of the for look to not make a check multiple times. Unless there is a chance to get a better fit of course. https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:119: if (search != disabled_requests_.end()) Is there a chance that prioritized request is on the disabled list? https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator.h (right): https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.h:444: std::list<int64_t> priortized_requests_; 1. why list over vector? 2. if still prefer to choose list, please add include
https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... File components/offline_pages/core/background/pick_request_task.cc (right): https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:88: bool is_priortized_request_picked = false; nit: was_prioritized_request_picked might be better https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:92: // Search for the priortized requests before iterating all the requests. There is an implicit assumption here that we should make explicit with a comment. Requests earlier in the list have higher priority than requests older in the list. https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:107: // priortized request has been chosen. On 2017/03/06 18:12:45, fgorski wrote: > if statement should probably happen outside of the for look to not make a check > multiple times. Unless there is a chance to get a better fit of course. Agreed, if a prioritized request is found, we should probably not run this loop at all. Today the loop does two things, it finds a better candidate if one exists, and detects if there are non-user-requested tasks remaining. We are going to be removing the checks for non-user-requested tasks. If we were keeping them, we could pull them into a separate loop, instead, we could just ignore them, and leave them in the loop for now until we properly remove the feature. https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... File components/offline_pages/core/background/pick_request_task_unittest.cc (right): https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/pick_request_task_unittest.cc:494: TEST_F(PickRequestTaskTest, ChoosePriortizedRequests) { Let's add a test with two prioritized requests, both of which are in the request queue, and ensure that the first prioritized request (which should be the last one in the request queue) gets chosen. To be doubly sure, make the first request in the queue very attractive (older than the other request, earlier in the queue, fewer completed tries, etc) https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:429: std::back_inserter(priortized_requests_)); After thinking about it a bit, I think the user will expect whatever they hit resume on last to have the highest priority. If the user resumes 5 web pages in one go (within a few seconds), their intent may be for them to go in the order they get resumed in. However, if they choose one, wait a few seconds and then choose another, presuming that we haven't already started on the first one, we should probably prioritize their most recent choice. Since we aren't going to be smart about checking to see if the resume requests were close in time, it is probably better to just consistently prefer the most recent, which means using a front inserter here.
addressed comments, PTAL thanks! As for fgorski@'s comment, all the prioritized requests are in a vector which the latter the request is, the higher priority it has. This is not a perfect solution though, and it is not persistent against removing chrome from memory. If this doesn't seem robust in this case i guess we have to implement priorities for requests in a lower level and keep it in database, which wasn't recommended by petewil@ when we were discussing. Happy to chat offline about this :) https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... File components/offline_pages/core/background/pick_request_task.cc (right): https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:88: bool is_priortized_request_picked = false; On 2017/03/06 18:30:57, Pete Williamson wrote: > nit: was_prioritized_request_picked might be better Done. https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:92: // Search for the priortized requests before iterating all the requests. On 2017/03/06 18:30:57, Pete Williamson wrote: > There is an implicit assumption here that we should make explicit with a > comment. Requests earlier in the list have higher priority than requests older > in the list. Done. https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:107: // priortized request has been chosen. On 2017/03/06 18:30:57, Pete Williamson wrote: > On 2017/03/06 18:12:45, fgorski wrote: > > if statement should probably happen outside of the for look to not make a > check > > multiple times. Unless there is a chance to get a better fit of course. > > Agreed, if a prioritized request is found, we should probably not run this loop > at all. > > Today the loop does two things, it finds a better candidate if one exists, and > detects if there are non-user-requested tasks remaining. We are going to be > removing the checks for non-user-requested tasks. If we were keeping them, we > could pull them into a separate loop, instead, we could just ignore them, and > leave them in the loop for now until we properly remove the feature. Refactored a little bit. Not sure if it's necessary to have a list of filtered requests, but otherwise I think there will be some duplicate code for checking disabled etc. Please share your input :) https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:119: if (search != disabled_requests_.end()) On 2017/03/06 18:12:45, fgorski wrote: > Is there a chance that prioritized request is on the disabled list? yeah there might be. added a check https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... File components/offline_pages/core/background/pick_request_task_unittest.cc (right): https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/pick_request_task_unittest.cc:494: TEST_F(PickRequestTaskTest, ChoosePriortizedRequests) { On 2017/03/06 18:30:57, Pete Williamson wrote: > Let's add a test with two prioritized requests, both of which are in the request > queue, and ensure that the first prioritized request (which should be the last > one in the request queue) gets chosen. > > To be doubly sure, make the first request in the queue very attractive (older > than the other request, earlier in the queue, fewer completed tries, etc) Done. https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator.cc (right): https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.cc:429: std::back_inserter(priortized_requests_)); On 2017/03/06 18:30:57, Pete Williamson wrote: > After thinking about it a bit, I think the user will expect whatever they hit > resume on last to have the highest priority. > > If the user resumes 5 web pages in one go (within a few seconds), their intent > may be for them to go in the order they get resumed in. However, if they choose > one, wait a few seconds and then choose another, presuming that we haven't > already started on the first one, we should probably prioritize their most > recent choice. > > Since we aren't going to be smart about checking to see if the resume requests > were close in time, it is probably better to just consistently prefer the most > recent, which means using a front inserter here. Done. https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... File components/offline_pages/core/background/request_coordinator.h (right): https://codereview.chromium.org/2729763002/diff/20001/components/offline_page... components/offline_pages/core/background/request_coordinator.h:444: std::list<int64_t> priortized_requests_; On 2017/03/06 18:12:45, fgorski wrote: > 1. why list over vector? > 2. if still prefer to choose list, please add include Done. was using list for a pop_front() action...
https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... File components/offline_pages/core/background/pick_request_task.cc (right): https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:74: // pick the most deserving request for our conditions. This was better before. Accidental change? https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:126: // Search for and pick the prioritized requests which is not disabled and in Nit: requests -> request https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:138: prioritized_requests_.pop_back(); Maybe instead of popping requests off the end of the array, we could just start at the highest index and iterate down towards 0. https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:141: if (!picked_request) { add comment // If no request was found from the priority list, then find the best request according to current policies. https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... File components/offline_pages/core/background/pick_request_task_unittest.cc (right): https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... components/offline_pages/core/background/pick_request_task_unittest.cc:528: // Make 2 prioritized requests, the second one should be picked. expand the comment a bit: "the second one should be picked because higher priority requests are later on the list"
Addressed more comments from Pete. PTAL, thanks! i'm using int rather than unsigned since it would give me compilation error and it would prevent a boundary checking... https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... File components/offline_pages/core/background/pick_request_task.cc (right): https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:74: // pick the most deserving request for our conditions. On 2017/03/07 19:06:04, Pete Williamson wrote: > This was better before. Accidental change? Done. https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:126: // Search for and pick the prioritized requests which is not disabled and in On 2017/03/07 19:06:04, Pete Williamson wrote: > Nit: requests -> request Done. https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:138: prioritized_requests_.pop_back(); On 2017/03/07 19:06:04, Pete Williamson wrote: > Maybe instead of popping requests off the end of the array, we could just start > at the highest index and iterate down towards 0. Done. https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:141: if (!picked_request) { On 2017/03/07 19:06:04, Pete Williamson wrote: > add comment > // If no request was found from the priority list, then find the best request > according to current policies. Done. https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... File components/offline_pages/core/background/pick_request_task_unittest.cc (right): https://codereview.chromium.org/2729763002/diff/40001/components/offline_page... components/offline_pages/core/background/pick_request_task_unittest.cc:528: // Make 2 prioritized requests, the second one should be picked. On 2017/03/07 19:06:04, Pete Williamson wrote: > expand the comment a bit: > "the second one should be picked because higher priority requests are later on > the list" Done.
Mostly looks good, one last thing... https://codereview.chromium.org/2729763002/diff/60001/components/offline_page... File components/offline_pages/core/background/pick_request_task.cc (right): https://codereview.chromium.org/2729763002/diff/60001/components/offline_page... components/offline_pages/core/background/pick_request_task.cc:128: // higher priority than earlier ones in the vector. Also remove the picked Why do we remove the picked request here instead of in the caller? It is a bit odd to change a data structure that belongs to the request coordinator in the picker. (Sorry, I should have noticed that before). If you do have to modify the list here, then the approach you had before is better, I was just thinking that the approach before was doing unnecessary work.
New patch after discussion offline, PTAL, thanks!
https://codereview.chromium.org/2729763002/diff/80001/components/offline_page... File components/offline_pages/core/background/request_coordinator.h (right): https://codereview.chromium.org/2729763002/diff/80001/components/offline_page... components/offline_pages/core/background/request_coordinator.h:456: // edited by request_picker_task. Owning it here for persistency. IS this comment correct? I thought that RC was filling it, and Picker was deleting it, so technically it seems like both are editing it. Furthermore, it seems like RC also removes items when we resume. Let's document this carefully somewhere.
Updated the comments describing prioritized_requests_. Not sure it's clear enough for future maintenance, open to advice! Thanks!
looks better. A few comments to handle. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... File components/offline_pages/core/background/pick_request_task.cc (right): https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:92: std::set<int64_t> available_request_ids; Did you consider unordered_set? https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:119: available_request_count++; since you have one more if after this, the request might not be available. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:123: available_request_ids.insert(requests[i]->request_id()); would it make sense to put this inside of a check: if (request available) { } https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:126: request_count_callback_.Run(requests.size(), available_request_count); available_request_ids.size() and remove available_requests_count https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:128: // Search for and pick the prioritized request which is not disabled and in Update comment to reflect what we discussed. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:133: if (available_request_ids.find(prioritized_requests_.back()) != if (available_requests_ids.count(prioritized_requests_.back()) > 0) I think it is easier to read then https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:135: for (unsigned i = 0; i < requests.size(); ++i) { please add a comment above why this is not an infinite loop: // A request is present in requests for every ID in available_request_ids... perhaps a NOTREACHED() after for loop would be helpful. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:149: for (unsigned i = 0; i < requests.size(); ++i) { Perhaps an auto iter? https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:150: if ((available_request_ids.find(requests[i]->request_id()) != same count() comment. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... File components/offline_pages/core/background/request_coordinator.h (right): https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/request_coordinator.h:455: // Used to track whether we have a prioritized request. s/whether we have a// s/request/requests/ https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/request_coordinator.h:456: // The elements can only be added by RC when there're resumed request and s/request// s/elements/requests/ s/there're/they are/ https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/request_coordinator.h:457: // there are two places where deletion from the deque would happen: s/deque/prioritized_request_/ https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/request_coordinator.h:458: // 1. When there are paused requests, RC will remove the corresponding 1. When request is paused RC will remove it. 2. When task is not available to be picked by PickRequestTask, because it was completed or cancelled), the task will remove it. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/request_coordinator.h:464: std::deque<int64_t> prioritized_requests_; Add comment about whether this is a FIFO or LIFO and follow up with PM and UX about that.
My concerns are addressed. lgtm
addressed comments, PTAnL, thanks! also writing a doc about resuming behaviors, will send out later. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... File components/offline_pages/core/background/pick_request_task.cc (right): https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:92: std::set<int64_t> available_request_ids; On 2017/03/08 17:33:23, fgorski wrote: > Did you consider unordered_set? Done. No i didn't...and seems like a hashtable is better. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:119: available_request_count++; On 2017/03/08 17:33:24, fgorski wrote: > since you have one more if after this, the request might not be available. i thought about the same thing but ended up keeping as it used to be. After asking Pete I think this number is for UMA only and it's working as intended and counting the requests which have state:AVAILABLE only. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:123: available_request_ids.insert(requests[i]->request_id()); On 2017/03/08 17:33:24, fgorski wrote: > would it make sense to put this inside of a check: > if (request available) { > > } it makes sense as a break-proof thing but not needed IMO. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:126: request_count_callback_.Run(requests.size(), available_request_count); On 2017/03/08 17:33:24, fgorski wrote: > available_request_ids.size() > > and remove available_requests_count See above comment. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:128: // Search for and pick the prioritized request which is not disabled and in On 2017/03/08 17:33:24, fgorski wrote: > Update comment to reflect what we discussed. Done. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:133: if (available_request_ids.find(prioritized_requests_.back()) != On 2017/03/08 17:33:24, fgorski wrote: > if (available_requests_ids.count(prioritized_requests_.back()) > 0) > > I think it is easier to read then Done. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:135: for (unsigned i = 0; i < requests.size(); ++i) { On 2017/03/08 17:33:24, fgorski wrote: > please add a comment above why this is not an infinite loop: > // A request is present in requests for every ID in available_request_ids... > > perhaps a NOTREACHED() after for loop would be helpful. Done. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:149: for (unsigned i = 0; i < requests.size(); ++i) { On 2017/03/08 17:33:24, fgorski wrote: > Perhaps an auto iter? Done. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:150: if ((available_request_ids.find(requests[i]->request_id()) != On 2017/03/08 17:33:24, fgorski wrote: > same count() comment. Done. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... File components/offline_pages/core/background/request_coordinator.h (right): https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/request_coordinator.h:455: // Used to track whether we have a prioritized request. On 2017/03/08 17:33:24, fgorski wrote: > s/whether we have a// > s/request/requests/ Done. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/request_coordinator.h:456: // The elements can only be added by RC when there're resumed request and On 2017/03/08 17:33:24, fgorski wrote: > s/request// > s/elements/requests/ > s/there're/they are/ Done. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/request_coordinator.h:457: // there are two places where deletion from the deque would happen: On 2017/03/08 17:33:24, fgorski wrote: > s/deque/prioritized_request_/ Done. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/request_coordinator.h:458: // 1. When there are paused requests, RC will remove the corresponding On 2017/03/08 17:33:24, fgorski wrote: > 1. When request is paused RC will remove it. > 2. When task is not available to be picked by PickRequestTask, because > it was completed or cancelled), the task will remove it. Done. https://codereview.chromium.org/2729763002/diff/100001/components/offline_pag... components/offline_pages/core/background/request_coordinator.h:464: std::deque<int64_t> prioritized_requests_; On 2017/03/08 17:33:24, fgorski wrote: > Add comment about whether this is a FIFO or LIFO and follow up with PM and UX > about that. Done.
lgtm with comment https://codereview.chromium.org/2729763002/diff/120001/components/offline_pag... File components/offline_pages/core/background/pick_request_task.cc (right): https://codereview.chromium.org/2729763002/diff/120001/components/offline_pag... components/offline_pages/core/background/pick_request_task.cc:7: #include <set> #include <unordered_set> Check if you still need a <set>
The CQ bit was checked by romax@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from petewil@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2729763002/#ps140001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489100390494820,
"parent_rev": "08ada0f645a7440d60b0004392b6330582c1da7e", "commit_rev":
"50952e8e2031ad1f9d59b4f9bcf0682dd08e6f1b"}
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Pick correct request when resuming. Fixed the bug where if user resumes a webpage download in notification, the request queue would not honor the priority of the tapped request and pick from all requests using normal logic. BUG=674653 ========== to ========== [Offline Pages] Pick correct request when resuming. Fixed the bug where if user resumes a webpage download in notification, the request queue would not honor the priority of the tapped request and pick from all requests using normal logic. BUG=674653 Review-Url: https://codereview.chromium.org/2729763002 Cr-Commit-Position: refs/heads/master@{#455898} Committed: https://chromium.googlesource.com/chromium/src/+/50952e8e2031ad1f9d59b4f9bcf0... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/50952e8e2031ad1f9d59b4f9bcf0... |
