Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2473553004:
Request Picker task (Closed)
|
Created:
4 years, 1 month ago by Pete Williamson Modified:
4 years, 1 month 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. |
DescriptionRequest Picker task
We are refactoring the Background Loading system to use async tasks.
The advantage of this is that tasks can finish uninterrupted by other
tasks which might interfere with them (better correctness, get rid
of those pesky race conditions).
To avoid circular header file dependencies, I moved several results objects from request_queue.h to a new file, queue_results.h, and the rename ended up touching quite a few files.
This change refactors the RequestPicker to use a task object.
BUG=651238
Committed: https://crrev.com/3113e19fb5a58ae8dd2bbc7af58c7584486120c0
Cr-Commit-Position: refs/heads/master@{#431617}
Patch Set 1 #
Total comments: 9
Patch Set 2 : CR fixes per DougArnett #
Total comments: 21
Patch Set 3 : merge so we can do a CQ dry run #Patch Set 4 : CR fixes per DougArnett and FGorski #
Total comments: 8
Patch Set 5 : Use RequestQueueStore instead of RequestQueue in the picker task #Patch Set 6 : merge #
Total comments: 2
Patch Set 7 : Fix unit test failures after merge #Patch Set 8 : Rename queue_status.h to request_queue_status.h #
Total comments: 2
Patch Set 9 : Missed a few places to convert back, take another merge #
Total comments: 2
Patch Set 10 : Replace use of header file with forward declare for enum #
Total comments: 67
Patch Set 11 : CR feedback per EStade, FGorski, and DougArnett #
Total comments: 7
Patch Set 12 : Fix unit tests #Patch Set 13 : merge with latest #Messages
Total messages: 69 (36 generated)
Description was changed from ========== Request Picker task We are refactoring the Background Loading system to use async tasks. The advantage of this is that tasks can finish uninterrupted by other tasks which might interfere with them (better correctness, get rid of those pesky race conditions). This change refactors the RequestPicker to use a task object. BUG=651238 ========== to ========== Request Picker task We are refactoring the Background Loading system to use async tasks. The advantage of this is that tasks can finish uninterrupted by other tasks which might interfere with them (better correctness, get rid of those pesky race conditions). To avoid circular header file dependencies, I moved several results objects from request_queue.h to a new file, queue_results.h, and the rename ended up touching quite a few files. This change refactors the RequestPicker to use a task object. BUG=651238 ==========
petewil@chromium.org changed reviewers: + dougarnett@chromium.org, fgorski@chromium.org
https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... components/offline_pages/background/pick_request_task.cc:73: ChooseRequest(std::move(valid_requests)); Consider ChooseRequestAndCallback() Or mentioning will call callback in comment above https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... components/offline_pages/background/pick_request_task.cc:76: RemoveExpiredEntries(std::move(expired_requests)); Generalize to remove other stale requests too? Maybe SplitRequests identifies additional cases and then can remove from RequestConditionsSatisfied().
https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... components/offline_pages/background/pick_request_task.cc:73: ChooseRequest(std::move(valid_requests)); On 2016/11/03 17:01:22, dougarnett wrote: > Consider ChooseRequestAndCallback() Or mentioning will call callback in comment > above Done. https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... components/offline_pages/background/pick_request_task.cc:76: RemoveExpiredEntries(std::move(expired_requests)); On 2016/11/03 17:01:22, dougarnett wrote: > Generalize to remove other stale requests too? Maybe SplitRequests identifies > additional cases and then can remove from RequestConditionsSatisfied(). Renamed to "RemoveStaleRequests"
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) 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_...)
https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... components/offline_pages/background/pick_request_task.cc:76: RemoveExpiredEntries(std::move(expired_requests)); On 2016/11/03 19:26:29, Pete Williamson wrote: > On 2016/11/03 17:01:22, dougarnett wrote: > > Generalize to remove other stale requests too? Maybe SplitRequests identifies > > additional cases and then can remove from RequestConditionsSatisfied(). > > Renamed to "RemoveStaleRequests" Still some more work though to check started_attempt_count and completed_attempt_count against policy limits in SplitRequests? https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task.cc:126: std::vector<std::unique_ptr<SavePageRequest>> expired_requests) { expired_requests => stale_requests ? https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/pick_request_task.h (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task.h:60: // Step 2b. Handle deleting expired entries and notifying observers. expired => stale
Some initial comments. Pete, I was under impression that QueueResults patch will go in first. Is there a reason this is happening all at once? https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task.cc:117: TaskComplete(); seems like you are completing the task twice. That does not seem right. https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/pick_request_task.h (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task.h:53: void HandleGetResults(QueueResults::GetRequestsResult, Can you focus on the outcome of the step when naming the method? https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task.h:60: // Step 2b. Handle deleting expired entries and notifying observers. On 2016/11/03 19:56:51, dougarnett wrote: > expired => stale I am happy with Expired, as long as we are consistent. https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/pick_request_task_builder.h (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task_builder.h:9: nit: space not needed. https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task_builder.h:20: class PickRequestTaskBuilder { Given the interface presented by this class it feels more like a factory and not a builder (in the design patterns sense). Do you think it would be reasonable to change it? Also, please add documentation. https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task_builder.h:31: PickRequestTask::RequestNotPickedCallback not_picked_callback, Could you use const & for both callbacks, unless you know of a reason not to. https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/request_queue.cc (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/request_queue.cc:124: // Using the PickerContext, create a picker task nit: period at the end. https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/request_queue.h (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/request_queue.h:102: // Takes ownership of the builder. if that is the case, it should be using a unique ptr. Also is that done in production code or for testing only?
https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/pick_request_task.h (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task.h:60: // Step 2b. Handle deleting expired entries and notifying observers. On 2016/11/03 22:00:29, fgorski wrote: > On 2016/11/03 19:56:51, dougarnett wrote: > > expired => stale > > I am happy with Expired, as long as we are consistent. In case my point is not clear, we should be culling at least 3 different types of request situations here (this really being the cleanup task): over expiry time, over start count, and over completed count. I can accept Expired as long as it is a term we understand applies to all the types.
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_...)
https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... components/offline_pages/background/pick_request_task.cc:76: RemoveExpiredEntries(std::move(expired_requests)); On 2016/11/03 19:56:51, dougarnett wrote: > On 2016/11/03 19:26:29, Pete Williamson wrote: > > On 2016/11/03 17:01:22, dougarnett wrote: > > > Generalize to remove other stale requests too? Maybe SplitRequests > identifies > > > additional cases and then can remove from RequestConditionsSatisfied(). > > > > Renamed to "RemoveStaleRequests" > > Still some more work though to check started_attempt_count and > completed_attempt_count against policy limits in SplitRequests? I'm not sure it is better to clean them here. We can clean them well when we detect the condition. We don't detect the condition of something expiring. Today we clean these up in AbortRequestAttempt() and OfflinerDoneCallback(). If you do want this to be done here, let me know, and I'll add a TODO and a bug (but this changelist is complex enough already). https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task.cc:117: TaskComplete(); On 2016/11/03 22:00:29, fgorski wrote: > seems like you are completing the task twice. That does not seem right. Here we have two tasks - The first task is choosing, and this TaskComplete ends the choosing task. The second task is started by calling request_queue_->RemoveRequests(), which creates a new task under the hood. If we don't complete the first task, the second task will never start, and the callback will never get called, so we really do need to call TaskComplete here. I removed the second call to TaskComplete in OnRequestsExpired(), since the RemoveRequests() call will have its own TaskComplete for the Remove task it builds. Now that I see that we need to use a second task (It was hidden under the request_queue call), I want to refactor the pruning out as you (FGorski) suggest, but I'd prefer to delay that to a different changelist, this one is big enough already. https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task.cc:126: std::vector<std::unique_ptr<SavePageRequest>> expired_requests) { On 2016/11/03 19:56:51, dougarnett wrote: > expired_requests => stale_requests ? Done. https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/pick_request_task.h (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task.h:53: void HandleGetResults(QueueResults::GetRequestsResult, On 2016/11/03 22:00:29, fgorski wrote: > Can you focus on the outcome of the step when naming the method? How about "ChooseAndPrune"? https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task.h:60: // Step 2b. Handle deleting expired entries and notifying observers. On 2016/11/03 22:33:15, dougarnett wrote: > On 2016/11/03 22:00:29, fgorski wrote: > > On 2016/11/03 19:56:51, dougarnett wrote: > > > expired => stale > > > > I am happy with Expired, as long as we are consistent. > > In case my point is not clear, we should be culling at least 3 different types > of request situations here (this really being the cleanup task): over expiry > time, over start count, and over completed count. I can accept Expired as long > as it is a term we understand applies to all the types. I changed it to stale everywhere. I'm not sure that this is the right place to remove the other cleanup items, though. https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/pick_request_task_builder.h (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task_builder.h:9: On 2016/11/03 22:00:29, fgorski wrote: > nit: space not needed. Done. https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task_builder.h:20: class PickRequestTaskBuilder { On 2016/11/03 22:00:29, fgorski wrote: > Given the interface presented by this class it feels more like a factory and not > a builder (in the design patterns sense). Do you think it would be reasonable to > change it? > > Also, please add documentation. Doc added. I'm not sure I understand your request. Do you mean just that I should change the name of the class from "builder" to "factory", or that the interface should somehow be different? https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task_builder.h:31: PickRequestTask::RequestNotPickedCallback not_picked_callback, On 2016/11/03 22:00:29, fgorski wrote: > Could you use const & for both callbacks, unless you know of a reason not to. Done. https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/request_queue.cc (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/request_queue.cc:124: // Using the PickerContext, create a picker task On 2016/11/03 22:00:29, fgorski wrote: > nit: period at the end. Done. https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/request_queue.h (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/request_queue.h:102: // Takes ownership of the builder. On 2016/11/03 22:00:29, fgorski wrote: > if that is the case, it should be using a unique ptr. > Also is that done in production code or for testing only? This is for production code. The idea is that we set it after the creation of the request queue so that the request coordinator factory doesn't need to build it at request queue time, so it can be built later. The intent is that it is easier to create a request_queue (which has lots of references all over the code). Let me know if you prefer, and I can instead set it in the factory. Also, changed to use a unique ptr.
https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... components/offline_pages/background/pick_request_task.cc:76: RemoveExpiredEntries(std::move(expired_requests)); On 2016/11/04 18:53:37, Pete Williamson wrote: > On 2016/11/03 19:56:51, dougarnett wrote: > > On 2016/11/03 19:26:29, Pete Williamson wrote: > > > On 2016/11/03 17:01:22, dougarnett wrote: > > > > Generalize to remove other stale requests too? Maybe SplitRequests > > identifies > > > > additional cases and then can remove from RequestConditionsSatisfied(). > > > > > > Renamed to "RemoveStaleRequests" > > > > Still some more work though to check started_attempt_count and > > completed_attempt_count against policy limits in SplitRequests? > > I'm not sure it is better to clean them here. We can clean them well when we > detect the condition. We don't detect the condition of something expiring. > > Today we clean these up in AbortRequestAttempt() and OfflinerDoneCallback(). > > If you do want this to be done here, let me know, and I'll add a TODO and a bug > (but this changelist is complex enough already). Yes, I want you to remove them here. If they are detected here it is because we never got the OfflinerDoneCallback (eg, crash or killed). This is bug 655178. Sure, you can do as a follow-up but this leads to bad bug with notification not getting cleared so we should fix for M-56.
https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... File components/offline_pages/background/pick_request_task_builder.h (right): https://codereview.chromium.org/2473553004/diff/20001/components/offline_page... components/offline_pages/background/pick_request_task_builder.h:20: class PickRequestTaskBuilder { On 2016/11/04 18:53:37, Pete Williamson wrote: > On 2016/11/03 22:00:29, fgorski wrote: > > Given the interface presented by this class it feels more like a factory and > not > > a builder (in the design patterns sense). Do you think it would be reasonable > to > > change it? > > > > Also, please add documentation. > > Doc added. > > I'm not sure I understand your request. Do you mean just that I should change > the name of the class from "builder" to "factory", or that the interface should > somehow be different? Yes, I think Factory suits this case better. Also, Please move the documentation to the .h file. I think you accidentally added it in the .cc file. https://codereview.chromium.org/2473553004/diff/60001/components/offline_page... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/60001/components/offline_page... components/offline_pages/background/pick_request_task.cc:133: base::Bind(&PickRequestTask::OnRequestsExpired, Does this work? Here is what I think happens here: PickRequestTask is going to be destroyed before the next task starts, which means you will not be able to run OnRequestsExpired... https://codereview.chromium.org/2473553004/diff/60001/components/offline_page... File components/offline_pages/background/request_queue.h (right): https://codereview.chromium.org/2473553004/diff/60001/components/offline_page... components/offline_pages/background/request_queue.h:103: void SetPickerBuilder(std::unique_ptr<PickRequestTaskBuilder>& builder) { don't pass unique_ptr by ref, please https://codereview.chromium.org/2473553004/diff/60001/components/offline_page... components/offline_pages/background/request_queue.h:104: picker_builder_.reset(builder.release()); picker_builder_ = std::move(builder); Is a common way to do this.
lgtm subject to adding TODO and addressing Filip's comments https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... components/offline_pages/background/pick_request_task.cc:76: RemoveExpiredEntries(std::move(expired_requests)); On 2016/11/04 19:28:26, dougarnett wrote: > On 2016/11/04 18:53:37, Pete Williamson wrote: > > On 2016/11/03 19:56:51, dougarnett wrote: > > > On 2016/11/03 19:26:29, Pete Williamson wrote: > > > > On 2016/11/03 17:01:22, dougarnett wrote: > > > > > Generalize to remove other stale requests too? Maybe SplitRequests > > > identifies > > > > > additional cases and then can remove from RequestConditionsSatisfied(). > > > > > > > > > Renamed to "RemoveStaleRequests" > > > > > > Still some more work though to check started_attempt_count and > > > completed_attempt_count against policy limits in SplitRequests? > > > > I'm not sure it is better to clean them here. We can clean them well when we > > detect the condition. We don't detect the condition of something expiring. > > > > Today we clean these up in AbortRequestAttempt() and OfflinerDoneCallback(). > > > > If you do want this to be done here, let me know, and I'll add a TODO and a > bug > > (but this changelist is complex enough already). > > Yes, I want you to remove them here. If they are detected here it > is because we never got the OfflinerDoneCallback (eg, crash or > killed). This is bug 655178. Sure, you can do as a follow-up but > this leads to bad bug with notification not getting cleared so > we should fix for M-56. Btw, I'm happy to do the follow-up here but I am not yet sure you agree this needs to be done so maybe we can chat in person today or tomorrow.
Adding the comment capturing what we discussed on Friday. https://codereview.chromium.org/2473553004/diff/60001/components/offline_page... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/60001/components/offline_page... components/offline_pages/background/pick_request_task.cc:132: request_queue_->RemoveRequests(stale_request_ids, As we discussed, this should be the store rather than the queue call. TaskComplete can be called as a shortcut, early, when there are no stale requests. Otherwise, completion of RemoveRequests should call TaskComplete.
petewil@chromium.org changed reviewers: + estade@chromium.org
estade: PTAL at offline_internals_ui_message_handler.cc/.h I split out the result types from RequestQueue into a new header. Since it is used in lots of places, it ended up getting used in offline_internals_ui_message_handler, which our team isn't on the owners list for. DougArnett, FGorski: I believe this addresses all your comments. https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/1/components/offline_pages/ba... components/offline_pages/background/pick_request_task.cc:76: RemoveExpiredEntries(std::move(expired_requests)); On 2016/11/07 17:09:07, dougarnett wrote: > On 2016/11/04 19:28:26, dougarnett wrote: > > On 2016/11/04 18:53:37, Pete Williamson wrote: > > > On 2016/11/03 19:56:51, dougarnett wrote: > > > > On 2016/11/03 19:26:29, Pete Williamson wrote: > > > > > On 2016/11/03 17:01:22, dougarnett wrote: > > > > > > Generalize to remove other stale requests too? Maybe SplitRequests > > > > identifies > > > > > > additional cases and then can remove from > RequestConditionsSatisfied(). > > > > > > > > > > > > Renamed to "RemoveStaleRequests" > > > > > > > > Still some more work though to check started_attempt_count and > > > > completed_attempt_count against policy limits in SplitRequests? > > > > > > I'm not sure it is better to clean them here. We can clean them well when > we > > > detect the condition. We don't detect the condition of something expiring. > > > > > > Today we clean these up in AbortRequestAttempt() and OfflinerDoneCallback(). > > > > > > If you do want this to be done here, let me know, and I'll add a TODO and a > > bug > > > (but this changelist is complex enough already). > > > > Yes, I want you to remove them here. If they are detected here it > > is because we never got the OfflinerDoneCallback (eg, crash or > > killed). This is bug 655178. Sure, you can do as a follow-up but > > this leads to bad bug with notification not getting cleared so > > we should fix for M-56. > > Btw, I'm happy to do the follow-up here but I am not yet sure you agree this > needs to be done so maybe we can chat in person today or tomorrow. I agree that it needs to be done (in the light of your comments). However, I hope to do it in a later changelist. https://codereview.chromium.org/2473553004/diff/60001/components/offline_page... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/60001/components/offline_page... components/offline_pages/background/pick_request_task.cc:132: request_queue_->RemoveRequests(stale_request_ids, On 2016/11/07 17:27:10, fgorski wrote: > As we discussed, this should be the store rather than the queue call. > > TaskComplete can be called as a shortcut, early, when there are no stale > requests. > > Otherwise, completion of RemoveRequests should call TaskComplete. Done. https://codereview.chromium.org/2473553004/diff/60001/components/offline_page... components/offline_pages/background/pick_request_task.cc:133: base::Bind(&PickRequestTask::OnRequestsExpired, On 2016/11/04 21:41:28, fgorski wrote: > Does this work? > > Here is what I think happens here: > PickRequestTask is going to be destroyed before the next task starts, which > means you will not be able to run OnRequestsExpired... It does now that I've gotten rid of the second CompleteTask(), which I could do because we refactored to use RequestQueueStore instead of RequestQueue in the PickRequestTask class. https://codereview.chromium.org/2473553004/diff/60001/components/offline_page... File components/offline_pages/background/request_queue.h (right): https://codereview.chromium.org/2473553004/diff/60001/components/offline_page... components/offline_pages/background/request_queue.h:103: void SetPickerBuilder(std::unique_ptr<PickRequestTaskBuilder>& builder) { On 2016/11/04 21:41:28, fgorski wrote: > don't pass unique_ptr by ref, please Done. https://codereview.chromium.org/2473553004/diff/60001/components/offline_page... components/offline_pages/background/request_queue.h:104: picker_builder_.reset(builder.release()); On 2016/11/04 21:41:28, fgorski wrote: > picker_builder_ = std::move(builder); > Is a common way to do this. Done.
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...)
Looks like some tests broke with the merge
https://codereview.chromium.org/2473553004/diff/100001/components/offline_pag... File components/offline_pages/background/queue_results.h (right): https://codereview.chromium.org/2473553004/diff/100001/components/offline_pag... components/offline_pages/background/queue_results.h:15: struct QueueResults { why this struct? seems like these types can be first-class members of the offline_pages namespace. offline_pages::QueueResults::GetRequestsResult::SUCCESS is just a little bit too verbose imo.
tests fixed, comments all addressed. https://codereview.chromium.org/2473553004/diff/100001/components/offline_pag... File components/offline_pages/background/queue_results.h (right): https://codereview.chromium.org/2473553004/diff/100001/components/offline_pag... components/offline_pages/background/queue_results.h:15: struct QueueResults { On 2016/11/08 14:40:30, Evan Stade wrote: > why this struct? seems like these types can be first-class members of the > offline_pages namespace. > > offline_pages::QueueResults::GetRequestsResult::SUCCESS is just a little bit too > verbose imo. Done.
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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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_...)
https://codereview.chromium.org/2473553004/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc (right): https://codereview.chromium.org/2473553004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:157: if (result == offline_pages::QueueResults::GetRequestsResult::SUCCESS) { doesn't seem this was updated?
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...
estate CR feedback, and took another merge https://codereview.chromium.org/2473553004/diff/140001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc (right): https://codereview.chromium.org/2473553004/diff/140001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:157: if (result == offline_pages::QueueResults::GetRequestsResult::SUCCESS) { On 2016/11/08 23:13:36, Evan Stade wrote: > doesn't seem this was updated? Ah, I did my grep in the directory where most of the files were, but not this one. I went back and double checked all files in the changelist.
offline_internals_ui_message_handler.* lgtm
https://codereview.chromium.org/2473553004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h (left): https://codereview.chromium.org/2473553004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:66: offline_pages::RequestQueue::GetRequestsResult result, wait a second, enum class lets you forward declare this. namespace offline_pages { enum class GetRequestsResult; } at the top of this file instead of the header include.
https://codereview.chromium.org/2473553004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h (left): https://codereview.chromium.org/2473553004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:66: offline_pages::RequestQueue::GetRequestsResult result, On 2016/11/09 00:43:03, Evan Stade wrote: > wait a second, enum class lets you forward declare this. > > namespace offline_pages { > enum class GetRequestsResult; > } > > at the top of this file instead of the header include. Done.
lgtm with nit https://codereview.chromium.org/2473553004/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h (right): https://codereview.chromium.org/2473553004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:16: // Forward declare the result types we use from the request queue. nit: unnecessary comment
A few more comments. (Mostly task implementation and nits) https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/mark_attempt_completed_task.h (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/mark_attempt_completed_task.h:12: #include "components/offline_pages/background/request_queue_store.h" nit: forward declare RequestQueueStore and remove this header. It was here for UdpateRequestsResult as much as for the store. You can remove it now. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:60: not_picked_callback_.Run(false); Early call to TaskComplete() is in order here (before return). Please check your code for cases like that. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:69: std::vector<int64_t> expired_request_ids; looks like this should be a type of the third parameter of SplitRequests. WDYT? https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:76: // Continue Async Processing. Add early return here if expired_request_ids is empty. (And remember to include TaskComplete()) This prevents one extra cross thread roundtrip. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:98: if (search != disabled_requests_.end()) { nit: drop {} https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:103: if (!RequestConditionsSatisfied(valid_requests[i].get())) I am missing a comment here explaining why we first check for user requested tasks and then check if the task matches the conditions. Could you please add one? https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:111: if (picked_request != nullptr) { nit: braces not necessary for both clauses. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:119: // TODO(petewil): Does that really need to be done on the task queue? Hard to this comment/todo probably needs revisiting after moving to the store-based approach. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:126: std::vector<int64_t> stale_request_ids; and these 3 lines is exactly why my comment about SplitRequests was valid. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:152: std::vector<std::unique_ptr<SavePageRequest>>* expired_requests) { this one should be std::vector<int64_t> https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task.h (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.h:101: // Member variables, all pointers unowned. nit: not owned. "unown" does not seem to be a word, except a Pokemon name. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task_factory.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.cc:15: OfflinerPolicy* policy, missing include https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.cc:16: RequestNotifier* notifier, missing include https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.cc:28: std::unique_ptr<PickRequestTask> task; nit: why not construct it in one line? https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task_factory.h (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.h:8: #include "components/offline_pages/background/device_conditions.h" nit: can this be forward declared? https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.h:9: #include "components/offline_pages/background/pick_request_task.h" nit: can this be forward declared? https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.h:10: #include "components/offline_pages/background/request_coordinator.h" is this needed? https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.h:17: class RequestQueue; should this be store? https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.h:32: std::set<int64_t>& disabled_requests); missing includes for set and int64_t https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task_unittest.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_unittest.cc:81: SavePageRequest last_expired_request_; is this field used? https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_unittest.cc:85: class PickRequestTaskTest : public testing::Test { Can you amend the tests to cover TaskComplete() calls? SetTaskCompletionCallbackForTesting is the method to use. Alternatively (or in addition to that) tests for this functionality in on the queue would help you discover missing taskcomplete() calls. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_unittest.cc:276: ; nit: ; https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/request_queue.h (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/request_queue.h:99: // Takes ownership of the factory. ownership transfer is implied by the parameter type. please document how is that used. Bonus points for explaining why it does not happen on construction. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/request_queue.h:100: void SetPickerFactory(std::unique_ptr<PickRequestTaskFactory> factory) { why is the factory injected by setter? https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/request_queue.h:120: // Builds PickRequestTask objects nit: please add period at the end. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/request_queue_unittest.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/request_queue_unittest.cc:536: TEST_F(RequestQueueTest, CleanStaleRequests) { please add a comment to this test: Request expiration happens when PickNextRequest is called on the queue. (I see there is one in code, but still) also, what about tests for picking, at least some basic ones, e.g. covering different taskComplete? https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/core/task_queue.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/core/task_queue.cc:33: DVLOG(2) << "running? " << HasRunningTask() << ", pending? " is this needed?
https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:71: expired_request_ids.push_back(request->request_id()); this *_ids vector not actually used?
https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:103: if (!RequestConditionsSatisfied(valid_requests[i].get())) On 2016/11/09 17:58:06, fgorski wrote: > I am missing a comment here explaining why we first check for user requested > tasks and then check if the task matches the conditions. > Could you please add one? Btw, that non_user_requested_tasks_remaining bool is really about returning indication of what TriggerConditions, if any, makes sense based on what is in the queue. Might be nice to define a more targeted enum in the future for this instead of this hard-to-name bool.
https://codereview.chromium.org/2473553004/diff/180001/chrome/browser/ui/webu... File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h (right): https://codereview.chromium.org/2473553004/diff/180001/chrome/browser/ui/webu... chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h:16: // Forward declare the result types we use from the request queue. On 2016/11/09 01:00:30, Evan Stade wrote: > nit: unnecessary comment Removed. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/mark_attempt_completed_task.h (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/mark_attempt_completed_task.h:12: #include "components/offline_pages/background/request_queue_store.h" On 2016/11/09 17:58:06, fgorski wrote: > nit: forward declare RequestQueueStore and remove this header. It was here for > UdpateRequestsResult as much as for the store. You can remove it now. Done. Would you like me to aslo make a parallel change in MarkAttemptStarted and MarkAttemptAborted? (I don't have them open for this change, but I could add them.) https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:60: not_picked_callback_.Run(false); On 2016/11/09 17:58:07, fgorski wrote: > Early call to TaskComplete() is in order here (before return). > > Please check your code for cases like that. Done, and checked the code for more cases (I did not find any in pick_request_task.cc) https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:69: std::vector<int64_t> expired_request_ids; On 2016/11/09 17:58:07, fgorski wrote: > looks like this should be a type of the third parameter of SplitRequests. > WDYT? Changed (but I actually replaced the second parameter instead of adding a third). https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:71: expired_request_ids.push_back(request->request_id()); On 2016/11/09 18:23:51, dougarnett wrote: > this *_ids vector not actually used? Good catch, removed (it got copied into the RemoveStaleRequests function, but I forgot to delete it here). https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:76: // Continue Async Processing. On 2016/11/09 17:58:07, fgorski wrote: > Add early return here if expired_request_ids is empty. (And remember to include > TaskComplete()) This prevents one extra cross thread roundtrip. Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:98: if (search != disabled_requests_.end()) { On 2016/11/09 17:58:06, fgorski wrote: > nit: drop {} Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:103: if (!RequestConditionsSatisfied(valid_requests[i].get())) On 2016/11/09 17:58:06, fgorski wrote: > I am missing a comment here explaining why we first check for user requested > tasks and then check if the task matches the conditions. > Could you please add one? Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:103: if (!RequestConditionsSatisfied(valid_requests[i].get())) On 2016/11/09 18:39:56, dougarnett wrote: > On 2016/11/09 17:58:06, fgorski wrote: > > I am missing a comment here explaining why we first check for user requested > > tasks and then check if the task matches the conditions. > > Could you please add one? > > Btw, that non_user_requested_tasks_remaining bool is really about returning > indication of what TriggerConditions, if any, makes sense based on what is in > the queue. Might be nice to define a more targeted enum in the future for this > instead of this hard-to-name bool. TODO added. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:111: if (picked_request != nullptr) { On 2016/11/09 17:58:07, fgorski wrote: > nit: braces not necessary for both clauses. Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:119: // TODO(petewil): Does that really need to be done on the task queue? Hard to On 2016/11/09 17:58:07, fgorski wrote: > this comment/todo probably needs revisiting after moving to the store-based > approach. The comment is current. It is basically explaining your original suggestion that it might be better to have a separate task for queue cleanup instead of piggybacking on the choosing task for efficiency reasons. LMK if you think I can improve the wording to make it more obvious. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:126: std::vector<int64_t> stale_request_ids; On 2016/11/09 17:58:07, fgorski wrote: > and these 3 lines is exactly why my comment about SplitRequests was valid. Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:152: std::vector<std::unique_ptr<SavePageRequest>>* expired_requests) { On 2016/11/09 17:58:07, fgorski wrote: > this one should be std::vector<int64_t> Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task.h (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.h:101: // Member variables, all pointers unowned. On 2016/11/09 17:58:07, fgorski wrote: > nit: not owned. > > "unown" does not seem to be a word, except a Pokemon name. Unown is not a word, but unowned does seem to be, English is strange that way. I do see lots of references to "unowned" on the internet, and in our source code, and in dictionaries searchable by the internet. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task_factory.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.cc:15: OfflinerPolicy* policy, On 2016/11/09 17:58:07, fgorski wrote: > missing include Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.cc:16: RequestNotifier* notifier, On 2016/11/09 17:58:07, fgorski wrote: > missing include Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.cc:28: std::unique_ptr<PickRequestTask> task; On 2016/11/09 17:58:07, fgorski wrote: > nit: why not construct it in one line? Done. I didn't do it in the first place because I thought it made the code easier to read and use in a debugger. I realize others have different preferences for what makes code easy to read. We don't normally use a symbolic debugger for our android code, so that is likely not an issue here. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task_factory.h (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.h:8: #include "components/offline_pages/background/device_conditions.h" On 2016/11/09 17:58:07, fgorski wrote: > nit: can this be forward declared? Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.h:9: #include "components/offline_pages/background/pick_request_task.h" On 2016/11/09 17:58:07, fgorski wrote: > nit: can this be forward declared? Nope, needed for the callback types. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.h:10: #include "components/offline_pages/background/request_coordinator.h" On 2016/11/09 17:58:07, fgorski wrote: > is this needed? Removed. (and then added in a few places that were depending on it, and had been getting it from here) https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.h:17: class RequestQueue; On 2016/11/09 17:58:07, fgorski wrote: > should this be store? Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.h:32: std::set<int64_t>& disabled_requests); On 2016/11/09 17:58:07, fgorski wrote: > missing includes for set and int64_t Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task_unittest.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_unittest.cc:81: SavePageRequest last_expired_request_; On 2016/11/09 17:58:07, fgorski wrote: > is this field used? Yes, it is used in the ChooseNonExpiredRequest test to verify that the request we expected to get expired actually got expired. It is used indirectly via the last_expired_request() accessor function. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_unittest.cc:85: class PickRequestTaskTest : public testing::Test { On 2016/11/09 17:58:07, fgorski wrote: > Can you amend the tests to cover TaskComplete() calls? > > SetTaskCompletionCallbackForTesting is the method to use. > > Alternatively (or in addition to that) tests for this functionality in on the > queue would help you discover missing taskcomplete() calls. Added tests in this class. I'm not sure how to add a test to the task queue tests to make sure that TackComplete() is always called, if that is what you meant. We could add a complete called tracker to the task_class, and check it manually from the tests, is that what you meant? https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_unittest.cc:276: ; On 2016/11/09 17:58:07, fgorski wrote: > nit: ; Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/request_queue.h (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/request_queue.h:99: // Takes ownership of the factory. On 2016/11/09 17:58:07, fgorski wrote: > ownership transfer is implied by the parameter type. > > please document how is that used. > Bonus points for explaining why it does not happen on construction. Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/request_queue.h:100: void SetPickerFactory(std::unique_ptr<PickRequestTaskFactory> factory) { On 2016/11/09 17:58:07, fgorski wrote: > why is the factory injected by setter? I made a design choice to not require a PickerFactory to make the RequestQueue easier to construct since we use it in so many places. This will ease use of the RequestQueue in unit tests and everywhere else besides the RequestCoordinator. If you disagree, my fallback plan is to just require a PickerFactory to make a RequestQueue, and make stub picker factories for all the unit tests that need a RequestQueue. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/request_queue.h:120: // Builds PickRequestTask objects On 2016/11/09 17:58:07, fgorski wrote: > nit: please add period at the end. Done. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/request_queue_unittest.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/request_queue_unittest.cc:536: TEST_F(RequestQueueTest, CleanStaleRequests) { On 2016/11/09 17:58:07, fgorski wrote: > please add a comment to this test: > Request expiration happens when PickNextRequest is called on the queue. (I see > there is one in code, but still) > > also, what about tests for picking, at least some basic ones, e.g. covering > different taskComplete? 1. Comments added. 2. Generally I try to test something at the lowest level possible, and we already have those tests in the pick request task unit test. Why also add tests here? If it is just to test TaskComplete, it is already tested now. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/core/task_queue.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/core/task_queue.cc:33: DVLOG(2) << "running? " << HasRunningTask() << ", pending? " On 2016/11/09 17:58:07, fgorski wrote: > is this needed? Yes. I've added it about 5 times while debugging, which normally tells me we need a permanent log statement. It is level 2, so it will be out of our face almost all the time, but when I did have a problem with a task not starting, this was almost always the line that let me know the problem.
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...)
Mostly looks good. Let me know if you need help fixing the tests. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/mark_attempt_completed_task.h (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/mark_attempt_completed_task.h:12: #include "components/offline_pages/background/request_queue_store.h" On 2016/11/09 23:53:55, Pete Williamson wrote: > On 2016/11/09 17:58:06, fgorski wrote: > > nit: forward declare RequestQueueStore and remove this header. It was here for > > UdpateRequestsResult as much as for the store. You can remove it now. > > Done. Would you like me to aslo make a parallel change in MarkAttemptStarted > and MarkAttemptAborted? (I don't have them open for this change, but I could > add them.) Not right now. If you can avoid adding files to this changelist that would probably be better for your productivity. You are welcome to start a new CL for clean up thou. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:69: std::vector<int64_t> expired_request_ids; On 2016/11/09 23:53:55, Pete Williamson wrote: > On 2016/11/09 17:58:07, fgorski wrote: > > looks like this should be a type of the third parameter of SplitRequests. > > WDYT? > > Changed (but I actually replaced the second parameter instead of adding a > third). You are technically correct. :) https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:119: // TODO(petewil): Does that really need to be done on the task queue? Hard to On 2016/11/09 23:53:55, Pete Williamson wrote: > On 2016/11/09 17:58:07, fgorski wrote: > > this comment/todo probably needs revisiting after moving to the store-based > > approach. > > The comment is current. It is basically explaining your original suggestion > that it might be better to have a separate task for queue cleanup instead of > piggybacking on the choosing task for efficiency reasons. LMK if you think I > can improve the wording to make it more obvious. OK. Reread this and it can stay this way. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task.h (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.h:101: // Member variables, all pointers unowned. On 2016/11/09 23:53:55, Pete Williamson wrote: > On 2016/11/09 17:58:07, fgorski wrote: > > nit: not owned. > > > > "unown" does not seem to be a word, except a Pokemon name. > Unown is not a word, but unowned does seem to be, English is strange that way. I > do see lots of references to "unowned" on the internet, and in our source code, > and in dictionaries searchable by the internet. Cool, but having read the definition: adjective 1. not having an owner. 2. not admitted to; unacknowledged. I think it might still be used incorrectly here. Leaving it up to you. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task_factory.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.cc:28: std::unique_ptr<PickRequestTask> task; On 2016/11/09 23:53:55, Pete Williamson wrote: > On 2016/11/09 17:58:07, fgorski wrote: > > nit: why not construct it in one line? > > Done. I didn't do it in the first place because I thought it made the code > easier to read and use in a debugger. I realize others have different > preferences for what makes code easy to read. We don't normally use a symbolic > debugger for our android code, so that is likely not an issue here. Acknowledged. https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task_unittest.cc (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task_unittest.cc:81: SavePageRequest last_expired_request_; On 2016/11/09 23:53:56, Pete Williamson wrote: > On 2016/11/09 17:58:07, fgorski wrote: > > is this field used? > > Yes, it is used in the ChooseNonExpiredRequest test to verify that the request > we expected to get expired actually got expired. It is used indirectly via the > last_expired_request() accessor function. Yeah, I meant to remove this comment, once I unfolded the code. https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:106: // that they are scheduled when we run out of user requested tasks, so Yes, but what if they don't satisfy the condition, why do we count them then? https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... File components/offline_pages/background/pick_request_task_factory.h (right): https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.h:8: #include <cstdint> stdint.h is what chromium uses for this. https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... File components/offline_pages/background/pick_request_task_unittest.cc (right): https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... components/offline_pages/background/pick_request_task_unittest.cc:149: conditions, disabled_requests_); Set the callback for task completion right about here, please.
https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... File components/offline_pages/background/pick_request_task.h (right): https://codereview.chromium.org/2473553004/diff/180001/components/offline_pag... components/offline_pages/background/pick_request_task.h:101: // Member variables, all pointers unowned. On 2016/11/10 19:15:50, fgorski wrote: > On 2016/11/09 23:53:55, Pete Williamson wrote: > > On 2016/11/09 17:58:07, fgorski wrote: > > > nit: not owned. > > > > > > "unown" does not seem to be a word, except a Pokemon name. > > Unown is not a word, but unowned does seem to be, English is strange that way. > I > > do see lots of references to "unowned" on the internet, and in our source > code, > > and in dictionaries searchable by the internet. > > Cool, but having read the definition: > adjective > 1. not having an owner. > 2. not admitted to; unacknowledged. > > I think it might still be used incorrectly here. Leaving it up to you. OK, see if you like the new wording. https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:106: // that they are scheduled when we run out of user requested tasks, so On 2016/11/10 19:15:50, fgorski wrote: > Yes, but what if they don't satisfy the condition, why do we count them then? We are using this flag to see if we need to make sure we have a scheduled task for later to check to see if they satisfy the condition later. If something satisfies the conditions, we will run now, and we don't need to put a task for later on, we can do that after the next request finishes. I revised the wording again, LMK if the new wording is not sufficient. https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... File components/offline_pages/background/pick_request_task_factory.h (right): https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... components/offline_pages/background/pick_request_task_factory.h:8: #include <cstdint> On 2016/11/10 19:15:50, fgorski wrote: > stdint.h is what chromium uses for this. Done. https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... File components/offline_pages/background/pick_request_task_unittest.cc (right): https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... components/offline_pages/background/pick_request_task_unittest.cc:149: conditions, disabled_requests_); On 2016/11/10 19:15:50, fgorski wrote: > Set the callback for task completion right about here, please. Done.
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... File components/offline_pages/background/pick_request_task.cc (right): https://codereview.chromium.org/2473553004/diff/200001/components/offline_pag... components/offline_pages/background/pick_request_task.cc:106: // that they are scheduled when we run out of user requested tasks, so On 2016/11/10 23:43:25, Pete Williamson wrote: > On 2016/11/10 19:15:50, fgorski wrote: > > Yes, but what if they don't satisfy the condition, why do we count them then? > > We are using this flag to see if we need to make sure we have a scheduled task > for later to check to see if they satisfy the condition later. If something > satisfies the conditions, we will run now, and we don't need to put a task for > later on, we can do that after the next request finishes. I revised the > wording again, LMK if the new wording is not sufficient. Looks better! Thanks.
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, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2473553004/#ps220001 (title: "Fix unit tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
components/offline_pages/background/request_coordinator.cc:
While running git apply --index -p1;
error: patch failed:
components/offline_pages/background/request_coordinator.cc:16
error: components/offline_pages/background/request_coordinator.cc: patch does
not apply
Patch: components/offline_pages/background/request_coordinator.cc
Index: components/offline_pages/background/request_coordinator.cc
diff --git a/components/offline_pages/background/request_coordinator.cc
b/components/offline_pages/background/request_coordinator.cc
index
098024afd8436c03a6875d2c1f49a780973de2b2..a23e7f1914ab1b0614787b0ec54cbf1f9c2b0c85
100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -16,7 +16,6 @@
#include "base/time/time.h"
#include "components/offline_pages/background/offliner_factory.h"
#include "components/offline_pages/background/offliner_policy.h"
-#include "components/offline_pages/background/request_picker.h"
#include "components/offline_pages/background/save_page_request.h"
#include "components/offline_pages/client_policy_controller.h"
#include "components/offline_pages/offline_page_item.h"
@@ -180,8 +179,9 @@ RequestCoordinator::RequestCoordinator(
immediate_schedule_callback_(base::Bind(&EmptySchedulerCallback)),
weak_ptr_factory_(this) {
DCHECK(policy_ != nullptr);
- picker_.reset(
- new RequestPicker(queue_.get(), policy_.get(), this, &event_logger_));
+ std::unique_ptr<PickRequestTaskFactory> picker_factory(
+ new PickRequestTaskFactory(policy_.get(), this, &event_logger_));
+ queue_->SetPickerFactory(std::move(picker_factory));
}
RequestCoordinator::~RequestCoordinator() {}
@@ -231,7 +231,7 @@ void RequestCoordinator::GetAllRequests(const
GetRequestsCallback& callback) {
void RequestCoordinator::GetQueuedRequestsCallback(
const GetRequestsCallback& callback,
- RequestQueue::GetRequestsResult result,
+ GetRequestsResult result,
std::vector<std::unique_ptr<SavePageRequest>> requests) {
callback.Run(std::move(requests));
}
@@ -266,7 +266,7 @@ void
RequestCoordinator::StopPrerendering(Offliner::RequestStatus stop_status) {
}
void RequestCoordinator::GetRequestsForSchedulingCallback(
- RequestQueue::GetRequestsResult result,
+ GetRequestsResult result,
std::vector<std::unique_ptr<SavePageRequest>> requests) {
bool user_requested = false;
@@ -304,8 +304,8 @@ bool RequestCoordinator::CancelActiveRequestIfItMatches(
void RequestCoordinator::AbortRequestAttempt(const SavePageRequest& request) {
if (request.started_attempt_count() >= policy_->GetMaxStartedTries()) {
- const BackgroundSavePageResult result(
- BackgroundSavePageResult::START_COUNT_EXCEEDED);
+ const RequestNotifier::BackgroundSavePageResult result(
+ RequestNotifier::BackgroundSavePageResult::START_COUNT_EXCEEDED);
event_logger_.RecordDroppedSavePageRequest(request.client_id().name_space,
result, request.request_id());
RemoveAttemptedRequest(request, result);
@@ -320,7 +320,7 @@ void RequestCoordinator::AbortRequestAttempt(const
SavePageRequest& request) {
void RequestCoordinator::RemoveAttemptedRequest(
const SavePageRequest& request,
- BackgroundSavePageResult result) {
+ RequestNotifier::BackgroundSavePageResult result) {
std::vector<int64_t> remove_requests;
remove_requests.push_back(request.request_id());
queue_->RemoveRequests(remove_requests,
@@ -339,10 +339,10 @@ void RequestCoordinator::MarkAttemptAbortedDone(
NotifyChanged(result->updated_items.at(0));
} else {
DVLOG(1) << "Failed to mark request aborted: " << request_id;
- RequestQueue::UpdateRequestResult request_result =
+ UpdateRequestResult request_result =
result->store_state != StoreState::LOADED
- ? RequestQueue::UpdateRequestResult::STORE_FAILURE
- : RequestQueue::UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
+ ? UpdateRequestResult::STORE_FAILURE
+ : UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
event_logger_.RecordUpdateRequestFailed(client_id.name_space,
request_result);
}
@@ -356,7 +356,7 @@ void RequestCoordinator::RemoveRequests(
request_ids,
base::Bind(&RequestCoordinator::HandleRemovedRequestsAndCallback,
weak_ptr_factory_.GetWeakPtr(), callback,
- BackgroundSavePageResult::REMOVED));
+ RequestNotifier::BackgroundSavePageResult::REMOVED));
// Record the network quality when this request is made.
if (network_quality_estimator_) {
@@ -419,7 +419,7 @@ RequestCoordinator::GetConnectionType() {
}
void RequestCoordinator::AddRequestResultCallback(
- RequestQueue::AddRequestResult result,
+ AddRequestResult result,
const SavePageRequest& request) {
NotifyAdded(request);
// Inform the scheduler that we have an outstanding task.
@@ -437,10 +437,10 @@ void RequestCoordinator::MarkAttemptCompletedDoneCallback(
NotifyChanged(result->updated_items.at(0));
} else {
DVLOG(1) << "Failed to mark request completed " << request_id;
- RequestQueue::UpdateRequestResult request_result =
+ UpdateRequestResult request_result =
result->store_state != StoreState::LOADED
- ? RequestQueue::UpdateRequestResult::STORE_FAILURE
- : RequestQueue::UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
+ ? UpdateRequestResult::STORE_FAILURE
+ : UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
event_logger_.RecordUpdateRequestFailed(client_id.name_space,
request_result);
}
@@ -472,7 +472,7 @@ void RequestCoordinator::CompletedRequestCallback(
void RequestCoordinator::HandleRemovedRequestsAndCallback(
const RemoveRequestsCallback& callback,
- BackgroundSavePageResult status,
+ RequestNotifier::BackgroundSavePageResult status,
std::unique_ptr<UpdateRequestsResult> result) {
// TODO(dougarnett): Define status code for user/api cancel and use here
// to determine whether to record cancel time UMA.
@@ -483,7 +483,7 @@ void RequestCoordinator::HandleRemovedRequestsAndCallback(
}
void RequestCoordinator::HandleRemovedRequests(
- BackgroundSavePageResult status,
+ RequestNotifier::BackgroundSavePageResult status,
std::unique_ptr<UpdateRequestsResult> result) {
for (const auto& request : result->updated_items)
NotifyCompleted(request, status);
@@ -514,6 +514,7 @@ void RequestCoordinator::HandleWatchdogTimeout() {
bool RequestCoordinator::StartProcessing(
const DeviceConditions& device_conditions,
const base::Callback<void(bool)>& callback) {
+ DVLOG(2) << "Scheduled " << __func__;
return StartProcessingInternal(ProcessingWindowState::SCHEDULED_WINDOW,
device_conditions, callback);
}
@@ -546,6 +547,7 @@ void RequestCoordinator::StartImmediatelyIfConnected() {
RequestCoordinator::OfflinerImmediateStartStatus
RequestCoordinator::TryImmediateStart() {
+ DVLOG(2) << "Immediate " << __func__;
// Make sure not already busy processing.
if (is_busy_)
return OfflinerImmediateStartStatus::BUSY;
@@ -603,14 +605,17 @@ void RequestCoordinator::TryNextRequest() {
return;
}
- // Choose a request to process that meets the available conditions.
- // This is an async call, and returns right away.
- picker_->ChooseNextRequest(base::Bind(&RequestCoordinator::RequestPicked,
- weak_ptr_factory_.GetWeakPtr()),
- base::Bind(&RequestCoordinator::RequestNotPicked,
- weak_ptr_factory_.GetWeakPtr()),
- current_conditions_.get(),
- disabled_requests_);
+ // Ask request queue to make a new PickRequestTask object, then put it on the
+ // task queue.
+ queue_->PickNextRequest(base::Bind(&RequestCoordinator::RequestPicked,
+ weak_ptr_factory_.GetWeakPtr()),
+ base::Bind(&RequestCoordinator::RequestNotPicked,
+ weak_ptr_factory_.GetWeakPtr()),
+ *current_conditions_.get(), disabled_requests_);
+ // TODO(petewil): Verify current_conditions has a good value on all calling
+ // paths. It is really more of a "last known conditions" than "current
+ // conditions". Consider having a call to Java to check the current
+ // conditions.
}
// Called by the request picker when a request has been picked.
@@ -687,10 +692,10 @@ void RequestCoordinator::StartOffliner(
// TODO(fgorski): what is the best result? Do we create a new status?
StopProcessing(Offliner::PRERENDERING_NOT_STARTED);
DVLOG(1) << "Failed to mark attempt started: " << request_id;
- RequestQueue::UpdateRequestResult request_result =
+ UpdateRequestResult request_result =
update_result->store_state != StoreState::LOADED
- ? RequestQueue::UpdateRequestResult::STORE_FAILURE
- : RequestQueue::UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
+ ? UpdateRequestResult::STORE_FAILURE
+ : UpdateRequestResult::REQUEST_DOES_NOT_EXIST;
event_logger_.RecordUpdateRequestFailed(client_namespace, request_result);
return;
}
@@ -753,18 +758,19 @@ void RequestCoordinator::OfflinerDoneCallback(const
SavePageRequest& request,
AbortRequestAttempt(request);
} else if (status == Offliner::RequestStatus::SAVED) {
// Remove the request from the queue if it succeeded.
- RemoveAttemptedRequest(request, BackgroundSavePageResult::SUCCESS);
- } else if (status == Offliner::RequestStatus::PRERENDERING_FAILED_NO_RETRY) {
RemoveAttemptedRequest(request,
- …
(message too large)
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, dougarnett@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2473553004/#ps240001 (title: "merge with latest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Request Picker task We are refactoring the Background Loading system to use async tasks. The advantage of this is that tasks can finish uninterrupted by other tasks which might interfere with them (better correctness, get rid of those pesky race conditions). To avoid circular header file dependencies, I moved several results objects from request_queue.h to a new file, queue_results.h, and the rename ended up touching quite a few files. This change refactors the RequestPicker to use a task object. BUG=651238 ========== to ========== Request Picker task We are refactoring the Background Loading system to use async tasks. The advantage of this is that tasks can finish uninterrupted by other tasks which might interfere with them (better correctness, get rid of those pesky race conditions). To avoid circular header file dependencies, I moved several results objects from request_queue.h to a new file, queue_results.h, and the rename ended up touching quite a few files. This change refactors the RequestPicker to use a task object. BUG=651238 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Request Picker task We are refactoring the Background Loading system to use async tasks. The advantage of this is that tasks can finish uninterrupted by other tasks which might interfere with them (better correctness, get rid of those pesky race conditions). To avoid circular header file dependencies, I moved several results objects from request_queue.h to a new file, queue_results.h, and the rename ended up touching quite a few files. This change refactors the RequestPicker to use a task object. BUG=651238 ========== to ========== Request Picker task We are refactoring the Background Loading system to use async tasks. The advantage of this is that tasks can finish uninterrupted by other tasks which might interfere with them (better correctness, get rid of those pesky race conditions). To avoid circular header file dependencies, I moved several results objects from request_queue.h to a new file, queue_results.h, and the rename ended up touching quite a few files. This change refactors the RequestPicker to use a task object. BUG=651238 Committed: https://crrev.com/3113e19fb5a58ae8dd2bbc7af58c7584486120c0 Cr-Commit-Position: refs/heads/master@{#431617} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/3113e19fb5a58ae8dd2bbc7af58c7584486120c0 Cr-Commit-Position: refs/heads/master@{#431617} |
