[Offline pages] Extracting AddRequest method in RequestQueueStore

[Offline pages] Extracting AddRequest method in RequestQueueStore

* Adding AddRequest to RQStoreSQL and RQInMemoryStore
* Updating the Add test.

BUG=645522
Committed: https://crrev.com/72ae3a4a08e47cdbb0df4e62e7910b83133f5490
Cr-Commit-Position: refs/heads/master@{#420084}

[fgorski, 2016-09-20 23:22:09]

The CQ bit was checked by fgorski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 23:22:40]

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

[fgorski, 2016-09-20 23:25:58]

PTAL

This is pending a rebase/sync to the 2 previous patches, but I don't intend to
add functionality in this patch.

[fgorski, 2016-09-20 23:26:13]

fgorski@chromium.org changed reviewers:
+ petewil@chromium.org

[fgorski, 2016-09-20 23:26:14]

PTAL

[commit-bot: I haz the power, 2016-09-20 23:33:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 23:33:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Pete Williamson, 2016-09-21 00:00:08]

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
File components/offline_pages/background/request_queue_in_memory_store.h
(right):

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.h:26: void
AddRequest(const SavePageRequest& offline_page,
So are you renaming AddOrUpdateRequest to UpdateRequest in a future patch?

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
File components/offline_pages/background/request_queue_store.h (right):

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store.h:32: typedef
base::Callback<void(UpdateStatus)> UpdateCallback;
Does it make sense to use the same Status type for both add and Update since
they are similar?

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
File components/offline_pages/background/request_queue_store_sql.cc (right):

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:113:
ItemActionStatus Insert(sql::Connection* db, const SavePageRequest& request) {
Nit - should we place this just above InsertOrReplace to maintain the function
ordering scheme in place already in the file?

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:116: "
(request_id, creation_time, activation_time,"
I remember DewittJ suggesting in the OfflinePageModel that we use a
parameterized form of this statement to make it harder to get things out of
order - would that also make sense here?

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:135: // return
value for second if.
Is this TODO already TODONE?  If not, add a bit more explanation, please.

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:319: const
RequestQueueStore::AddCallback& callback) {
Should we also have a TODO for UMA here?

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:442: //
RemoveRequestsByRequestId to be more parallel with RemoveRequestsByClientId.
Should we add "TODO" to this comment?

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
File components/offline_pages/offline_page_types.h (right):

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/offline_page_types.h:61: enum class ItemActionStatus {
Would this make more sense in request_queue_store.h next to UpdateStatus? 
OfflinePageTypes seems like an unnecessary dependency from the point of view of
this type.

[fgorski, 2016-09-21 04:10:14]

The CQ bit was checked by fgorski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 04:10:35]

Dry run: 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-09-21 04:18:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 04:18:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[fgorski, 2016-09-21 04:31:13]

The CQ bit was checked by fgorski@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 04:31:41]

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

[fgorski, 2016-09-21 05:07:51]

Addressed and patch is fixed now.

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
File components/offline_pages/background/request_queue_in_memory_store.h
(right):

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_in_memory_store.h:26: void
AddRequest(const SavePageRequest& offline_page,
On 2016/09/21 00:00:07, Pete Williamson wrote:
> So are you renaming AddOrUpdateRequest to UpdateRequest in a future patch?

Correct.

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
File components/offline_pages/background/request_queue_store.h (right):

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store.h:32: typedef
base::Callback<void(UpdateStatus)> UpdateCallback;
On 2016/09/21 00:00:07, Pete Williamson wrote:
> Does it make sense to use the same Status type for both add and Update since
> they are similar?

Yes, and it will use the bulk object and deliver status in a ID -> status
mapping... in tomorrow's patch.

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
File components/offline_pages/background/request_queue_store_sql.cc (right):

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:113:
ItemActionStatus Insert(sql::Connection* db, const SavePageRequest& request) {
On 2016/09/21 00:00:08, Pete Williamson wrote:
> Nit - should we place this just above InsertOrReplace to maintain the function
> ordering scheme in place already in the file?

Done. To be honest I am not very fond of the current ordering. It feels rather
accidental.

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:116: "
(request_id, creation_time, activation_time,"
On 2016/09/21 00:00:08, Pete Williamson wrote:
> I remember DewittJ suggesting in the OfflinePageModel that we use a
> parameterized form of this statement to make it harder to get things out of
> order - would that also make sense here?

No, update will have a different form. Please compare to the update statement in
the OPM, or wait for the next patch.

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:135: // return
value for second if.
On 2016/09/21 00:00:08, Pete Williamson wrote:
> Is this TODO already TODONE?  If not, add a bit more explanation, please.

Done.

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:319: const
RequestQueueStore::AddCallback& callback) {
On 2016/09/21 00:00:08, Pete Williamson wrote:
> Should we also have a TODO for UMA here?

Done.

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/background/request_queue_store_sql.cc:442: //
RemoveRequestsByRequestId to be more parallel with RemoveRequestsByClientId.
On 2016/09/21 00:00:08, Pete Williamson wrote:
> Should we add "TODO" to this comment?

Done. It should be actually gone. Thanks for pointing out a more obvious problem
with that statement than the typo I was fixing.

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
File components/offline_pages/offline_page_types.h (right):

https://codereview.chromium.org/2356433004/diff/20001/components/offline_page...
components/offline_pages/offline_page_types.h:61: enum class ItemActionStatus {
On 2016/09/21 00:00:08, Pete Williamson wrote:
> Would this make more sense in request_queue_store.h next to UpdateStatus? 
> OfflinePageTypes seems like an unnecessary dependency from the point of view
of
> this type.

I am thinking of how to best structure that. I think a base file for stores
makes the most sense. For now I am putting it here. Let's discuss this tomorrow.

[commit-bot: I haz the power, 2016-09-21 05:17:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 05:17:30]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-09-21 16:43:46]

lgtm

[fgorski, 2016-09-21 16:55:58]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2016-09-21 16:56:26]

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-09-21 17:04:17]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-21 17:06:50]

Description was changed from

==========
[Offline pages] Extracting AddRequest method in RequestQueueStore

* Adding AddRequest to RQStoreSQL and RQInMemoryStore
* Updating the Add test.

BUG=645522
==========

to

==========
[Offline pages] Extracting AddRequest method in RequestQueueStore

* Adding AddRequest to RQStoreSQL and RQInMemoryStore
* Updating the Add test.

BUG=645522

Committed: https://crrev.com/72ae3a4a08e47cdbb0df4e62e7910b83133f5490
Cr-Commit-Position: refs/heads/master@{#420084}
==========

[commit-bot: I haz the power, 2016-09-21 17:06:51]

Patchset 4 (id:??) landed as
https://crrev.com/72ae3a4a08e47cdbb0df4e62e7910b83133f5490
Cr-Commit-Position: refs/heads/master@{#420084}