PaymentHandler: Implement GetAllPaymentApps().

content/browser/service_worker/service_worker_database.cc

Index: content/browser/service_worker/service_worker_database.cc
diff --git a/content/browser/service_worker/service_worker_database.cc b/content/browser/service_worker/service_worker_database.cc
index 535b6a402700aefe223ee0b4d021fdfb54e32c03..f93413de05bdd3b4b67825790cc53475e5a1e195 100644
--- a/content/browser/service_worker/service_worker_database.cc
+++ b/content/browser/service_worker/service_worker_database.cc
@@ -1029,6 +1029,67 @@ ServiceWorkerDatabase::ReadUserDataForAllRegistrations(
   return status;
 }
 
+ServiceWorkerDatabase::Status
+ServiceWorkerDatabase::ReadUserDataForAllRegistrationsByKeyPrefix(

[nhiroki, 2017/05/09 23:49:15]

Can you add unittests for this API?

[zino, 2017/05/10 15:57:42]

Done.

+    const std::string& user_data_name_prefix,

[nhiroki, 2017/05/09 23:49:15]

Can you explain key/value format that we're about to read? Are there design
document or impl comments about it? I'd like to understand database access
pattern.

[zino, 2017/05/10 15:57:43]

Unfortunately, there is no latest version of design document.
[1]
https://docs.google.com/document/d/1rWsvKQAwIboN2ZDuYYAkfce8GF27twi4UHTt0hcby...
[2]
https://drive.google.com/file/d/0B6zWycIQV6zHa2R2U3JfVkllc0U/view?usp=sharing

Instead I'll explain them here.

You are already very familiar with cache storage API in service worker.
The following code will store a response cache into disk cache.
   cache.put(request, response);

The |request| is key and the |response| is value.
The following code will store two request/response pairs into disk cache.
  assert request1.url == 'https://example.com/?abcd';
  assert request2.url == 'https://example.com/?defg';
  cache.put(request1, response1);
  cache.put(request2, response2);

We can use cache.match/matchAll() to query stored cache data.
  // The following statement means that key is 'https://example.com/?abcd'.
  assert request1.url == 'https://example.com/?abcd';

  var responses = await cache.matchAll(request1);
  assert responses.length == 1;
  assert responses[0] == response1;

Also, we can use |ignoreSearch| option with them.
  // The following statement means that key is 'https://example.com/?abcd'.
  assert request1.url == 'https://example.com/?abcd';

  // But matchAll() with ignoreSearch: true will ignore the query string in URL.
  // So, the key(prefix) is 'https://example.com/';
  // Then, matchAll() will resolve all responses that match the key prefix.
  var responses = await cache.matchAll(request1, { ignoreSearch: true });
  assert responses.length == 2;
  assert responses[0] == response1;
  assert responses[1] == response2;

We need a way to query all stored data that match with key prefix. (similar to
case of ignoreSearch)
For example, the following code stores instruments into service worker database.
  registration.paymentManager.instruments.set('abcd', instrument1);
  registration.paymentManager.instruments.set('efgh', instrument2);

When merchant site requests payment to browser via PaymentRequest API, then
the UA will display the all proper stored payment instruments by
GetAllPaymentApps() method. (this implementation)

At this point, the UA doesn't know which key is used by JS author.
(The key can be 'abcd' or 'efgh' or other things..)
So, we need this function for UA to query all data by key prefix.

The new additional unit test can be helpful for your understanding.

[please use gerrit instead, 2017/05/10 18:14:35]

Please put this information in the design doc. Link to the design doc from the
header file.

[zino, 2017/05/10 18:36:32]

I don't disagree but I didn't find any past case in cs.chromium.org.
Is it usually recommended to put the design doc link to the header file?

If you don't mind, I'll add the link to this patch description. WDYT?

+    std::vector<std::pair<int64_t, std::string>>* user_data) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK(user_data->empty());
+
+  Status status = LazyOpen(false);
+  if (IsNewOrNonexistentDatabase(status))
+    return STATUS_OK;
+  if (status != STATUS_OK)
+    return status;
+
+  std::string key_prefix = kRegHasUserDataKeyPrefix + user_data_name_prefix;
+  {
+    std::unique_ptr<leveldb::Iterator> itr(
+        db_->NewIterator(leveldb::ReadOptions()));
+    for (itr->Seek(key_prefix); itr->Valid(); itr->Next()) {
+      status = LevelDBStatusToStatus(itr->status());
+      if (status != STATUS_OK) {
+        user_data->clear();
+        break;
+      }
+
+      if (!itr->key().starts_with(key_prefix))
+        break;
+
+      std::string user_data_name_with_id;
+      if (!RemovePrefix(itr->key().ToString(), kRegHasUserDataKeyPrefix,
+                        &user_data_name_with_id)) {
+        break;
+      }
+
+      std::vector<std::string> splited_data = base::SplitString(

[please use gerrit instead, 2017/05/09 18:01:47]

s/splited_data/parts/

[zino, 2017/05/10 15:57:42]

Done.

+          user_data_name_with_id, base::StringPrintf("%c", kKeySeparator),
+          base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
+      DCHECK(splited_data.size() == 2);

[please use gerrit instead, 2017/05/09 18:01:47]

Database can be corrupted on disk, so don't make assumptions via DCHECK(). Use
if statement to check whether the size is 2. If not, then skip over this piece
of data. Better yet, erase the corrupt data.

[nhiroki, 2017/05/09 23:49:15]

+1 to erase the corrupt data. Maybe it's like this?

  if (splited_data.size() != 2) {
    status = STATUS_ERROR_CORRUPTED;
    user_data->clear();
    break;
  }

HandleReadResult() disables the database and
ServiceWorkerStorage::DidGetUserDataForAllRegistrations() will start database
recreation.

[zino, 2017/05/10 15:57:43]

Done.

+
+      int64_t registration_id;
+      status = ParseId(splited_data[1], &registration_id);
+      if (status != STATUS_OK) {
+        user_data->clear();
+        break;
+      }
+
+      std::string value;
+      status = LevelDBStatusToStatus(db_->Get(
+          leveldb::ReadOptions(),
+          CreateUserDataKey(registration_id, splited_data[0]), &value));
+      if (status != STATUS_OK) {
+        user_data->clear();
+        break;
+      }
+      user_data->push_back(std::make_pair(registration_id, value));
+    }
+  }
+
+  HandleReadResult(FROM_HERE, status);
+  return status;
+}
+
 ServiceWorkerDatabase::Status ServiceWorkerDatabase::GetUncommittedResourceIds(
     std::set<int64_t>* ids) {
   return ReadResourceIds(kUncommittedResIdKeyPrefix, ids);