Make Service Worker DB UserData methods accept multiple keys at once

Make Service Worker DB UserData methods accept multiple keys at once

Primary benefit: it's now possible to read/write multiple keys at once
transactionally. This will be used in
https://codereview.chromium.org/1944863002 to fix the way
PushMessagingMessageFilter stores the sender ID and subscription ID.

Secondary benefit: this occasionally reduces the number of async hops,
for example in PushMessagingMessageFilter::UnsubscribeHavingGottenIds.

Since Chrome now supports initializer lists, it doesn't add much
code complexity when it isn't necessary (and hence it didn't seem
worth duplicating all these methods).

BUG=608831
Committed: https://crrev.com/fa6acac911373fd2e0f56a8a5f9652a7cc21b53e
Cr-Commit-Position: refs/heads/master@{#392045}

[johnme, 2016-05-03 18:14:49]

Description was changed from

==========
Make Service Worker DB UserData methods accept multiple keys at once

Primary benefit: it's now possible to read/write multiple keys at once
transactionally. This will be used by a follow-up patch to fix the way
PushMessagingMessageFilter stores the sender ID and subscription ID.

Secondary benefit: this occasionally reduces the number of async hops,
for example in PushMessagingMessageFilter::UnsubscribeHavingGottenIds.

BUG=589461
==========

to

==========
Make Service Worker DB UserData methods accept multiple keys at once

Primary benefit: it's now possible to read/write multiple keys at once
transactionally. This will be used by a follow-up patch to fix the way
PushMessagingMessageFilter stores the sender ID and subscription ID.

Secondary benefit: this occasionally reduces the number of async hops,
for example in PushMessagingMessageFilter::UnsubscribeHavingGottenIds.

BUG=608831
==========

[johnme, 2016-05-03 18:17:02]

Description was changed from

==========
Make Service Worker DB UserData methods accept multiple keys at once

Primary benefit: it's now possible to read/write multiple keys at once
transactionally. This will be used by a follow-up patch to fix the way
PushMessagingMessageFilter stores the sender ID and subscription ID.

Secondary benefit: this occasionally reduces the number of async hops,
for example in PushMessagingMessageFilter::UnsubscribeHavingGottenIds.

BUG=608831
==========

to

==========
Make Service Worker DB UserData methods accept multiple keys at once

Primary benefit: it's now possible to read/write multiple keys at once
transactionally. This will be used in
https://codereview.chromium.org/1944863002 to fix the way
PushMessagingMessageFilter stores the sender ID and subscription ID.

Secondary benefit: this occasionally reduces the number of async hops,
for example in PushMessagingMessageFilter::UnsubscribeHavingGottenIds.

Since Chrome now supports initializer lists, it doesn't add much
code complexity when it isn't necessary (and hence it didn't seem
worth duplicating all these methods).

BUG=608831
==========

[johnme, 2016-05-03 18:18:47]

johnme@chromium.org changed reviewers:
+ nhiroki@chromium.org

[johnme, 2016-05-03 18:20:35]

johnme@chromium.org changed reviewers:
- nhiroki@chromium.org

[johnme, 2016-05-03 18:20:55]

johnme@chromium.org changed reviewers:
+ michaeln@chromium.org

[michaeln, 2016-05-03 19:38:06]

lgtm

https://codereview.chromium.org/1945753002/diff/1/content/browser/push_messag...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1945753002/diff/1/content/browser/push_messag...
content/browser/push_messaging/push_messaging_message_filter.cc:343: if
(service_worker_status == SERVICE_WORKER_OK) {
maybe dcheck vector.size() == 1, ditto other return sites where a certain number
is expected

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_database.h (right):

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_database.h:176: // database.
also document that the function returns OK only if all keys are found

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_registration.h (left):

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_registration.h:37:
ServiceWorkerStatusCode status)> GetUserDataCallback;
thnx

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_storage.h:172: // is deleted when
the associated registraton is deleted.
Can you document what happens when some keys are found and others are not.

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_storage_unittest.cc (right):

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_storage_unittest.cc:973:
GetUserData(kRegistrationId, {"key2"}, &data_out));
also expect NOT_FOUND and empty data for GetUserData(..., {key, key2},...)

https://codereview.chromium.org/1945753002/diff/1/content/public/browser/push...
File content/public/browser/push_messaging_service.cc (right):

https://codereview.chromium.org/1945753002/diff/1/content/public/browser/push...
content/public/browser/push_messaging_service.cc:22:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
maybe DCHECK_LE(data.size(), 1)

[Michael van Ouwerkerk, 2016-05-04 09:08:53]

Just some drive-by nits... sorry :-)

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.h:157: const
std::vector<std::string>& keys,
nit: why not std::set<std::string>?

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.h:162: const
std::vector<std::pair<std::string, std::string>>& key_value_pairs,
nit: why not std::map<std::string, std::string>?

[johnme, 2016-05-05 10:54:03]

johnme@chromium.org changed reviewers:
+ jochen@chromium.org

[johnme, 2016-05-05 10:54:04]

Addressed review comments - thanks!

jochen@chromium.org: Please review minor changes in:
- content/public/browser/push_messaging_service.cc
- content/browser/background_sync/background_sync_manager.cc

https://codereview.chromium.org/1945753002/diff/1/content/browser/push_messag...
File content/browser/push_messaging/push_messaging_message_filter.cc (right):

