Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(99)

Issue 2359933007: [Offline pages] Introduces TaskQueue to serialize tasks that asynchronously access SQLStore (Closed)

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+583 lines, -6 lines) Patch
M components/BUILD.gn View 1 1 chunk +1 line, -0 lines 0 comments Download
A + components/offline_pages/core/BUILD.gn View 1 2 2 chunks +23 lines, -6 lines 0 comments Download
A components/offline_pages/core/task.h View 1 2 3 4 5 1 chunk +82 lines, -0 lines 0 comments Download
A components/offline_pages/core/task.cc View 1 2 3 4 5 1 chunk +43 lines, -0 lines 0 comments Download
A components/offline_pages/core/task_queue.h View 1 2 3 4 5 1 chunk +64 lines, -0 lines 0 comments Download
A components/offline_pages/core/task_queue.cc View 1 2 3 4 5 1 chunk +49 lines, -0 lines 0 comments Download
A components/offline_pages/core/task_queue_unittest.cc View 1 2 3 4 5 1 chunk +123 lines, -0 lines 0 comments Download
A components/offline_pages/core/task_unittest.cc View 1 2 3 4 5 1 chunk +81 lines, -0 lines 0 comments Download
A components/offline_pages/core/test_task.h View 1 2 3 1 chunk +59 lines, -0 lines 0 comments Download
A components/offline_pages/core/test_task.cc View 1 2 3 1 chunk +58 lines, -0 lines 0 comments Download

Messages

Total messages: 40 (24 generated)
fgorski
Patch 1/2 to discuss tomorrow. PTAL
4 years, 2 months ago (2016-09-26 23:46:05 UTC) #2
dougarnett
On 2016/09/26 23:46:05, fgorski wrote: > Patch 1/2 to discuss tomorrow. PTAL Basically looks good
4 years, 2 months ago (2016-09-27 19:17:17 UTC) #3
Pete Williamson
https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/core/BUILD.gn File components/offline_pages/core/BUILD.gn (right): https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/core/BUILD.gn#newcode11 components/offline_pages/core/BUILD.gn:11: "task_queue.cc", What happened to offline_page_header? Did it get removed, ...
4 years, 2 months ago (2016-09-27 23:49:48 UTC) #4
fgorski
PTAL. Pete's comments addressed, and hopefully tests running properly now. https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/core/BUILD.gn File components/offline_pages/core/BUILD.gn (right): https://codereview.chromium.org/2359933007/diff/1/components/offline_pages/core/BUILD.gn#newcode11 ...
4 years, 2 months ago (2016-09-28 22:19:29 UTC) #11
Pete Williamson
https://codereview.chromium.org/2359933007/diff/40001/components/offline_pages/core/task.h File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_pages/core/task.h#newcode18 components/offline_pages/core/task.h:18: // To use TaskQueue: Maybe this would be better ...
4 years, 2 months ago (2016-09-29 00:54:44 UTC) #15
dougarnett
lgtm with some requests to clarify comments https://codereview.chromium.org/2359933007/diff/40001/components/offline_pages/core/task.h File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_pages/core/task.h#newcode28 components/offline_pages/core/task.h:28: typedef base::Callback<void(Task*)> ...
4 years, 2 months ago (2016-09-29 15:40:05 UTC) #16
fgorski
Addressed. PTAL https://codereview.chromium.org/2359933007/diff/40001/components/offline_pages/core/task.h File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/40001/components/offline_pages/core/task.h#newcode18 components/offline_pages/core/task.h:18: // To use TaskQueue: On 2016/09/29 00:54:43, ...
4 years, 2 months ago (2016-09-29 17:37:37 UTC) #18
Pete Williamson
lgtm
4 years, 2 months ago (2016-09-29 18:26:01 UTC) #20
dewittj
lgtm % comments https://codereview.chromium.org/2359933007/diff/60001/components/offline_pages/core/task.h File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/60001/components/offline_pages/core/task.h#newcode71 components/offline_pages/core/task.h:71: void TaskComplete(); Feels like TaskQueue should ...
4 years, 2 months ago (2016-09-29 18:40:56 UTC) #22
fgorski
Addressed. Cait, could you please stamp components/BUILD.gn? https://codereview.chromium.org/2359933007/diff/60001/components/offline_pages/core/task.h File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/60001/components/offline_pages/core/task.h#newcode71 components/offline_pages/core/task.h:71: void TaskComplete(); ...
4 years, 2 months ago (2016-09-30 17:33:12 UTC) #28
Cait (Slow)
lgtm
4 years, 2 months ago (2016-09-30 19:03:47 UTC) #31
Dmitry Titov
lgtm with couple of small suggestions: https://codereview.chromium.org/2359933007/diff/80001/components/offline_pages/core/task.h File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/80001/components/offline_pages/core/task.h#newcode61 components/offline_pages/core/task.h:61: void SetTaskCompletionCallback( It ...
4 years, 2 months ago (2016-09-30 22:13:08 UTC) #32
fgorski
Addressed https://codereview.chromium.org/2359933007/diff/80001/components/offline_pages/core/task.h File components/offline_pages/core/task.h (right): https://codereview.chromium.org/2359933007/diff/80001/components/offline_pages/core/task.h#newcode61 components/offline_pages/core/task.h:61: void SetTaskCompletionCallback( On 2016/09/30 22:13:08, Dmitry Titov wrote: ...
4 years, 2 months ago (2016-09-30 23:13:57 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2359933007/100001
4 years, 2 months ago (2016-09-30 23:14:33 UTC) #36
commit-bot: I haz the power
Committed patchset #6 (id:100001)
4 years, 2 months ago (2016-10-01 01:57:44 UTC) #38
commit-bot: I haz the power
4 years, 2 months ago (2016-10-01 02:01:27 UTC) #40
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/0674abb71a7737c42f368d6b68d81e86208012e3
Cr-Commit-Position: refs/heads/master@{#422269}

Powered by Google App Engine
This is Rietveld 408576698