[Offline pages] Hooking up SQL store to Offline RequestQueue

[Offline pages] Hooking up SQL store to Offline RequestQueue

Adds a constant for directory that stores the SQL store for request queue
Updates RequestCoordinatorFactory to initialize RequestQueue with the
SQL based store

BUG=607221
Committed: https://crrev.com/c67805cbdf92a3f4c7f2c9363116a428816c27a0
Cr-Commit-Position: refs/heads/master@{#402855}

[fgorski, 2016-06-23 20:22:58]

fgorski@chromium.org changed reviewers:
+ dougarnett@chromium.org, jochen@chromium.org, petewil@chromium.org

[fgorski, 2016-06-23 20:22:59]

PTAL.

This adds a SQL store to request coordinator.

[Pete Williamson, 2016-06-23 22:28:49]

lgtm when tests are fixed.

[jochen (gone - plz use gerrit), 2016-06-24 13:34:41]

Can you add a test that checks that the database is correctly persisted?

[dougarnett, 2016-06-24 16:08:28]

lgtm with some naming suggestions

https://codereview.chromium.org/2093623003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/request_coordinator_factory.cc
(right):

https://codereview.chromium.org/2093623003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/request_coordinator_factory.cc:50:
scoped_refptr<base::SequencedTaskRunner> background_task_runner =
Is this only for the store or you expect can be used for other things?
If just for store, then I wonder about a name or comment to better identify
that.

https://codereview.chromium.org/2093623003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/request_coordinator_factory.cc:53:
base::FilePath store_path =
queue_store_path?

https://codereview.chromium.org/2093623003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/request_coordinator_factory.cc:57:
std::unique_ptr<RequestQueueStoreSQL> store(
queue_store?

[fgorski, 2016-06-24 18:18:46]

Addressing feedback from Doug

https://codereview.chromium.org/2093623003/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/request_coordinator_factory.cc
(right):

https://codereview.chromium.org/2093623003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/request_coordinator_factory.cc:50:
scoped_refptr<base::SequencedTaskRunner> background_task_runner =
On 2016/06/24 16:08:28, dougarnett wrote:
> Is this only for the store or you expect can be used for other things?
> If just for store, then I wonder about a name or comment to better identify
> that.

It is suitable for any background activity, therefor not exclusive to the queue.
Per our discussion I'll leave this as is.

https://codereview.chromium.org/2093623003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/request_coordinator_factory.cc:53:
base::FilePath store_path =
On 2016/06/24 16:08:28, dougarnett wrote:
> queue_store_path?

Done.

https://codereview.chromium.org/2093623003/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/request_coordinator_factory.cc:57:
std::unique_ptr<RequestQueueStoreSQL> store(
On 2016/06/24 16:08:28, dougarnett wrote:
> queue_store?

Done.

[fgorski, 2016-06-24 18:27:43]

On 2016/06/24 13:34:41, jochen wrote:
> Can you add a test that checks that the database is correctly persisted?

Hi, Jochen,

Not sure what test you suggest:
* Testing that DB reads/writes -> already done in store test (Related to the
question)
* Testing that RequestQueue talks to the store -> done in RequestQueue test
* Testing that RequestCoordinator uses Queue -> Done in RC test.
* Testing that RCFactory properly produces RC -> Done in factory tests

* Testing that DB persists stuff and has the same content after being shut down
and reopened?
  I realized we might be missing that test and will add that in a separate
patch.

Is there anything in addition to that you would like this patch to do?

Thanks.
Filip

[jochen (gone - plz use gerrit), 2016-06-27 08:27:31]

On 2016/06/24 at 18:27:43, fgorski wrote:
> On 2016/06/24 13:34:41, jochen wrote:
> > Can you add a test that checks that the database is correctly persisted?
> 
> Hi, Jochen,
> 
> Not sure what test you suggest:
> * Testing that DB reads/writes -> already done in store test (Related to the
question)
> * Testing that RequestQueue talks to the store -> done in RequestQueue test
> * Testing that RequestCoordinator uses Queue -> Done in RC test.
> * Testing that RCFactory properly produces RC -> Done in factory tests
> 
> * Testing that DB persists stuff and has the same content after being shut
down and reopened?
>   I realized we might be missing that test and will add that in a separate
patch.

that step is what I meant.

> 
> Is there anything in addition to that you would like this patch to do?
> 
> Thanks.
> Filip

Why not land the test together with the code? You can easily create such a test
as browser_tests where you first setup a profile in a test whose name starts
with PRE_ and the check that the profile was persisted correctly in a test
without that prefix, e.g. IN_PROCESS_BROWSER_TEST(OfflinePage,
PRE_PersistRequestQueue) and IN_PROCESS_BROWSER_TEST(OfflinePage,
PersisteRequestQueue)

[fgorski, 2016-06-28 21:13:08]

On 2016/06/27 08:27:31, jochen wrote:
> On 2016/06/24 at 18:27:43, fgorski wrote:
> > On 2016/06/24 13:34:41, jochen wrote:
> > > Can you add a test that checks that the database is correctly persisted?
> > 
> > Hi, Jochen,
> > 
> > Not sure what test you suggest:
> > * Testing that DB reads/writes -> already done in store test (Related to the
> question)
> > * Testing that RequestQueue talks to the store -> done in RequestQueue test
> > * Testing that RequestCoordinator uses Queue -> Done in RC test.
> > * Testing that RCFactory properly produces RC -> Done in factory tests
> > 
> > * Testing that DB persists stuff and has the same content after being shut
> down and reopened?
> >   I realized we might be missing that test and will add that in a separate
> patch.
> 
> that step is what I meant.
> 
> > 
> > Is there anything in addition to that you would like this patch to do?
> > 
> > Thanks.
> > Filip
> 
> Why not land the test together with the code? You can easily create such a
test
> as browser_tests where you first setup a profile in a test whose name starts
> with PRE_ and the check that the profile was persisted correctly in a test
> without that prefix, e.g. IN_PROCESS_BROWSER_TEST(OfflinePage,
> PRE_PersistRequestQueue) and IN_PROCESS_BROWSER_TEST(OfflinePage,
> PersisteRequestQueue)

PTAL.
I added the test with other RequestQueueStore tests. I think it is clean and
simple enough over there.

[Pete Williamson, 2016-06-28 21:45:28]

new test lgtm

[jochen (gone - plz use gerrit), 2016-06-29 15:18:43]

lgtm

[fgorski, 2016-06-29 16:29:24]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-06-29 16:29:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
dougarnett@chromium.org
Link to the patchset: https://codereview.chromium.org/2093623003/#ps80001
(title: "Adding a test for SQL store actually persisting requests across
resets")

[commit-bot: I haz the power, 2016-06-29 16:30:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 17:39:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-29 17:40:02]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 17:42:27]

Description was changed from

==========
[Offline pages] Hooking up SQL store to Offline RequestQueue

Adds a constant for directory that stores the SQL store for request queue
Updates RequestCoordinatorFactory to initialize RequestQueue with the
SQL based store

BUG=607221
==========

to

==========
[Offline pages] Hooking up SQL store to Offline RequestQueue

Adds a constant for directory that stores the SQL store for request queue
Updates RequestCoordinatorFactory to initialize RequestQueue with the
SQL based store

BUG=607221

Committed: https://crrev.com/c67805cbdf92a3f4c7f2c9363116a428816c27a0
Cr-Commit-Position: refs/heads/master@{#402855}
==========

[commit-bot: I haz the power, 2016-06-29 17:42:28]

Patchset 5 (id:??) landed as
https://crrev.com/c67805cbdf92a3f4c7f2c9363116a428816c27a0
Cr-Commit-Position: refs/heads/master@{#402855}