https://codereview.chromium.org/1945753002/diff/1/content/browser/push_messag...
content/browser/push_messaging/push_messaging_message_filter.cc:343: if
(service_worker_status == SERVICE_WORKER_OK) {
On 2016/05/03 19:38:05, michaeln wrote:
> maybe dcheck vector.size() == 1, ditto other return sites where a certain
number
> is expected

Done.

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_context_wrapper.h (right):

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.h:157: const
std::vector<std::string>& keys,
On 2016/05/04 09:08:52, Michael van Ouwerkerk wrote:
> nit: why not std::set<std::string>?

Because the order of the results is based on the order of these keys (and having
the result order depend on the lexicographic order of the keys would be hella
confusing).

It could be a std::set for Clear, but consistency seemed nice.

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_context_wrapper.h:162: const
std::vector<std::pair<std::string, std::string>>& key_value_pairs,
On 2016/05/04 09:08:52, Michael van Ouwerkerk wrote:
> nit: why not std::map<std::string, std::string>?

I considered this, as both std::map and std::unordered_map would have the same
initializer_lists, but they perform slightly worse (especially since this gets
copied during the thread-hop, which would cause a 2nd n log n, or a re-hashing
of all the keys), and it seems slightly cleaner for the order of writes to be as
specified, rather than alphabetical by key or arbitrary (for example this makes
the tests deterministic). But I could go either way on this.

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_database.h (right):

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_database.h:176: // database.
On 2016/05/03 19:38:05, michaeln wrote:
> also document that the function returns OK only if all keys are found

Done.

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_registration.h (left):

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_registration.h:37:
ServiceWorkerStatusCode status)> GetUserDataCallback;
On 2016/05/03 19:38:06, michaeln wrote:
> thnx

Acknowledged.

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_storage.h:172: // is deleted when
the associated registraton is deleted.
On 2016/05/03 19:38:06, michaeln wrote:
> Can you document what happens when some keys are found and others are not.

Done.

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_storage_unittest.cc (right):

https://codereview.chromium.org/1945753002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_storage_unittest.cc:973:
GetUserData(kRegistrationId, {"key2"}, &data_out));
On 2016/05/03 19:38:06, michaeln wrote:
> also expect NOT_FOUND and empty data for GetUserData(..., {key, key2},...) 

I already check that below (see "Multiple gets fail if one is not found.").

https://codereview.chromium.org/1945753002/diff/1/content/public/browser/push...
File content/public/browser/push_messaging_service.cc (right):

https://codereview.chromium.org/1945753002/diff/1/content/public/browser/push...
content/public/browser/push_messaging_service.cc:22:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2016/05/03 19:38:06, michaeln wrote:
> maybe DCHECK_LE(data.size(), 1)
> 

I put a DCHECK_EQ(1u, data.size()); in the success case

[michaeln, 2016-05-05 19:45:00]

still lgtm, thnx

[jochen (gone - plz use gerrit), 2016-05-06 09:49:15]

lgtm

[johnme, 2016-05-06 11:59:58]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-05-06 11:59:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1945753002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-06 12:00:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1945753002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1945753002/40001

[commit-bot: I haz the power, 2016-05-06 13:20:58]

Description was changed from

==========
Make Service Worker DB UserData methods accept multiple keys at once

Primary benefit: it's now possible to read/write multiple keys at once
transactionally. This will be used in
https://codereview.chromium.org/1944863002 to fix the way
PushMessagingMessageFilter stores the sender ID and subscription ID.

Secondary benefit: this occasionally reduces the number of async hops,
for example in PushMessagingMessageFilter::UnsubscribeHavingGottenIds.

Since Chrome now supports initializer lists, it doesn't add much
code complexity when it isn't necessary (and hence it didn't seem
worth duplicating all these methods).

BUG=608831
==========

to

==========
Make Service Worker DB UserData methods accept multiple keys at once

Primary benefit: it's now possible to read/write multiple keys at once
transactionally. This will be used in
https://codereview.chromium.org/1944863002 to fix the way
PushMessagingMessageFilter stores the sender ID and subscription ID.

Secondary benefit: this occasionally reduces the number of async hops,
for example in PushMessagingMessageFilter::UnsubscribeHavingGottenIds.

Since Chrome now supports initializer lists, it doesn't add much
code complexity when it isn't necessary (and hence it didn't seem
worth duplicating all these methods).

BUG=608831
==========

[commit-bot: I haz the power, 2016-05-06 13:20:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-06 13:22:33]

Description was changed from

==========
Make Service Worker DB UserData methods accept multiple keys at once

Primary benefit: it's now possible to read/write multiple keys at once
transactionally. This will be used in
https://codereview.chromium.org/1944863002 to fix the way
PushMessagingMessageFilter stores the sender ID and subscription ID.

Secondary benefit: this occasionally reduces the number of async hops,
for example in PushMessagingMessageFilter::UnsubscribeHavingGottenIds.

Since Chrome now supports initializer lists, it doesn't add much
code complexity when it isn't necessary (and hence it didn't seem
worth duplicating all these methods).

BUG=608831
==========

to

==========
Make Service Worker DB UserData methods accept multiple keys at once

Primary benefit: it's now possible to read/write multiple keys at once
transactionally. This will be used in
https://codereview.chromium.org/1944863002 to fix the way
PushMessagingMessageFilter stores the sender ID and subscription ID.

Secondary benefit: this occasionally reduces the number of async hops,
for example in PushMessagingMessageFilter::UnsubscribeHavingGottenIds.

Since Chrome now supports initializer lists, it doesn't add much
code complexity when it isn't necessary (and hence it didn't seem
worth duplicating all these methods).

BUG=608831

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

[commit-bot: I haz the power, 2016-05-06 13:22:34]

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