|
|
Created:
4 years, 3 months ago by fgorski Modified:
4 years, 2 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] Introduces TaskQueue to serialize tasks that asynchronously access SQLStore
* Adds a TaskQueue and Task interface
* Adds tests for TaskQueue.
* Introduces a core/ directory under components/offline_pages
BUG=645522, 651238
R=dimich@chromium.org
Committed: https://crrev.com/0674abb71a7737c42f368d6b68d81e86208012e3
Cr-Commit-Position: refs/heads/master@{#422269}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressing feedback from petewil #Patch Set 3 : Addressing gn problems, adding missing test file to build #
Total comments: 36
Patch Set 4 : Addressing CR feedback #
Total comments: 8
Patch Set 5 : Addressing final feedback #
Total comments: 4
Patch Set 6 : Addressing feedback from Dmitry #
Messages
Total messages: 40 (24 generated)
fgorski@chromium.org changed reviewers: + dougarnett@chromium.org, petewil@chromium.org
Patch 1/2 to discuss tomorrow. PTAL
On 2016/09/26 23:46:05, fgorski wrote: > Patch 1/2 to discuss tomorrow. PTAL Basically looks good
https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... File components/offline_pages/core/BUILD.gn (right): https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/BUILD.gn:11: "task_queue.cc", What happened to offline_page_header? Did it get removed, or is all its content in task_queue.cc now? https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... File components/offline_pages/core/task_queue.cc (right): https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.cc:14: void TaskQueue::Task::SetCompletionCallback( Why does this set both the runner and the callback? Maybe it should be called SetRunnerAndCallback()? I also wonder why we need a runner instead of the task queue using its own thread, more documentation might help. https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.cc:39: tasks_.push(std::move(task)); Should this be persistent? I'm guessing not, but I wanted to make sure we thought about it. https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.cc:63: DCHECK_EQ(task, current_task_.get()); This doesn't seem to call the completion for the task. If not here, where does it get called? https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... File components/offline_pages/core/task_queue.h (right): https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.h:26: // the tasks, that need to be run sequentially. New task will only be started, nit - clearer without the comma after "started" https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.h:40: class Task { Would it make the code less complicated to move task into its own file? Maybe more docs about how to use this: (Derive a calss from Task that implements your logic, add that to the TaskQueue, set it up for a runner, have task states that follow on eachother, use the same queue for all tasks using the same resouorce (such as the request queue). https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.h:59: // Complete call, which should be made at every point, where the task is nit - clearer without comma. https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.h:96: std::queue<std::unique_ptr<Task>> tasks_; Is FIFO the right policy? For example, if we aborted a request, deleting the queue entry, maybe that should happen before an update to the same request.
The CQ bit was checked by fgorski@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by fgorski@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...
PTAL. Pete's comments addressed, and hopefully tests running properly now. https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... File components/offline_pages/core/BUILD.gn (right): https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/BUILD.gn:11: "task_queue.cc", On 2016/09/27 23:49:47, Pete Williamson wrote: > What happened to offline_page_header? Did it get removed, or is all its content > in task_queue.cc now? I think this is a quirkiness of the diffing system. This patch creates this file. https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... File components/offline_pages/core/task_queue.cc (right): https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.cc:14: void TaskQueue::Task::SetCompletionCallback( On 2016/09/27 23:49:47, Pete Williamson wrote: > Why does this set both the runner and the callback? Maybe it should be called > SetRunnerAndCallback()? > > I also wonder why we need a runner instead of the task queue using its own > thread, more documentation might help. Because we want the completion to be executed on the right thread and outside of the call stack. I updated the SetCompletionCallback documentation in the header. https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.cc:39: tasks_.push(std::move(task)); On 2016/09/27 23:49:47, Pete Williamson wrote: > Should this be persistent? I'm guessing not, but I wanted to make sure we > thought about it. You are correct. It shouldn't be persisted. This is completely in memory serialization of execution. https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.cc:63: DCHECK_EQ(task, current_task_.get()); On 2016/09/27 23:49:47, Pete Williamson wrote: > This doesn't seem to call the completion for the task. If not here, where does > it get called? line 38 https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... File components/offline_pages/core/task_queue.h (right): https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.h:26: // the tasks, that need to be run sequentially. New task will only be started, On 2016/09/27 23:49:47, Pete Williamson wrote: > nit - clearer without the comma after "started" Done. https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.h:40: class Task { On 2016/09/27 23:49:47, Pete Williamson wrote: > Would it make the code less complicated to move task into its own file? > > Maybe more docs about how to use this: (Derive a calss from Task that > implements your logic, add that to the TaskQueue, set it up for a runner, have > task states that follow on eachother, use the same queue for all tasks using the > same resouorce (such as the request queue). This is mixing together task implementation guidance, task queue handling, and stuff that is already delivered by the task/task_queue implementations. I moved the class and added comments. We can refine it from there, if we learn a bit more about that. https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.h:59: // Complete call, which should be made at every point, where the task is On 2016/09/27 23:49:47, Pete Williamson wrote: > nit - clearer without comma. Done. https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/co... components/offline_pages/core/task_queue.h:96: std::queue<std::unique_ptr<Task>> tasks_; On 2016/09/27 23:49:47, Pete Williamson wrote: > Is FIFO the right policy? For example, if we aborted a request, deleting the > queue entry, maybe that should happen before an update to the same request. RC has full control over in what order the tasks are called.
Description was changed from ========== [Offline pages] Introduces TaskQueue to serialize tasks that asynchronously access SQLStore * Adds a TaskQueue and Task interface * Adds tests for TaskQueue. * Introduces a core/ directory under components/offline_pages BUG=645522 R=dimich@chromium.org ========== to ========== [Offline pages] Introduces TaskQueue to serialize tasks that asynchronously access SQLStore * Adds a TaskQueue and Task interface * Adds tests for TaskQueue. * Introduces a core/ directory under components/offline_pages BUG=645522, 651238 R=dimich@chromium.org ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:18: // To use TaskQueue: Maybe this would be better as "To use a Task" * Derive your new task from offline_pages::Task * ... * Have the calling code put the task on the TaskQueue using AddTask() https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:36: // add things like UMA in the Run method. maybe add: // The task will have an additional function for each async step, with // each step setting up the next step as the completion callback for // the current step. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:39: // Sets the completion callback and whatever is set on it before the task is We have lots of different kinds of completions - let's name them in such a way as to be able to tell them apart easily 1. The callback to the Task owner when everything is done 2. The callback to the Task Queue when everything is done 3. The callback into the task object when a single async step completes. It would be great to have visually different names for each of these concepts. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:41: // allows task owner to inject completion callback and specify appropriate Naive user question: How does the Task client get a task runner for its current thread? Might be good to add that to comments here. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:55: void Complete(); Maybe TaskComplete() to differentiate it a bit more visually from the completion callback? https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:63: DISALLOW_COPY_AND_ASSIGN(Task); You've disallowed copy and assign, should you also disallow move ctr and move assign? https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/task_queue.h (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_queue.h:25: // the tasks, that need to be run sequentially. New task will only be started This will read smoother without the comma. In this case, RC creates the Task inheriting from the task class, but RQ is the consumer of the TaskQueue, so this comment could be improved a bit. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/task_queue_unittest.cc (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_queue_unittest.cc:73: ConsumedResource resource; Would it be safer to have different resources for each task? (Maybe more robust) https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_queue_unittest.cc:100: TEST_F(OfflineTaskQueueTest, LeaveEearly) { nit - LeaveEearly -> LeaveEarly https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_queue_unittest.cc:111: resource.CompleteStep(); I don't see how this causes an early exit. Comment, perhaps? https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/task_unittest.cc (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_unittest.cc:67: TEST_F(OfflineTaskTest, LeaveEearly) { nit LeaveEearly -> LeaveEarly https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/test_task.h (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/test_task.h:27: // Sample test task. This should not be used as a example of task implementation Would it be worthwhile to make it a good example? Folks often turn to tests for a good sample. If not, maybe we could point them to a good example to use instead (the upcoming ChangeState Task).
lgtm with some requests to clarify comments https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:28: typedef base::Callback<void(Task*)> CompletionCallback; TaskCompletionCallback? https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:40: // put in the queue, will be overwritten. Setting the runner and callback here wording seems a bit confusing wrt overwritten and before put in the queue. Does it only take effect before it is put in the queue? Do you expect this to be used multiple times for a task? Do you expect the Task to keep changing it as it performs async operations? [Don't need to answer those questions - just meant to show how the wording might be confusing] https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:50: // Call |Complete| at every point when the task is finished so that task queue what does "at every point" mean? Is it finished more than once? https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/task_queue.cc (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_queue.cc:17: task->SetCompletionCallback( ah, so this is never meant to be used by the Task (it needs to define its own callback and call it as necessary). Perhaps the comment in the .h should specify that this is called by TaskQueue when it is added (didn't quite get that). https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/task_queue.h (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_queue.h:19: // Class for coordinating sets of asynchronous operations, which switch threads wording seems confusing to me - not sure if the "sets of operations" are Tasks or operations within a Task. Kind of seems like the former. Below you mention "multiple tasks in a message loop" and "run a single operation". Be good to try to clarify terminology here. Consider just this: Coordinates running asynchronous operations (Tasks) so that only one runs at a time to completion. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/test_task.h (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/test_task.h:42: void Step2(); Maybe good to promote the "step" terminology in the class definition and then this example fits it.
The CQ bit was checked by fgorski@chromium.org to run a CQ dry run
Addressed. PTAL https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:18: // To use TaskQueue: On 2016/09/29 00:54:43, Pete Williamson wrote: > Maybe this would be better as "To use a Task" > * Derive your new task from offline_pages::Task > * > ... > > * Have the calling code put the task on the TaskQueue using AddTask() Done. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:28: typedef base::Callback<void(Task*)> CompletionCallback; On 2016/09/29 15:40:04, dougarnett wrote: > TaskCompletionCallback? > Done. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:36: // add things like UMA in the Run method. On 2016/09/29 00:54:44, Pete Williamson wrote: > maybe add: > // The task will have an additional function for each async step, with > // each step setting up the next step as the completion callback for > // the current step. Done. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:39: // Sets the completion callback and whatever is set on it before the task is On 2016/09/29 00:54:44, Pete Williamson wrote: > We have lots of different kinds of completions - let's name them in such a way > as to be able to tell them apart easily > 1. The callback to the Task owner when everything is done > 2. The callback to the Task Queue when everything is done > 3. The callback into the task object when a single async step completes. > > It would be great to have visually different names for each of these concepts. I went with Doug's suggestion above and added to documentation. Let me know what you think. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:40: // put in the queue, will be overwritten. Setting the runner and callback here On 2016/09/29 15:40:05, dougarnett wrote: > wording seems a bit confusing wrt overwritten and before put in the queue. Does > it only take effect before it is put in the queue? Do you expect this to be used > multiple times for a task? Do you expect the Task to keep changing it as it > performs async operations? > [Don't need to answer those questions - just meant to show how the wording might > be confusing] Actually this raises a valid point. I'll add DCHECKs that it is only set once and make a comment that it should only be set by TaskQueue. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:41: // allows task owner to inject completion callback and specify appropriate On 2016/09/29 00:54:44, Pete Williamson wrote: > Naive user question: How does the Task client get a task runner for its current > thread? Might be good to add that to comments here. That is actually left for the caller to work out. e.g. base::ThreadTaskRunnerHandle::Get(). This has changed several times in the past, so I don't think it is worth documenting that way. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:50: // Call |Complete| at every point when the task is finished so that task queue On 2016/09/29 15:40:05, dougarnett wrote: > what does "at every point" mean? Is it finished more than once? Attempted clarification. Let me know. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:55: void Complete(); On 2016/09/29 00:54:44, Pete Williamson wrote: > Maybe TaskComplete() to differentiate it a bit more visually from the completion > callback? Done. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task.h:63: DISALLOW_COPY_AND_ASSIGN(Task); On 2016/09/29 00:54:43, Pete Williamson wrote: > You've disallowed copy and assign, should you also disallow move ctr and move > assign? Cool. It got me thinking. I don't think that is commonly done and I couldn't find a macro. Also notice that these operation don't create a new copy of a task, which is what this macro is for. The way task is used (enforced by TaskQueue interface) is through a unique_ptr, which provides the move semantics explicitly. Please, correct me if my understanding is incorrect. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/task_queue.cc (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_queue.cc:17: task->SetCompletionCallback( On 2016/09/29 15:40:05, dougarnett wrote: > ah, so this is never meant to be used by the Task (it needs to define its own > callback and call it as necessary). > Perhaps the comment in the .h should specify that this is called by TaskQueue > when it is added (didn't quite get that). Done. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/task_queue.h (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_queue.h:19: // Class for coordinating sets of asynchronous operations, which switch threads On 2016/09/29 15:40:05, dougarnett wrote: > wording seems confusing to me - not sure if the "sets of operations" are Tasks > or operations within a Task. Kind of seems like the former. Below you mention > "multiple tasks in a message loop" and "run a single operation". Be good to try > to clarify terminology here. > > Consider just this: Coordinates running asynchronous operations (Tasks) so that > only one runs at a time to completion. Done. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_queue.h:25: // the tasks, that need to be run sequentially. New task will only be started On 2016/09/29 00:54:44, Pete Williamson wrote: > This will read smoother without the comma. > > In this case, RC creates the Task inheriting from the task class, but RQ is the > consumer of the TaskQueue, so this comment could be improved a bit. Background component is the consumer, therefore that is where the tasks are defined, and that is where a TaskQueue is created and held. In case of RC, everything happens in the RQ. I rephrased the comment a bit. Let me know how to improve it more. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/task_queue_unittest.cc (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_queue_unittest.cc:73: ConsumedResource resource; On 2016/09/29 00:54:44, Pete Williamson wrote: > Would it be safer to have different resources for each task? (Maybe more > robust) No, why? The point is that tasks share the resource. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_queue_unittest.cc:100: TEST_F(OfflineTaskQueueTest, LeaveEearly) { On 2016/09/29 00:54:44, Pete Williamson wrote: > nit - LeaveEearly -> LeaveEarly Done. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_queue_unittest.cc:111: resource.CompleteStep(); On 2016/09/29 00:54:44, Pete Williamson wrote: > I don't see how this causes an early exit. Comment, perhaps? Done. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/task_unittest.cc (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/task_unittest.cc:67: TEST_F(OfflineTaskTest, LeaveEearly) { On 2016/09/29 00:54:44, Pete Williamson wrote: > nit LeaveEearly -> LeaveEarly Done. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... File components/offline_pages/core/test_task.h (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/test_task.h:27: // Sample test task. This should not be used as a example of task implementation On 2016/09/29 00:54:44, Pete Williamson wrote: > Would it be worthwhile to make it a good example? Folks often turn to tests for > a good sample. If not, maybe we could point them to a good example to use > instead (the upcoming ChangeState Task). No point in making this better as this is a test support. Add reference to ChangeRequqestsStateTask. https://codereview.chromium.org/2359933007/diff/40001/components/offline_page... components/offline_pages/core/test_task.h:42: void Step2(); On 2016/09/29 15:40:05, dougarnett wrote: > Maybe good to promote the "step" terminology in the class definition and then > this example fits it. No I don't believe that. I actually thought about renaming this to something better, but that actually fits the test scenario. What I expect in implementation is a valid name describing what the step is, like: UpdateRequestsState, SelectItemsToUpdate or something like that. See https://codereview.chromium.org/2372043002/diff/20001/components/offline_page... for example.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
dewittj@chromium.org changed reviewers: + dewittj@chromium.org
lgtm % comments https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... components/offline_pages/core/task.h:71: void TaskComplete(); Feels like TaskQueue should be a TaskDelegate with a Complete() function that can do the completion callback and know about the runner. Then this task API would be even simpler. You could have a SetDelegate API that TaskQueue calls, or take a TaskDelegate in the constructor. https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... File components/offline_pages/core/task_queue.cc (right): https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... components/offline_pages/core/task_queue.cc:33: if (CurrentlyRunning()) redundant with HasTasks, but I think that HasTasks should not check CurrentlyRunning. https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... File components/offline_pages/core/task_queue.h (right): https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... components/offline_pages/core/task_queue.h:38: bool HasTasks() const; This is misleading for the public API, it should not also check if running. https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... components/offline_pages/core/task_queue.h:40: bool CurrentlyRunning() const; CurrentlyRunningForTest
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 fgorski@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...
fgorski@chromium.org changed reviewers: + caitkp@chromium.org
Addressed. Cait, could you please stamp components/BUILD.gn? https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... components/offline_pages/core/task.h:71: void TaskComplete(); On 2016/09/29 18:40:55, dewittj wrote: > Feels like TaskQueue should be a TaskDelegate with a Complete() function that > can do the completion callback and know about the runner. Then this task API > would be even simpler. You could have a SetDelegate API that TaskQueue calls, > or take a TaskDelegate in the constructor. As we discussed, it may be harder to work with if we introduce the delegate and I don't want the Task to know about the Queue directly. I do however want the queue to have a final say about the completion callback when accepting the task. Leaving it as is for now. https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... File components/offline_pages/core/task_queue.cc (right): https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... components/offline_pages/core/task_queue.cc:33: if (CurrentlyRunning()) On 2016/09/29 18:40:56, dewittj wrote: > redundant with HasTasks, but I think that HasTasks should not check > CurrentlyRunning. Done. https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... File components/offline_pages/core/task_queue.h (right): https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... components/offline_pages/core/task_queue.h:38: bool HasTasks() const; On 2016/09/29 18:40:56, dewittj wrote: > This is misleading for the public API, it should not also check if running. Done. https://codereview.chromium.org/2359933007/diff/60001/components/offline_page... components/offline_pages/core/task_queue.h:40: bool CurrentlyRunning() const; On 2016/09/29 18:40:56, dewittj wrote: > CurrentlyRunningForTest Renamed to IsCurrentlyRunning(). It is used internally and not just for testing. It is a const method so no harm is done from checking it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm with couple of small suggestions: https://codereview.chromium.org/2359933007/diff/80001/components/offline_page... File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/80001/components/offline_page... components/offline_pages/core/task.h:61: void SetTaskCompletionCallback( It took me to read the long comment and also verify implementation to understand that this is not the Completion Callback I am looking for... If this is public for test, it'd be nice to call it SetTaskCompletionCallbackForTest as we have presubmit that checks for these called outside of tests. However it is not here only for test, contrary to what the comment says. It is here also for TaskQueue. Could it be safer having TaskQueue to be a friend of Task and private Task::SetCallback, and public SetCallbackForTest that immediately calls that private SetCallback? https://codereview.chromium.org/2359933007/diff/80001/components/offline_page... File components/offline_pages/core/task_queue.h (right): https://codereview.chromium.org/2359933007/diff/80001/components/offline_page... components/offline_pages/core/task_queue.h:40: bool IsCurrentlyRunning() const; HasRunningTask? for consistency in naming...
Addressed https://codereview.chromium.org/2359933007/diff/80001/components/offline_page... File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/80001/components/offline_page... components/offline_pages/core/task.h:61: void SetTaskCompletionCallback( On 2016/09/30 22:13:08, Dmitry Titov wrote: > It took me to read the long comment and also verify implementation to understand > that this is not the Completion Callback I am looking for... > > If this is public for test, it'd be nice to call it > SetTaskCompletionCallbackForTest as we have presubmit that checks for these > called outside of tests. > > However it is not here only for test, contrary to what the comment says. It is > here also for TaskQueue. > > Could it be safer having TaskQueue to be a friend of Task and private > Task::SetCallback, and public SetCallbackForTest that immediately calls that > private SetCallback? Done. https://codereview.chromium.org/2359933007/diff/80001/components/offline_page... File components/offline_pages/core/task_queue.h (right): https://codereview.chromium.org/2359933007/diff/80001/components/offline_page... components/offline_pages/core/task_queue.h:40: bool IsCurrentlyRunning() const; On 2016/09/30 22:13:08, Dmitry Titov wrote: > HasRunningTask? for consistency in naming... Done.
The CQ bit was checked by fgorski@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org, petewil@chromium.org, dewittj@chromium.org, dimich@chromium.org, caitkp@chromium.org Link to the patchset: https://codereview.chromium.org/2359933007/#ps100001 (title: "Addressing feedback from Dmitry")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Introduces TaskQueue to serialize tasks that asynchronously access SQLStore * Adds a TaskQueue and Task interface * Adds tests for TaskQueue. * Introduces a core/ directory under components/offline_pages BUG=645522, 651238 R=dimich@chromium.org ========== to ========== [Offline pages] Introduces TaskQueue to serialize tasks that asynchronously access SQLStore * Adds a TaskQueue and Task interface * Adds tests for TaskQueue. * Introduces a core/ directory under components/offline_pages BUG=645522, 651238 R=dimich@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Introduces TaskQueue to serialize tasks that asynchronously access SQLStore * Adds a TaskQueue and Task interface * Adds tests for TaskQueue. * Introduces a core/ directory under components/offline_pages BUG=645522, 651238 R=dimich@chromium.org ========== to ========== [Offline pages] Introduces TaskQueue to serialize tasks that asynchronously access SQLStore * Adds a TaskQueue and Task interface * Adds tests for TaskQueue. * Introduces a core/ directory under components/offline_pages BUG=645522, 651238 R=dimich@chromium.org Committed: https://crrev.com/0674abb71a7737c42f368d6b68d81e86208012e3 Cr-Commit-Position: refs/heads/master@{#422269} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0674abb71a7737c42f368d6b68d81e86208012e3 Cr-Commit-Position: refs/heads/master@{#422269} |