Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2801513002:
[Payments] Add web app manifest section table in SQLite web database (Closed)
|
Created:
3 years, 8 months ago by gogerald1 Modified:
3 years, 8 months ago Reviewers:
Peter Kasting, please use gerrit instead, sdefresne, Greg Billock, pwnall, Scott Hess - ex-Googler CC:
chromium-reviews, droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, jam, gogerald+paymentswatch_chromium.org, rouslan+payments_chromium.org, darin-cc_chromium.org, agrieve+watch_chromium.org, mahmadi+paymentswatch_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd web app manifest section table in SQLite web database
The table has three columns: {'id', 'min_version', 'fingerprints'}.
The table has no primary key.
This table is used to reduce payment app's loading time in payment request, refer below doc for details.
https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj-4-5fq_nI2azC7M/edit?usp=sharing
BUG=708508
Review-Url: https://codereview.chromium.org/2801513002
Cr-Commit-Position: refs/heads/master@{#464062}
Committed: https://chromium.googlesource.com/chromium/src/+/753494c7f71f9a82c63440f47140e475df7e8771
Patch Set 1 #
Total comments: 63
Patch Set 2 : address comments #
Total comments: 32
Patch Set 3 : fix nits #
Total comments: 16
Patch Set 4 : remove thread check #Patch Set 5 : move files #Patch Set 6 : rebase and rename for the new manifest format #
Total comments: 24
Patch Set 7 : address sql comments #
Total comments: 8
Patch Set 8 : simplify #
Total comments: 4
Patch Set 9 : remove std::move for temporary return value #
Total comments: 4
Patch Set 10 #Messages
Total messages: 135 (100 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Description was changed from ========== Add payment manifest section table BUG= ========== to ========== Add payment manifest section table BUG= ==========
Description was changed from ========== Add payment manifest section table BUG= ========== to ========== Add payment manifest section table in SQLite web database BUG= ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) 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_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:50008) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:90001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== Add payment manifest section table in SQLite web database BUG= ========== to ========== Add payment manifest section table in SQLite web database BUG=708508 ==========
Patchset #1 (id:110001) has been deleted
Patchset #1 (id:130001) has been deleted
Patchset #1 (id:150001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gogerald@chromium.org changed reviewers: + rouslan@chromium.org
Hi rouslan@, ptal, will use this table for verifying payment apps in the next CL,
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Can you expand the CL description? For example, it would be nice to see all the column names. Also, please link to the design doc.
Description was changed from ========== Add payment manifest section table in SQLite web database BUG=708508 ========== to ========== Add payment manifest section table in SQLite web database The table has four columns. Column one is a string stores the payment method name of this manifest section. Column two is a string stores the payment app's package name. Column three is a 64 bit integer stores the minimum version the payment app. Column fourth is a blob stores serialized finger prints. BUG=708508 ==========
Description was changed from
==========
Add payment manifest section table in SQLite web database
The table has four columns.
Column one is a string stores the payment method name of this manifest section.
Column two is a string stores the payment app's package name.
Column three is a 64 bit integer stores the minimum version the payment app.
Column fourth is a blob stores serialized finger prints.
BUG=708508
==========
to
==========
Add payment manifest section table in SQLite web database
The table has four columns: {'method_name', 'package_name', 'version',
'finger_prints'}
This table is for optimize payment app's loading time in payment request, refer
below doc for details.
https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj...
BUG=708508
==========
Description was changed from
==========
Add payment manifest section table in SQLite web database
The table has four columns: {'method_name', 'package_name', 'version',
'finger_prints'}
This table is for optimize payment app's loading time in payment request, refer
below doc for details.
https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj...
BUG=708508
==========
to
==========
Add payment manifest section table in SQLite web database
The table has four columns: {'method_name', 'package_name', 'version',
'finger_prints'}
This table is used for optimizing payment app's loading time in payment request,
refer below doc for details.
https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj...
BUG=708508
==========
Description was changed from
==========
Add payment manifest section table in SQLite web database
The table has four columns: {'method_name', 'package_name', 'version',
'finger_prints'}
This table is used for optimizing payment app's loading time in payment request,
refer below doc for details.
https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj...
BUG=708508
==========
to
==========
Add payment manifest section table in SQLite web database
The table has four columns: {'method_name', 'package_name', 'version',
'finger_prints'}
This table is used to reduce payment app's loading time in payment request,
refer below doc for details.
https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj...
BUG=708508
==========
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Add payment manifest section table in SQLite web database
The table has four columns: {'method_name', 'package_name', 'version',
'finger_prints'}
This table is used to reduce payment app's loading time in payment request,
refer below doc for details.
https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj...
BUG=708508
==========
to
==========
Add payment manifest section table in SQLite web database
The table has four columns: {'method_name', 'package_name', 'version',
'finger_prints'}.
The table has no primary key.
This table is used to reduce payment app's loading time in payment request,
refer below doc for details.
https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj...
BUG=708508
==========
On 2017/04/05 19:44:18, ಠ_ಠ wrote: > Can you expand the CL description? For example, it would be nice to see all the > column names. Also, please link to the design doc. Updated, ptal,
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/BUILD.gn (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/BUILD.gn:79: "//components/webdata/common:common", :common is redundant. Remove. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/BUILD.gn:82: "//sql:sql", :sql is redundant. Remove. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/payment_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:11: namespace { Newline after "namespace {". https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:13: const uint32_t kFingerPrintLength = 32; size_t https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:19: return reinterpret_cast<void*>(&table_key); Shouldn't you cast to WebDatabaseTable::TypeKey? https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:21: } Newline before "}" and add a comment " // namespace". https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:64: sql::Statement s1(db_->GetUniqueStatement( Use a transaction to group these statements together. Otherwise a failure in the middle of the function will leave the table in an inconsistent state. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:68: if (!s1.Run()) { No need for {} on a single-line body of an if-statement. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:87: s2.Run(); Check for error. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:104: if (!s.is_valid()) { No {}. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:133: new std::vector<uint8_t>()); base::MakeUnique. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:137: } This if statement is not useful, because the for loop below does not do anything when |finger_prints| is empty. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:139: for (auto finger_print : finger_prints) { const auto& https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:143: } Replace this for loop with: serialized_finger_prints->insert(serialized_finger_prints->end(), finger_print.begin(), finger_print.end()); https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:152: DCHECK(finger_prints.size() % kFingerPrintLength == 0); Don't trust anything that comes from disk. If this condition is not met, don't go into the loop below. You may want to have a boolean return value to indicate to the caller whether deserialization succeeded. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:160: } You can do this loop easier: for (size_t i = 0; i < finger_prints.size(); i += kFingerPrintLength) { deserialized_finger_prints.emplace_back(finger_prints.begin() + i, finger_prints.begin() + i + kFingerPrintLength); } https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:161: } Newline below. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/payment_manifest_section_table.h (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.h:15: // expects the following schema. On what thread does this object live? https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.h:45: std::vector<mojom::PaymentManifestSectionPtr>& manifest); both parameters should be const. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.h:50: std::string& method_name); const parameter. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.h:53: std::unique_ptr<std::vector<uint8_t>> SerializeFingerPrints( 1) Move these two functions into anonymous namespace in the .cc file. 2) Add extensive comments explaining what's happening in serialization and serialization. 3) Add these includes in the .cc file. #include <stdint.h> #include <memory> #include <vector> https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.h:59: }; Disallow copy and assign. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.h:60: } // namespace payments Add newline after }; https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/payment_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:11: namespace { Newline below. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:12: const uint32_t kFingerPrintLength = 32; size_t https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:21: ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); Can you put this in the constructor instead? https://codereview.chromium.org/2801513002/diff/170001/components/webdata_ser... File components/webdata_services/DEPS (right): https://codereview.chromium.org/2801513002/diff/170001/components/webdata_ser... components/webdata_services/DEPS:5: "+components/payments/content", :utils? https://codereview.chromium.org/2801513002/diff/170001/components/webdata_ser... File components/webdata_services/web_data_service_wrapper.cc (right): https://codereview.chromium.org/2801513002/diff/170001/components/webdata_ser... components/webdata_services/web_data_service_wrapper.cc:98: base::WrapUnique(new payments::PaymentManifestSectionTable)); base::MakeUnique.
Patchset #2 (id:190001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
Patchset #2 (id:210001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:230001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi rouslan@, please take another look, https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/BUILD.gn (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/BUILD.gn:79: "//components/webdata/common:common", On 2017/04/05 20:33:30, ಠ_ಠ wrote: > :common is redundant. Remove. common is not the only target in that gn, why it is redundant? https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/BUILD.gn:82: "//sql:sql", On 2017/04/05 20:33:30, ಠ_ಠ wrote: > :sql is redundant. Remove. ditto https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/payment_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:11: namespace { On 2017/04/05 20:33:30, ಠ_ಠ wrote: > Newline after "namespace {". Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:13: const uint32_t kFingerPrintLength = 32; On 2017/04/05 20:33:30, ಠ_ಠ wrote: > size_t Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:19: return reinterpret_cast<void*>(&table_key); On 2017/04/05 20:33:31, ಠ_ಠ wrote: > Shouldn't you cast to WebDatabaseTable::TypeKey? WebDatabaseTable::TypeKey is a void pointer in essential, might be easier to understand why we could do that to use avoid here. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:21: } On 2017/04/05 20:33:31, ಠ_ಠ wrote: > Newline before "}" and add a comment " // namespace". Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:64: sql::Statement s1(db_->GetUniqueStatement( On 2017/04/05 20:33:30, ಠ_ಠ wrote: > Use a transaction to group these statements together. Otherwise a failure in the > middle of the function will leave the table in an inconsistent state. Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:68: if (!s1.Run()) { On 2017/04/05 20:33:31, ಠ_ಠ wrote: > No need for {} on a single-line body of an if-statement. It's this true? long time ago (> 1 year) when I am doing C++ in Chrome :-), bracket is always required. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:87: s2.Run(); On 2017/04/05 20:33:30, ಠ_ಠ wrote: > Check for error. Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:104: if (!s.is_valid()) { On 2017/04/05 20:33:30, ಠ_ಠ wrote: > No {}. Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:133: new std::vector<uint8_t>()); On 2017/04/05 20:33:30, ಠ_ಠ wrote: > base::MakeUnique. Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:137: } On 2017/04/05 20:33:30, ಠ_ಠ wrote: > This if statement is not useful, because the for loop below does not do anything > when |finger_prints| is empty. Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:139: for (auto finger_print : finger_prints) { On 2017/04/05 20:33:30, ಠ_ಠ wrote: > const auto& Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:143: } On 2017/04/05 20:33:30, ಠ_ಠ wrote: > Replace this for loop with: > > serialized_finger_prints->insert(serialized_finger_prints->end(), > finger_print.begin(), finger_print.end()); Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:152: DCHECK(finger_prints.size() % kFingerPrintLength == 0); On 2017/04/05 20:33:30, ಠ_ಠ wrote: > Don't trust anything that comes from disk. If this condition is not met, don't > go into the loop below. You may want to have a boolean return value to indicate > to the caller whether deserialization succeeded. Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:160: } On 2017/04/05 20:33:31, ಠ_ಠ wrote: > You can do this loop easier: > > for (size_t i = 0; i < finger_prints.size(); i += kFingerPrintLength) { > deserialized_finger_prints.emplace_back(finger_prints.begin() + i, > finger_prints.begin() + i + kFingerPrintLength); > } Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:161: } On 2017/04/05 20:33:30, ಠ_ಠ wrote: > Newline below. Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/payment_manifest_section_table.h (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.h:15: // expects the following schema. On 2017/04/05 20:33:31, ಠ_ಠ wrote: > On what thread does this object live? Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.h:45: std::vector<mojom::PaymentManifestSectionPtr>& manifest); On 2017/04/05 20:33:31, ಠ_ಠ wrote: > both parameters should be const. Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.h:50: std::string& method_name); On 2017/04/05 20:33:31, ಠ_ಠ wrote: > const parameter. Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.h:53: std::unique_ptr<std::vector<uint8_t>> SerializeFingerPrints( On 2017/04/05 20:33:31, ಠ_ಠ wrote: > 1) Move these two functions into anonymous namespace in the .cc file. > 2) Add extensive comments explaining what's happening in serialization and > serialization. > 3) Add these includes in the .cc file. > > #include <stdint.h> > > #include <memory> > #include <vector> Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.h:59: }; On 2017/04/05 20:33:31, ಠ_ಠ wrote: > Disallow copy and assign. Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.h:60: } // namespace payments On 2017/04/05 20:33:31, ಠ_ಠ wrote: > Add newline after }; Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/payment_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:11: namespace { On 2017/04/05 20:33:31, ಠ_ಠ wrote: > Newline below. Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:12: const uint32_t kFingerPrintLength = 32; On 2017/04/05 20:33:31, ಠ_ಠ wrote: > size_t Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:21: ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); On 2017/04/05 20:33:31, ಠ_ಠ wrote: > Can you put this in the constructor instead? Why would we do that? we are not supposed to use the same file_ for multiple test cases, right? https://codereview.chromium.org/2801513002/diff/170001/components/webdata_ser... File components/webdata_services/DEPS (right): https://codereview.chromium.org/2801513002/diff/170001/components/webdata_ser... components/webdata_services/DEPS:5: "+components/payments/content", On 2017/04/05 20:33:31, ಠ_ಠ wrote: > :utils? No, it doesn't work, DEPS seems only support directory instead of compile target like .gn https://codereview.chromium.org/2801513002/diff/170001/components/webdata_ser... File components/webdata_services/web_data_service_wrapper.cc (right): https://codereview.chromium.org/2801513002/diff/170001/components/webdata_ser... components/webdata_services/web_data_service_wrapper.cc:98: base::WrapUnique(new payments::PaymentManifestSectionTable)); On 2017/04/05 20:33:31, ಠ_ಠ wrote: > base::MakeUnique. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
lgtm https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/BUILD.gn (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/BUILD.gn:79: "//components/webdata/common:common", On 2017/04/05 23:38:48, gogerald1 wrote: > On 2017/04/05 20:33:30, ಠ_ಠ wrote: > > :common is redundant. Remove. > > common is not the only target in that gn, why it is redundant? "//components/webdata/common:common" is equivalent to "//components/webdata/common". So ":common" is not adding any more information, it only slows down whoever is reading this file. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/payment_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:104: if (!s.is_valid()) { On 2017/04/05 23:38:48, gogerald1 wrote: > On 2017/04/05 20:33:30, ಠ_ಠ wrote: > > No {}. > > Done. Not yet. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/payment_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:21: ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); On 2017/04/05 23:38:49, gogerald1 wrote: > On 2017/04/05 20:33:31, ಠ_ಠ wrote: > > Can you put this in the constructor instead? > > Why would we do that? we are not supposed to use the same file_ for multiple > test cases, right? Both constructor and SetUp() are called for every test case, so file_ is never reused. See reference: https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... https://codereview.chromium.org/2801513002/diff/170001/components/webdata_ser... File components/webdata_services/DEPS (right): https://codereview.chromium.org/2801513002/diff/170001/components/webdata_ser... components/webdata_services/DEPS:5: "+components/payments/content", On 2017/04/05 23:38:49, gogerald1 wrote: > On 2017/04/05 20:33:31, ಠ_ಠ wrote: > > :utils? > > No, it doesn't work, DEPS seems only support directory instead of compile target > like .gn Acknowledged. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... File components/payments/content/payment_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:32: for (const auto finger_print : finger_prints) { Let's put a "&" after "auto" to make sure that no copies happen. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:33: DCHECK(finger_print.size() == kFingerPrintLength); DCHECK_EQ(kFingerPrintLength, finger_print.size()); (This will print a prettier error message.) https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:46: std::vector<std::vector<uint8_t>>& deserialized_finger_prints) { Output is usually a pointer instead of ref. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:50: for (size_t i = 0; i < finger_prints.size();) { You can move i+=kFingerprintlength into this line. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:169: continue; Let's not read out only partial information from the table. If any part of the data is not valid, let's ignore all of it. Like so: if (!s.ColumnBlobAsVector(index, &finger_prints)) { manifest.clear(); return manifest; } https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:175: continue; manifest.clear(); return manifest; https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... File components/payments/content/payment_manifest_section_table.h (right): https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.h:19: // The interfaces should only be accessed on DB thread. Please put a DCHECK for the DB thread in AddPaymentManifestSections() and GetPaymentManifestSections(). https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... File components/payments/content/payment_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:60: for (size_t i = 0; i < 2; i++) { Use "2U" instead of "2" to avoid compiler complaints about comparing signed "2" and unsigned "i". https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:103: for (size_t i = 0; i < 2; i++) { 2U https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:118: for (size_t i = 0; i < 2; i++) { 2U https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:143: for (size_t i = 0; i < 2; i++) { 2U https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:156: for (size_t i = 0; i < 2; i++) { 2U https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:177: for (size_t i = 0; i < 2; i++) { 2U https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:197: for (size_t i = 0; i < 2; i++) { 2U https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:218: for (size_t i = 0; i < 2; i++) { 2U https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:235: for (size_t i = 0; i < 2; i++) { 2U
gogerald@chromium.org changed reviewers: + pkasting@chromium.org
Hi pkasting@, please review changes in //components/webdata_services/* https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/BUILD.gn (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/BUILD.gn:79: "//components/webdata/common:common", On 2017/04/06 14:35:28, ಠ_ಠ wrote: > On 2017/04/05 23:38:48, gogerald1 wrote: > > On 2017/04/05 20:33:30, ಠ_ಠ wrote: > > > :common is redundant. Remove. > > > > common is not the only target in that gn, why it is redundant? > > "//components/webdata/common:common" is equivalent to > "//components/webdata/common". So ":common" is not adding any more information, > it only slows down whoever is reading this file. common is the default target of //components/webdata/common, then make it explicit would be better? Anyway, addressed it as you commented, saw various preference in gn, but mostly use implicit target name https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/payment_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:104: if (!s.is_valid()) { On 2017/04/06 14:35:28, ಠ_ಠ wrote: > On 2017/04/05 23:38:48, gogerald1 wrote: > > On 2017/04/05 20:33:30, ಠ_ಠ wrote: > > > No {}. > > > > Done. > > Not yet. Done. https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... File components/payments/content/payment_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/170001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:21: ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); On 2017/04/06 14:35:28, ಠ_ಠ wrote: > On 2017/04/05 23:38:49, gogerald1 wrote: > > On 2017/04/05 20:33:31, ಠ_ಠ wrote: > > > Can you put this in the constructor instead? > > > > Why would we do that? we are not supposed to use the same file_ for multiple > > test cases, right? > > Both constructor and SetUp() are called for every test case, so file_ is never > reused. See reference: > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... Ah, Great to know, but it still looks slightly better to keep it in SetUp since CreateUniqueTempDir and Init has returning value. It might be better to ASSERT the returning values before continue (assert throws message, can not return value in constructor). And We do not expect subclass this test class. WDYT? https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... File components/payments/content/payment_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:32: for (const auto finger_print : finger_prints) { On 2017/04/06 14:35:29, ಠ_ಠ wrote: > Let's put a "&" after "auto" to make sure that no copies happen. Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:33: DCHECK(finger_print.size() == kFingerPrintLength); On 2017/04/06 14:35:28, ಠ_ಠ wrote: > DCHECK_EQ(kFingerPrintLength, finger_print.size()); > > (This will print a prettier error message.) Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:46: std::vector<std::vector<uint8_t>>& deserialized_finger_prints) { On 2017/04/06 14:35:29, ಠ_ಠ wrote: > Output is usually a pointer instead of ref. Agreed, but ref looks slightly better here since deserialized_finger_prints should not be null. And I named it deserialized_* for output. Make sense? https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:50: for (size_t i = 0; i < finger_prints.size();) { On 2017/04/06 14:35:29, ಠ_ಠ wrote: > You can move i+=kFingerprintlength into this line. Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:169: continue; On 2017/04/06 14:35:29, ಠ_ಠ wrote: > Let's not read out only partial information from the table. If any part of the > data is not valid, let's ignore all of it. Like so: > > if (!s.ColumnBlobAsVector(index, &finger_prints)) { > manifest.clear(); > return manifest; > } Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:175: continue; On 2017/04/06 14:35:28, ಠ_ಠ wrote: > manifest.clear(); > return manifest; Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... File components/payments/content/payment_manifest_section_table.h (right): https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table.h:19: // The interfaces should only be accessed on DB thread. On 2017/04/06 14:35:29, ಠ_ಠ wrote: > Please put a DCHECK for the DB thread in AddPaymentManifestSections() and > GetPaymentManifestSections(). Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... File components/payments/content/payment_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:60: for (size_t i = 0; i < 2; i++) { On 2017/04/06 14:35:29, ಠ_ಠ wrote: > Use "2U" instead of "2" to avoid compiler complaints about comparing signed "2" > and unsigned "i". Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:103: for (size_t i = 0; i < 2; i++) { On 2017/04/06 14:35:29, ಠ_ಠ wrote: > 2U Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:118: for (size_t i = 0; i < 2; i++) { On 2017/04/06 14:35:29, ಠ_ಠ wrote: > 2U Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:143: for (size_t i = 0; i < 2; i++) { On 2017/04/06 14:35:29, ಠ_ಠ wrote: > 2U Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:156: for (size_t i = 0; i < 2; i++) { On 2017/04/06 14:35:29, ಠ_ಠ wrote: > 2U Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:177: for (size_t i = 0; i < 2; i++) { On 2017/04/06 14:35:29, ಠ_ಠ wrote: > 2U Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:197: for (size_t i = 0; i < 2; i++) { On 2017/04/06 14:35:29, ಠ_ಠ wrote: > 2U Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:218: for (size_t i = 0; i < 2; i++) { On 2017/04/06 14:35:29, ಠ_ಠ wrote: > 2U Done. https://codereview.chromium.org/2801513002/diff/250001/components/payments/co... components/payments/content/payment_manifest_section_table_unittest.cc:235: for (size_t i = 0; i < 2; i++) { On 2017/04/06 14:35:29, ಠ_ಠ wrote: > 2U Done.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... File components/payments/content/payment_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:15: using content::BrowserThread; This pollutes the global namespace of header files that you've included. Move the "using" inside of the anonymous namespace and prepend the name namespace with "::" to avoid possible collision with payments::content::BrowserThread. https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:19: using ::content::BrowserThread; https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:105: DCHECK_CURRENTLY_ON(BrowserThread::DB); DCHECK is pre-processed out of the code in release mode, so your code in release mode will be: if (BrowserThread::IsMessageLoopValid(BrowserThread::DB)) if (!db_->BeginTransaction()) return false; That would be incorrect. You've most likely added that IsMessageLoopValid check because of failures in your unit tests. The correct fix, however, is to put a dummy message loop object in your test fixture. https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:152: if (BrowserThread::IsMessageLoopValid(BrowserThread::DB)) Ditto. https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:175: NOTREACHED(); Remove NOTREACHED(). This is easily reachable through file corruption. NOTREACHED() should only be used for bugs in your code. Corrupted files are outside of your control. So your code can be perfect, but still hit this code path. https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:182: NOTREACHED(); Ditto.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... File components/payments/content/payment_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:105: DCHECK_CURRENTLY_ON(BrowserThread::DB); On 2017/04/06 16:47:19, ಠ_ಠ wrote: > DCHECK is pre-processed out of the code in release mode, so your code in release > mode will be: > > if (BrowserThread::IsMessageLoopValid(BrowserThread::DB)) > if (!db_->BeginTransaction()) > return false; > > That would be incorrect. > > You've most likely added that IsMessageLoopValid check because of failures in > your unit tests. The correct fix, however, is to put a dummy message loop object > in your test fixture. Yes, the reason of using IsMessageLoopValid is for unit tests. Thought about introducing message loop in unit tests. But doubted whether it is worth since these interfaces are synchronous and we must implement WebDataServiceBase to handle asynchronous call from UI thread to these interfaces later. So it is very less likely to access these interfaces from incorrect thread if the WebDataServiceBase is implemented correctly, where we can introduce specific test to make sure. Thus, I prefer to move these checks to the implementation of WebDataServiceBase. Make sense? FYI, Other tables, like AutofillTable, KeywordTable, LoginsTable and TokenServiceTable, don't do this check as well, might because of the similar reason.
https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... File components/payments/content/payment_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:105: DCHECK_CURRENTLY_ON(BrowserThread::DB); On 2017/04/06 18:31:55, gogerald1 wrote: > On 2017/04/06 16:47:19, ಠ_ಠ wrote: > > DCHECK is pre-processed out of the code in release mode, so your code in > release > > mode will be: > > > > if (BrowserThread::IsMessageLoopValid(BrowserThread::DB)) > > if (!db_->BeginTransaction()) > > return false; > > > > That would be incorrect. > > > > You've most likely added that IsMessageLoopValid check because of failures in > > your unit tests. The correct fix, however, is to put a dummy message loop > object > > in your test fixture. > > Yes, the reason of using IsMessageLoopValid is for unit tests. Thought about > introducing message loop in unit tests. But doubted whether it is worth since > these interfaces are synchronous and we must implement WebDataServiceBase to > handle asynchronous call from UI thread to these interfaces later. So it is very > less likely to access these interfaces from incorrect thread if the > WebDataServiceBase is implemented correctly, where we can introduce specific > test to make sure. Thus, I prefer to move these checks to the implementation of > WebDataServiceBase. Make sense? > > FYI, Other tables, like AutofillTable, KeywordTable, LoginsTable and > TokenServiceTable, don't do this check as well, might because of the similar > reason. > ok.
LGTM https://codereview.chromium.org/2801513002/diff/270001/components/webdata_ser... File components/webdata_services/web_data_service_wrapper.cc (right): https://codereview.chromium.org/2801513002/diff/270001/components/webdata_ser... components/webdata_services/web_data_service_wrapper.cc:98: base::MakeUnique<payments::PaymentManifestSectionTable>()); Thanks for using MakeUnique! Optional: While here, also change the lines above to use it.
Patchset #5 (id:310001) has been deleted
Patchset #5 (id:330001) has been deleted
Patchset #5 (id:350001) has been deleted
Patchset #5 (id:370001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gogerald@chromium.org changed reviewers: + blundell@chromium.org
Moved the files from //components/payments/content/ to //components/payments/android since its android only for now. PTAL. Hi blundell@, ptal of the changes in //components/BUILD.gn https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... File components/payments/content/payment_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:15: using content::BrowserThread; On 2017/04/06 16:47:19, ಠ_ಠ wrote: > This pollutes the global namespace of header files that you've included. Move > the "using" inside of the anonymous namespace and prepend the name namespace > with "::" to avoid possible collision with payments::content::BrowserThread. Acknowledged. Not applicable anymore. https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:19: On 2017/04/06 16:47:19, ಠ_ಠ wrote: > using ::content::BrowserThread; Acknowledged. Not applicable anymore. https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:105: DCHECK_CURRENTLY_ON(BrowserThread::DB); On 2017/04/06 16:47:19, ಠ_ಠ wrote: > DCHECK is pre-processed out of the code in release mode, so your code in release > mode will be: > > if (BrowserThread::IsMessageLoopValid(BrowserThread::DB)) > if (!db_->BeginTransaction()) > return false; > > That would be incorrect. > > You've most likely added that IsMessageLoopValid check because of failures in > your unit tests. The correct fix, however, is to put a dummy message loop object > in your test fixture. Acknowledged. Not applicable anymore. https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:152: if (BrowserThread::IsMessageLoopValid(BrowserThread::DB)) On 2017/04/06 16:47:19, ಠ_ಠ wrote: > Ditto. Acknowledged. Not applicable anymore. https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:175: NOTREACHED(); On 2017/04/06 16:47:19, ಠ_ಠ wrote: > Remove NOTREACHED(). This is easily reachable through file corruption. > NOTREACHED() should only be used for bugs in your code. Corrupted files are > outside of your control. So your code can be perfect, but still hit this code > path. Done. https://codereview.chromium.org/2801513002/diff/270001/components/payments/co... components/payments/content/payment_manifest_section_table.cc:182: NOTREACHED(); On 2017/04/06 16:47:19, ಠ_ಠ wrote: > Ditto. Done. https://codereview.chromium.org/2801513002/diff/270001/components/webdata_ser... File components/webdata_services/web_data_service_wrapper.cc (right): https://codereview.chromium.org/2801513002/diff/270001/components/webdata_ser... components/webdata_services/web_data_service_wrapper.cc:98: base::MakeUnique<payments::PaymentManifestSectionTable>()); On 2017/04/06 19:04:36, Peter Kasting wrote: > Thanks for using MakeUnique! Optional: While here, also change the lines above > to use it. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gogerald@chromium.org changed reviewers: + sdefresne@chromium.org - blundell@chromium.org
Change reviewer since blundell@ is OOO. Hi sdefresne@, ptal of the changes in //components/BUILD.gn.
components/BUILD.gn lgtm
Thanks all, submitting
You've stated that you don't want to include anything content-related in components/webdata_services, so you include only components/payments/android. However, you include components/payments/content/ from components/payments/android. This is a layering violation. To fix it, please move all *.mojom files from components/payments/content/ to components/payments/mojom/. Then you'll be good to go with a submit.
On 2017/04/10 16:08:37, ಠ_ಠ wrote: > You've stated that you don't want to include anything content-related in > components/webdata_services, so you include only components/payments/android. > However, you include components/payments/content/ from > components/payments/android. This is a layering violation. To fix it, please > move all *.mojom files from components/payments/content/ to > components/payments/mojom/. Then you'll be good to go with a submit. As talked offline, will do this in the upcoming next CL,
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2801513002/#ps390001 (title: "move files")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
gogerald@chromium.org changed reviewers: + gbillock@chromium.org
Hi gbillock@, presubmit said I need LGTM from the owner of //sql for dependency in this CL. The files use sql are components/payments/android/* directory, ptal.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi rouslan@, another look? rebased and renamed for the new manifest format,
Change //sql reviewer since gbillock@ looks not working on Chrome anymore. Hi shess@, presubmit said I need LGTM from the owner of //sql for dependency in this CL. The files use sql are in components/payments/android/* directory. PTAL.
gogerald@chromium.org changed reviewers: + shess@chromium.org
Change //sql reviewer since gbillock@ looks not working on Chrome anymore. Hi shess@, presubmit said I need LGTM from the owner of //sql for dependency in this CL. The files use sql are in components/payments/android/* directory. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Oops, I reviewed, then looked at previous reviews. Since everyone apparently looked at the fingerprint-generator loop, you can take that suggestion as advisory, if you don't want to go in that direction. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... File components/payments/android/web_app_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:75: if (!db_->Execute("CREATE TABLE web_app_manifest_section ( " How about "CREATE TABLE IF NOT EXISTS web_app_manifest_section..."? https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:99: if (manifest == nullptr || manifest->id.empty()) Is this something which should never happen because it's not appropriate use of the API? In that case a DCHECK seems better. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:102: if (!db_->BeginTransaction()) Manual transaction handling is brittle, use sql::Transaction unless you have a solid reason to do otherwise. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:148: return manifest; |manifest| is not apparently initialized at this point. If you mean nullptr, use nullptr. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:160: return manifest; Just return nullptr. I assume |manifest| is scoped to delete itself. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:165: return manifest; Also here. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... File components/payments/android/web_app_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:36: std::unique_ptr<WebDatabase> db_; Put this above |table_|, since |table_| might refer to |db_| (sorry if I misunderstand this code). https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:55: manifest->min_version = static_cast<int64_t>(1); I think 1l or 1L will also be the right 1. Though I'm surprised simple 1 doesn't work. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:63: } I see what you're doing, here, but it might also be worthwhile to just fill in the fingerprints from a static buffer (or random data), then verify returned value from the original buffer. If you want to keep this form, then it might make sense to have helpers for "generate from seed" and "compare from seed", so that the duplicate bits of code don't get out of sync with each other. Also, the generate/compare code is complexity irrelevant to the test, this would read cleaner as: manifest->id = ...; manifest->min_version = ...; manifest->fingerprints.push_back(GenerateFingerprint(...)); manifest->fingerprints.push_back(GenerateFingerprint(...));
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:450001) has been deleted
Description was changed from
==========
Add payment manifest section table in SQLite web database
The table has four columns: {'method_name', 'package_name', 'version',
'finger_prints'}.
The table has no primary key.
This table is used to reduce payment app's loading time in payment request,
refer below doc for details.
https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj...
BUG=708508
==========
to
==========
Add web app manifest section table in SQLite web database
The table has three columns: {'id', 'min_version', 'fingerprints'}.
The table has no primary key.
This table is used to reduce payment app's loading time in payment request,
refer below doc for details.
https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj...
BUG=708508
==========
Patchset #8 (id:470001) has been deleted
Thanks shess@, another look? https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... File components/payments/android/web_app_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:75: if (!db_->Execute("CREATE TABLE web_app_manifest_section ( " On 2017/04/11 17:24:06, Scott Hess wrote: > How about "CREATE TABLE IF NOT EXISTS web_app_manifest_section..."? Done. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:99: if (manifest == nullptr || manifest->id.empty()) On 2017/04/11 17:24:06, Scott Hess wrote: > Is this something which should never happen because it's not appropriate use of > the API? In that case a DCHECK seems better. Done. manifest should never be nullptr, DCHECKed it. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:102: if (!db_->BeginTransaction()) On 2017/04/11 17:24:06, Scott Hess wrote: > Manual transaction handling is brittle, use sql::Transaction unless you have a > solid reason to do otherwise. Done. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:148: return manifest; On 2017/04/11 17:24:06, Scott Hess wrote: > |manifest| is not apparently initialized at this point. If you mean nullptr, > use nullptr. Done. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:160: return manifest; On 2017/04/11 17:24:06, Scott Hess wrote: > Just return nullptr. I assume |manifest| is scoped to delete itself. Done. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:165: return manifest; On 2017/04/11 17:24:06, Scott Hess wrote: > Also here. Done. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... File components/payments/android/web_app_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:36: std::unique_ptr<WebDatabase> db_; On 2017/04/11 17:24:06, Scott Hess wrote: > Put this above |table_|, since |table_| might refer to |db_| (sorry if I > misunderstand this code). we first create |table_| and then add it to |db_|, so it might make more sense to keep |db_| below |table_| https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:55: manifest->min_version = static_cast<int64_t>(1); On 2017/04/11 17:24:06, Scott Hess wrote: > I think 1l or 1L will also be the right 1. Though I'm surprised simple 1 > doesn't work. Use "int64_t" would be more specific and understandable? https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:63: } On 2017/04/11 17:24:06, Scott Hess wrote: > I see what you're doing, here, but it might also be worthwhile to just fill in > the fingerprints from a static buffer (or random data), then verify returned > value from the original buffer. If you want to keep this form, then it might > make sense to have helpers for "generate from seed" and "compare from seed", so > that the duplicate bits of code don't get out of sync with each other. Also, > the generate/compare code is complexity irrelevant to the test, this would read > cleaner as: > manifest->id = ...; > manifest->min_version = ...; > manifest->fingerprints.push_back(GenerateFingerprint(...)); > manifest->fingerprints.push_back(GenerateFingerprint(...)); Done.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you! Found a couple more minor items. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... File components/payments/android/web_app_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:99: if (manifest == nullptr || manifest->id.empty()) On 2017/04/11 19:01:04, gogerald1 wrote: > On 2017/04/11 17:24:06, Scott Hess wrote: > > Is this something which should never happen because it's not appropriate use > of > > the API? In that case a DCHECK seems better. > > Done. manifest should never be nullptr, DCHECKed it. I'm wondering about the empty() check, now. All of the later false returns are from cases where something went wrong with the database, but this one is where the caller called with in incorrect value. AddWebAppManifest() sounds like something you shouldn't even call if you don't have an id, and the caller maybe made a mistake. It is good to keep the invalid id from getting to the database, though, because it will be persisted even if the caller makes a mistake. So IMHO NOTREACHED() is reasonable. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... File components/payments/android/web_app_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:36: std::unique_ptr<WebDatabase> db_; On 2017/04/11 19:01:04, gogerald1 wrote: > On 2017/04/11 17:24:06, Scott Hess wrote: > > Put this above |table_|, since |table_| might refer to |db_| (sorry if I > > misunderstand this code). > > we first create |table_| and then add it to |db_|, so it might make more sense > to keep |db_| below |table_| The bit I'm worried about is that table_.db_ maintains a direct pointer to db_, so could lead to use-after-free errors. Though there is a comment on WebDatabaseTable::Db_ which indicates that this case is expected, so I guess if this code considers that kind of thing acceptable, I'll defer to previous owner reviews on the point. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:55: manifest->min_version = static_cast<int64_t>(1); On 2017/04/11 19:01:04, gogerald1 wrote: > On 2017/04/11 17:24:06, Scott Hess wrote: > > I think 1l or 1L will also be the right 1. Though I'm surprised simple 1 > > doesn't work. > > Use "int64_t" would be more specific and understandable? How would it be so? Is it important to know in this context that min_version is an int64_t? We certainly don't annotate our other throw-away constants that way, you weren't explicit with id (which is not a const char*). https://codereview.chromium.org/2801513002/diff/490001/components/payments/an... File components/payments/android/web_app_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/490001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:138: "LIMIT 1")); Why are you doing the LIMIT 1, here? Oooooh. Shouldn't you have a unique index on [id]? Then you could also use INSERT OR REPLACE in AddWebAppManifest(), which would allow you to drop the DELETE and remove the explicit sql::Transaction (because the INSERT OR REPLACE is implicitly atomic). https://codereview.chromium.org/2801513002/diff/490001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:142: return nullptr; This is implicit in s.Step(). It will return |false| if the statement is invalid. There are a few edge cases involving testing of error-handling which sometimes need an explicit is_valid() test, but if you don't need it, best to drop it. Also, Chromium style below would be to if(!s.Step()) return nullptr, then un-nest the current if() block. https://codereview.chromium.org/2801513002/diff/490001/components/payments/an... File components/payments/android/web_app_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/490001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:102: manifest_1->fingerprints.push_back(kFingerprintTwo); Thanks, I like this better. The one problem is that I'm not entirely sure whether the global vectors will work out, though perhaps static initializers are fine in tests? Or if constexpr can be added? https://codereview.chromium.org/2801513002/diff/490001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:126: } ASSERT_TRUE(bobpay_manifest->fingerprints[0] == kFingerprintOne); should work fine. Though if static objects are banned in tests, that changes things.
Patchset #9 (id:510001) has been deleted
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks shess@, one more look? https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... File components/payments/android/web_app_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:99: if (manifest == nullptr || manifest->id.empty()) On 2017/04/11 19:31:45, Scott Hess wrote: > On 2017/04/11 19:01:04, gogerald1 wrote: > > On 2017/04/11 17:24:06, Scott Hess wrote: > > > Is this something which should never happen because it's not appropriate use > > of > > > the API? In that case a DCHECK seems better. > > > > Done. manifest should never be nullptr, DCHECKed it. > > I'm wondering about the empty() check, now. All of the later false returns are > from cases where something went wrong with the database, but this one is where > the caller called with in incorrect value. AddWebAppManifest() sounds like > something you shouldn't even call if you don't have an id, and the caller maybe > made a mistake. > > It is good to keep the invalid id from getting to the database, though, because > it will be persisted even if the caller makes a mistake. So IMHO NOTREACHED() > is reasonable. Done. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... File components/payments/android/web_app_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:36: std::unique_ptr<WebDatabase> db_; On 2017/04/11 19:31:45, Scott Hess wrote: > On 2017/04/11 19:01:04, gogerald1 wrote: > > On 2017/04/11 17:24:06, Scott Hess wrote: > > > Put this above |table_|, since |table_| might refer to |db_| (sorry if I > > > misunderstand this code). > > > > we first create |table_| and then add it to |db_|, so it might make more sense > > to keep |db_| below |table_| > > The bit I'm worried about is that table_.db_ maintains a direct pointer to db_, > so could lead to use-after-free errors. > > Though there is a comment on WebDatabaseTable::Db_ which indicates that this > case is expected, so I guess if this code considers that kind of thing > acceptable, I'll defer to previous owner reviews on the point. Acknowledged. should be fine here since db_->Init(file_) will initialize table_.db_. WebDataServiceWrapper will make sure db_ is initialized for the table in production code. https://codereview.chromium.org/2801513002/diff/410001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:55: manifest->min_version = static_cast<int64_t>(1); On 2017/04/11 19:31:45, Scott Hess wrote: > On 2017/04/11 19:01:04, gogerald1 wrote: > > On 2017/04/11 17:24:06, Scott Hess wrote: > > > I think 1l or 1L will also be the right 1. Though I'm surprised simple 1 > > > doesn't work. > > > > Use "int64_t" would be more specific and understandable? > > How would it be so? Is it important to know in this context that min_version is > an int64_t? We certainly don't annotate our other throw-away constants that > way, you weren't explicit with id (which is not a const char*). The reason I was thinking is that: manifest->min_version is defined to int64_t explicitly and 'L' doesn't mean 64 bit integer, make sense? https://codereview.chromium.org/2801513002/diff/490001/components/payments/an... File components/payments/android/web_app_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/490001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:138: "LIMIT 1")); On 2017/04/11 19:31:46, Scott Hess wrote: > Why are you doing the LIMIT 1, here? > > Oooooh. Shouldn't you have a unique index on [id]? Then you could also use > INSERT OR REPLACE in AddWebAppManifest(), which would allow you to drop the > DELETE and remove the explicit sql::Transaction (because the INSERT OR REPLACE > is implicitly atomic). Done. https://codereview.chromium.org/2801513002/diff/490001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:142: return nullptr; On 2017/04/11 19:31:46, Scott Hess wrote: > This is implicit in s.Step(). It will return |false| if the statement is > invalid. There are a few edge cases involving testing of error-handling which > sometimes need an explicit is_valid() test, but if you don't need it, best to > drop it. > > Also, Chromium style below would be to if(!s.Step()) return nullptr, then > un-nest the current if() block. Done. https://codereview.chromium.org/2801513002/diff/490001/components/payments/an... File components/payments/android/web_app_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/490001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:102: manifest_1->fingerprints.push_back(kFingerprintTwo); On 2017/04/11 19:31:46, Scott Hess wrote: > Thanks, I like this better. The one problem is that I'm not entirely sure > whether the global vectors will work out, though perhaps static initializers are > fine in tests? Or if constexpr can be added? Done. Changed to generate fingerprint dynamically https://codereview.chromium.org/2801513002/diff/490001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:126: } On 2017/04/11 19:31:46, Scott Hess wrote: > ASSERT_TRUE(bobpay_manifest->fingerprints[0] == kFingerprintOne); > > should work fine. Though if static objects are banned in tests, that changes > things. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Thanks for the changes. LGTM with nits. [Otherwise, THUNDERDOME!] https://codereview.chromium.org/2801513002/diff/530001/components/payments/an... File components/payments/android/web_app_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/530001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:31: std::unique_ptr<std::vector<uint8_t>> GenerateFingerprint(uint8_t seed) { plain std::vector<> should work fine for this. No need for std::move() in return, RVO will handle it. https://codereview.chromium.org/2801513002/diff/530001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:60: std::move(GenerateFingerprint(1)); Then these can just be std::vector<>, too. I think you need the std::move() here if you want to be pedantic, but, honestly, I don't think I'd bother.
pwnall@chromium.org changed reviewers: + pwnall@chromium.org
RS LGTM
Patchset #7 (id:430001) has been deleted
Thanks https://codereview.chromium.org/2801513002/diff/530001/components/payments/an... File components/payments/android/web_app_manifest_section_table_unittest.cc (right): https://codereview.chromium.org/2801513002/diff/530001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:31: std::unique_ptr<std::vector<uint8_t>> GenerateFingerprint(uint8_t seed) { On 2017/04/11 23:37:10, Scott Hess wrote: > plain std::vector<> should work fine for this. No need for std::move() in > return, RVO will handle it. Done. https://codereview.chromium.org/2801513002/diff/530001/components/payments/an... components/payments/android/web_app_manifest_section_table_unittest.cc:60: std::move(GenerateFingerprint(1)); On 2017/04/11 23:37:10, Scott Hess wrote: > Then these can just be std::vector<>, too. I think you need the std::move() > here if you want to be pedantic, but, honestly, I don't think I'd bother. Done.
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % comments https://codereview.chromium.org/2801513002/diff/550001/components/payments/an... File components/payments/android/web_app_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/550001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:98: DCHECK(manifest != nullptr); Either DCHECK_NE(nullptr, manifest) or DCHECK(manifest); https://codereview.chromium.org/2801513002/diff/550001/components/payments/an... File components/payments/android/web_app_manifest_section_table.h (right): https://codereview.chromium.org/2801513002/diff/550001/components/payments/an... components/payments/android/web_app_manifest_section_table.h:49: mojom::WebAppManifestSectionPtr GetWebAppManifest(std::string web_app); const-ref parameter
On 2017/04/12 13:55:18, gogerald1 wrote: > Thanks > > https://codereview.chromium.org/2801513002/diff/530001/components/payments/an... > File components/payments/android/web_app_manifest_section_table_unittest.cc > (right): > > https://codereview.chromium.org/2801513002/diff/530001/components/payments/an... > components/payments/android/web_app_manifest_section_table_unittest.cc:31: > std::unique_ptr<std::vector<uint8_t>> GenerateFingerprint(uint8_t seed) { > On 2017/04/11 23:37:10, Scott Hess wrote: > > plain std::vector<> should work fine for this. No need for std::move() in > > return, RVO will handle it. > > Done. > > https://codereview.chromium.org/2801513002/diff/530001/components/payments/an... > components/payments/android/web_app_manifest_section_table_unittest.cc:60: > std::move(GenerateFingerprint(1)); > On 2017/04/11 23:37:10, Scott Hess wrote: > > Then these can just be std::vector<>, too. I think you need the std::move() > > here if you want to be pedantic, but, honestly, I don't think I'd bother. > > Done. Still LGTM. Thanks. [Rouslan's requests look reasonable, though :-).]
Thanks all, sending to CQ https://codereview.chromium.org/2801513002/diff/550001/components/payments/an... File components/payments/android/web_app_manifest_section_table.cc (right): https://codereview.chromium.org/2801513002/diff/550001/components/payments/an... components/payments/android/web_app_manifest_section_table.cc:98: DCHECK(manifest != nullptr); On 2017/04/12 14:28:09, ಠ_ಠ wrote: > Either DCHECK_NE(nullptr, manifest) or DCHECK(manifest); Done. https://codereview.chromium.org/2801513002/diff/550001/components/payments/an... File components/payments/android/web_app_manifest_section_table.h (right): https://codereview.chromium.org/2801513002/diff/550001/components/payments/an... components/payments/android/web_app_manifest_section_table.h:49: mojom::WebAppManifestSectionPtr GetWebAppManifest(std::string web_app); On 2017/04/12 14:28:09, ಠ_ಠ wrote: > const-ref parameter Done.
The CQ bit was checked by gogerald@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, sdefresne@chromium.org, pwnall@chromium.org, rouslan@chromium.org, shess@chromium.org Link to the patchset: https://codereview.chromium.org/2801513002/#ps570001 (title: "")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 570001, "attempt_start_ts": 1492010878098320,
"parent_rev": "2296abe71f34c79cc7ef1c01b22fc9e11283e457", "commit_rev":
"e26551574a7e760008a0151a6d8afcd82757f784"}
CQ is committing da patch.
Bot data: {"patchset_id": 570001, "attempt_start_ts": 1492010878098320,
"parent_rev": "3804c15d9dd121b800391b7467e5e53223d8373c", "commit_rev":
"753494c7f71f9a82c63440f47140e475df7e8771"}
Message was sent while issue was closed.
Description was changed from
==========
Add web app manifest section table in SQLite web database
The table has three columns: {'id', 'min_version', 'fingerprints'}.
The table has no primary key.
This table is used to reduce payment app's loading time in payment request,
refer below doc for details.
https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj...
BUG=708508
==========
to
==========
Add web app manifest section table in SQLite web database
The table has three columns: {'id', 'min_version', 'fingerprints'}.
The table has no primary key.
This table is used to reduce payment app's loading time in payment request,
refer below doc for details.
https://docs.google.com/a/google.com/document/d/1Ncsp96Ae5836NLOdPwHRzcX3zkgj...
BUG=708508
Review-Url: https://codereview.chromium.org/2801513002
Cr-Commit-Position: refs/heads/master@{#464062}
Committed:
https://chromium.googlesource.com/chromium/src/+/753494c7f71f9a82c63440f47140...
==========
Message was sent while issue was closed.
Committed patchset #10 (id:570001) as https://chromium.googlesource.com/chromium/src/+/753494c7f71f9a82c63440f47140... |
