Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556433002:
PaymentApp: Implement GetAllManifests() in PaymentAppContext. (Closed)
DescriptionPaymentApp: Implement GetAllManifests() in PaymentAppContext.
This CL is implementing GetAllManifests() method in PaymentAppContext. It is
used to query all manifests data in Chrome layer to display all registered
payment apps in the payment request UI.
Wrote a test for this CL in payment_app_context_impl_unittest.cc and moved
a lot of existing codes in payment_app_manager_unittest.cc to
payment_app_content_unittest_base.cc to share many codes between the new test
and the existing test.
BUG=661608
TEST=payment_app_context_impl_unittest.cc
Committed: https://crrev.com/e7f7d131d832a62ea5e480c7781736acfba90f62
Cr-Commit-Position: refs/heads/master@{#439546}
Patch Set 1 #Patch Set 2 #
Total comments: 2
Patch Set 3 #
Total comments: 23
Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 #
Total comments: 56
Patch Set 7 : addressed comments #Messages
Total messages: 41 (26 generated)
Description was changed from ========== Export BUG= ========== to ========== Export BUG= ==========
jinho.bang@samsung.com changed reviewers: + tommyt@opera.com
Hi Tommy, I'm really sorry. I didn't know that the issue is protected.. Maybe, you can see this issue from now.
https://codereview.chromium.org/2556433002/diff/20001/content/public/browser/... File content/public/browser/payment_app_context.h (right): https://codereview.chromium.org/2556433002/diff/20001/content/public/browser/... content/public/browser/payment_app_context.h:16: virtual void GetAllManifests(const GetAllManifestsCallback& callback) = 0; Tommy, To query all manifest data, I think you can use this function in chrome side. Also, you probably should change scope_url to registration_id. What do you think?
https://codereview.chromium.org/2556433002/diff/20001/content/public/browser/... File content/public/browser/payment_app_context.h (right): https://codereview.chromium.org/2556433002/diff/20001/content/public/browser/... content/public/browser/payment_app_context.h:16: virtual void GetAllManifests(const GetAllManifestsCallback& callback) = 0; On 2016/12/07 12:33:02, zino wrote: > Tommy, > > To query all manifest data, I think you can use this function in chrome side. > Also, you probably should change scope_url to registration_id. > What do you think? I think that sounds good. I will make the change you suggested.
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Description was changed from ========== Export BUG= ========== to ========== PaymentApp: Implement GetAllManifests() in PaymentAppContext. This CL is implementing GetAllManifests() method in PaymentAppContext. It is used to query all manifests data in Chrome layer to display all registered payment apps in the payment request UI. Wrote a test for this CL in payment_app_context_impl_unittest.cc and moved a lot of existing codes in payment_app_manager_unittest.cc to payment_app_content_unittest_base.cc to share many codes between the new test and the existing test. BUG=661608 TEST=payment_app_context_impl_unittest.cc ==========
jinho.bang@samsung.com changed reviewers: + avi@chromium.org, rouslan@chromium.org
PTAL + rouslan@ (and tommyt@) for content/browser/payments + Avi@ for other parts of content directory.
non payment content lgtm
The CQ bit was checked by jinho.bang@samsung.com 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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #4 (id:100001) has been deleted
https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... File content/browser/payments/payment_app_content_unittest_base.cc (right): https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_content_unittest_base.cc:44: payment_app_context_ = new PaymentAppContextImpl(); Can you initialize payment_app_context_ in the member initializer list? https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_content_unittest_base.cc:75: for (auto& manager : payment_app_context_->services_) { const auto& https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_content_unittest_base.cc:92: NOTREACHED(); return nullptr; https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... File content/browser/payments/payment_app_context_impl.cc (right): https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl.cc:58: PaymentAppDatabase* PaymentAppContextImpl::payment_app_database() const { DCHECK_CURRENTLY_ON(BrowserThread::IO); https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... File content/browser/payments/payment_app_context_impl.h (right): https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl.h:43: // Should be accessed only on the IO thread. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl.h:46: // Call on the UI thread. // Should be called on the UI thread. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl.h:64: // void DidGetAllManifestsOnIO(const PaymentAppDatabase::Manifests& Seems you forgot to remove this. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... File content/browser/payments/payment_app_context_impl_unittest.cc (right): https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl_unittest.cc:25: PaymentAppContextTest() {} Need a destructor as well. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl_unittest.cc:64: }; The standard way to create test data list is: static const struct { const char* scopeUrl; const char* scriptUrl; } kPaymentAppInfo[] = { {"https://example.com/a", "https://example.com/a/script.js"}, {"https://example.com/b", "https://example.com/b/script.js"}, {"https://example.com/c", "https://example.com/c/script.js"} }; Then use it in a loop like so: for (size_t i = 0; i < arraysize(kPaymentAppInfo); ++i) { CreatePaymentApp(GURL(kPaymentAppInfo[i].scopeUrl), GURL(kPaymentAppInfo[i].scriptUrl)); } https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl_unittest.cc:75: for (auto& manifest : manifests) { const auto& https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl_unittest.cc:76: EXPECT_EQ(manifest.second->icon, std::string("payment-app-icon")); Is std::string() wrapper necessary? I see that you're using "visa" without the wrapper below. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... File content/browser/payments/payment_app_manager_unittest.cc (right): https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_manager_unittest.cc:74: EXPECT_EQ(read_manifest->options[0]->icon, std::string("payment-app-icon")); Same question about std::string() wrappers in this file.
Patchset #5 (id:140001) has been deleted
Thank you for review. I addressed all your comments. Could you please review again? https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... File content/browser/payments/payment_app_content_unittest_base.cc (right): https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_content_unittest_base.cc:44: payment_app_context_ = new PaymentAppContextImpl(); On 2016/12/12 20:17:39, rouslan wrote: > Can you initialize payment_app_context_ in the member initializer list? Done. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_content_unittest_base.cc:75: for (auto& manager : payment_app_context_->services_) { On 2016/12/12 20:17:39, rouslan wrote: > const auto& Done. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_content_unittest_base.cc:92: On 2016/12/12 20:17:39, rouslan wrote: > NOTREACHED(); > return nullptr; Done. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... File content/browser/payments/payment_app_context_impl.cc (right): https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl.cc:58: PaymentAppDatabase* PaymentAppContextImpl::payment_app_database() const { On 2016/12/12 20:17:39, rouslan wrote: > DCHECK_CURRENTLY_ON(BrowserThread::IO); Done. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... File content/browser/payments/payment_app_context_impl.h (right): https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl.h:43: On 2016/12/12 20:17:39, rouslan wrote: > // Should be accessed only on the IO thread. Done. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl.h:46: // Call on the UI thread. On 2016/12/12 20:17:39, rouslan wrote: > // Should be called on the UI thread. Done. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl.h:64: // void DidGetAllManifestsOnIO(const PaymentAppDatabase::Manifests& On 2016/12/12 20:17:39, rouslan wrote: > Seems you forgot to remove this. Done. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... File content/browser/payments/payment_app_context_impl_unittest.cc (right): https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl_unittest.cc:25: PaymentAppContextTest() {} On 2016/12/12 20:17:39, rouslan wrote: > Need a destructor as well. Done. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl_unittest.cc:64: }; On 2016/12/12 20:17:39, rouslan wrote: > The standard way to create test data list is: > > static const struct { > const char* scopeUrl; > const char* scriptUrl; > } kPaymentAppInfo[] = { > {"https://example.com/a", "https://example.com/a/script.js"}, > {"https://example.com/b", "https://example.com/b/script.js"}, > {"https://example.com/c", "https://example.com/c/script.js"} > }; > > Then use it in a loop like so: > > for (size_t i = 0; i < arraysize(kPaymentAppInfo); ++i) { > CreatePaymentApp(GURL(kPaymentAppInfo[i].scopeUrl), > GURL(kPaymentAppInfo[i].scriptUrl)); > } Done. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl_unittest.cc:75: for (auto& manifest : manifests) { On 2016/12/12 20:17:39, rouslan wrote: > const auto& Done. https://codereview.chromium.org/2556433002/diff/80001/content/browser/payment... content/browser/payments/payment_app_context_impl_unittest.cc:76: EXPECT_EQ(manifest.second->icon, std::string("payment-app-icon")); On 2016/12/12 20:17:39, rouslan wrote: > Is std::string() wrapper necessary? I see that you're using "visa" without the > wrapper below. The type is base::Optional<std::string>. https://cs.chromium.org/chromium/src/out/Debug/gen/components/payments/paymen...
https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... File content/browser/payments/payment_app_content_unittest_base.cc (right): https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:12: #include "content/browser/service_worker/service_worker_context_wrapper.h" You don't use ServiceWorkerContextWrapper anywhere here. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:14: #include "content/public/test/test_browser_context.h" You don't use TestBrowserContext anywhere here/. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:22: ServiceWorkerStatusCode status, #include "content/common/service_worker/service_worker_status_code.h" https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:24: int64_t registration_id) { #include <stdint.h> https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:34: embedded_worker_helper_(new EmbeddedWorkerTestHelper(base::FilePath())), #include "base/files/file_path.h" https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:42: payment_app_context_->Init(embedded_worker_helper_->context_wrapper()); You should RunUntilIdle here, because Init() posts a task to IO thread. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:77: payments::mojom::PaymentAppManagerPtr manager; You're using the name "manager" for both for-loop iteration and as a stack variable here as well. This can get confusing and lead to bugs. Please use a different name here. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:78: mojo::InterfaceRequest<payments::mojom::PaymentAppManager> request = #include "mojo/public/cpp/bindings/interface_request.h" https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:79: mojo::GetProxy(&manager); #include "mojo/public/cpp/bindings/associated_interface_ptr.h" https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:87: existing_managers.find(manager.first) != existing_managers.end()) Your comment and your code disagrees. Use this to match your comment: if (existing_managers.find(manager.first) == existing_managers.end()) https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:100: ASSERT_TRUE(manager); ASSERT_NE(nullptr, manager) is better, because |manager| is not a boolean. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... File content/browser/payments/payment_app_content_unittest_base.h (right): https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.h:12: #include "content/browser/payments/payment_app_context_impl.h" You may be able to use forward declaration: class PaymentAppContextImpl; https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.h:27: PaymentAppContextImpl* GetPaymentAppContext() const; simple getters can be defined in the header with underscores in the name instead of CamelCase: PaymentAppContextImpl* payment_app_context() const { return payment_app_context_.get(); } https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.h:28: PaymentAppManager* CreatePaymentAppManager(const GURL& scope_url, #include "url/gurl.h" https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.h:32: payments::mojom::PaymentAppManifestPtr manifest, #include "components/payments/payment_app.mojom.h" https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.h:42: scoped_refptr<PaymentAppContextImpl> payment_app_context_; #include "base/memory/ref_counted.h" https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.h:45: DISALLOW_COPY_AND_ASSIGN(PaymentAppContentUnitTestBase); #include "base/macros.h" https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... File content/browser/payments/payment_app_context_impl_unittest.cc (right): https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:12: payments::mojom::PaymentAppManifestError* out_error, #include "components/payments/payment_app.mojom.h" https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:19: PaymentAppContext::Manifests* out_manifests, #include "content/public/browser/payment_app_context.h" https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:22: *out_manifests = std::move(manifests); #include <utility> https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:37: void CreatePaymentApp(const GURL& scope_url, const GURL& sw_script_url) { #include "url/gurl.h" https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:38: PaymentAppManager* manager = class PaymentAppManager; (forward declare on top of the file) https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:45: option->icon = std::string("payment-app-icon"); I doubt you need to do std::string(), but if you do, then #include <string> https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:65: DISALLOW_COPY_AND_ASSIGN(PaymentAppContextTest); #include "base/macros.h" https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:68: TEST_F(PaymentAppContextTest, Test) { #include "testing/gtest/include/gtest/gtest.h" https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:77: for (size_t i = 0; i < arraysize(kPaymentAppInfo); i++) #include <cstddef> https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:77: for (size_t i = 0; i < arraysize(kPaymentAppInfo); i++) Need {} for the for-loop.
Thank you for review. I addressed your comments. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... File content/browser/payments/payment_app_content_unittest_base.cc (right): https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:12: #include "content/browser/service_worker/service_worker_context_wrapper.h" On 2016/12/16 21:41:21, rouslan wrote: > You don't use ServiceWorkerContextWrapper anywhere here. Used it here: embedded_worker_helper_->context_wrapper()->set_storage_partition() https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:14: #include "content/public/test/test_browser_context.h" On 2016/12/16 21:41:21, rouslan wrote: > You don't use TestBrowserContext anywhere here/. Used it here: new StoragePartitionImpl(embedded_worker_helper_->browser_context(), ...) https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:22: ServiceWorkerStatusCode status, On 2016/12/16 21:41:21, rouslan wrote: > #include "content/common/service_worker/service_worker_status_code.h" Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:24: int64_t registration_id) { On 2016/12/16 21:41:21, rouslan wrote: > #include <stdint.h> Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:34: embedded_worker_helper_(new EmbeddedWorkerTestHelper(base::FilePath())), On 2016/12/16 21:41:20, rouslan wrote: > #include "base/files/file_path.h" Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:42: payment_app_context_->Init(embedded_worker_helper_->context_wrapper()); On 2016/12/16 21:41:21, rouslan wrote: > You should RunUntilIdle here, because Init() posts a task to IO thread. Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:77: payments::mojom::PaymentAppManagerPtr manager; On 2016/12/16 21:41:21, rouslan wrote: > You're using the name "manager" for both for-loop iteration and as a stack > variable here as well. This can get confusing and lead to bugs. Please use a > different name here. Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:78: mojo::InterfaceRequest<payments::mojom::PaymentAppManager> request = On 2016/12/16 21:41:21, rouslan wrote: > #include "mojo/public/cpp/bindings/interface_request.h" Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:79: mojo::GetProxy(&manager); On 2016/12/16 21:41:20, rouslan wrote: > #include "mojo/public/cpp/bindings/associated_interface_ptr.h" Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:87: existing_managers.find(manager.first) != existing_managers.end()) On 2016/12/16 21:41:20, rouslan wrote: > Your comment and your code disagrees. Use this to match your comment: > > if (existing_managers.find(manager.first) == existing_managers.end()) You're right.. I used base::ContainsKey() instead. Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:100: ASSERT_TRUE(manager); On 2016/12/16 21:41:21, rouslan wrote: > ASSERT_NE(nullptr, manager) is better, because |manager| is not a boolean. Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... File content/browser/payments/payment_app_content_unittest_base.h (right): https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.h:12: #include "content/browser/payments/payment_app_context_impl.h" On 2016/12/16 21:41:21, rouslan wrote: > You may be able to use forward declaration: > > class PaymentAppContextImpl; Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.h:27: PaymentAppContextImpl* GetPaymentAppContext() const; On 2016/12/16 21:41:21, rouslan wrote: > simple getters can be defined in the header with underscores in the name instead > of CamelCase: > > PaymentAppContextImpl* payment_app_context() const { return > payment_app_context_.get(); } Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.h:28: PaymentAppManager* CreatePaymentAppManager(const GURL& scope_url, On 2016/12/16 21:41:21, rouslan wrote: > #include "url/gurl.h" Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.h:32: payments::mojom::PaymentAppManifestPtr manifest, On 2016/12/16 21:41:21, rouslan wrote: > #include "components/payments/payment_app.mojom.h" Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.h:42: scoped_refptr<PaymentAppContextImpl> payment_app_context_; On 2016/12/16 21:41:21, rouslan wrote: > #include "base/memory/ref_counted.h" Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.h:45: DISALLOW_COPY_AND_ASSIGN(PaymentAppContentUnitTestBase); On 2016/12/16 21:41:21, rouslan wrote: > #include "base/macros.h" Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... File content/browser/payments/payment_app_context_impl_unittest.cc (right): https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:12: payments::mojom::PaymentAppManifestError* out_error, On 2016/12/16 21:41:21, rouslan wrote: > #include "components/payments/payment_app.mojom.h" Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:19: PaymentAppContext::Manifests* out_manifests, On 2016/12/16 21:41:21, rouslan wrote: > #include "content/public/browser/payment_app_context.h" Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:22: *out_manifests = std::move(manifests); On 2016/12/16 21:41:21, rouslan wrote: > #include <utility> Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:37: void CreatePaymentApp(const GURL& scope_url, const GURL& sw_script_url) { On 2016/12/16 21:41:21, rouslan wrote: > #include "url/gurl.h" Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:38: PaymentAppManager* manager = On 2016/12/16 21:41:21, rouslan wrote: > class PaymentAppManager; > > (forward declare on top of the file) Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:45: option->icon = std::string("payment-app-icon"); On 2016/12/16 21:41:21, rouslan wrote: > I doubt you need to do std::string(), but if you do, then #include <string> There is no way to set const char*. The option->icon is base::Optional type, not std::string type. The base::Optional class has only '=' operator overriding for base::Optional type itself. If we want to allow const char*, we will have to add a new overriding for T(std::string) type in the class. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:65: DISALLOW_COPY_AND_ASSIGN(PaymentAppContextTest); On 2016/12/16 21:41:21, rouslan wrote: > #include "base/macros.h" Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:68: TEST_F(PaymentAppContextTest, Test) { On 2016/12/16 21:41:21, rouslan wrote: > #include "testing/gtest/include/gtest/gtest.h" Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:77: for (size_t i = 0; i < arraysize(kPaymentAppInfo); i++) On 2016/12/16 21:41:21, rouslan wrote: > #include <cstddef> Done. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_context_impl_unittest.cc:77: for (size_t i = 0; i < arraysize(kPaymentAppInfo); i++) On 2016/12/16 21:41:22, rouslan wrote: > Need {} for the for-loop. Done.
The CQ bit was checked by jinho.bang@samsung.com 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: This issue passed the CQ dry run.
LGTM % nit https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... File content/browser/payments/payment_app_content_unittest_base.cc (right): https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:12: #include "content/browser/service_worker/service_worker_context_wrapper.h" On 2016/12/17 17:11:54, zino wrote: > On 2016/12/16 21:41:21, rouslan wrote: > > You don't use ServiceWorkerContextWrapper anywhere here. > > Used it here: > embedded_worker_helper_->context_wrapper()->set_storage_partition() Acknowledged. https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... content/browser/payments/payment_app_content_unittest_base.cc:14: #include "content/public/test/test_browser_context.h" On 2016/12/17 17:11:54, zino wrote: > On 2016/12/16 21:41:21, rouslan wrote: > > You don't use TestBrowserContext anywhere here/. > > Used it here: > new StoragePartitionImpl(embedded_worker_helper_->browser_context(), ...) Calling browser_context() without dereferencing the returned pointer should not require an include, IIRC.
On 2016/12/19 14:48:05, rouslan wrote: > LGTM % nit > > https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... > File content/browser/payments/payment_app_content_unittest_base.cc (right): > > https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... > content/browser/payments/payment_app_content_unittest_base.cc:12: #include > "content/browser/service_worker/service_worker_context_wrapper.h" > On 2016/12/17 17:11:54, zino wrote: > > On 2016/12/16 21:41:21, rouslan wrote: > > > You don't use ServiceWorkerContextWrapper anywhere here. > > > > Used it here: > > embedded_worker_helper_->context_wrapper()->set_storage_partition() > > Acknowledged. > > https://codereview.chromium.org/2556433002/diff/180001/content/browser/paymen... > content/browser/payments/payment_app_content_unittest_base.cc:14: #include > "content/public/test/test_browser_context.h" > On 2016/12/17 17:11:54, zino wrote: > > On 2016/12/16 21:41:21, rouslan wrote: > > > You don't use TestBrowserContext anywhere here/. > > > > Used it here: > > new StoragePartitionImpl(embedded_worker_helper_->browser_context(), ...) > > Calling browser_context() without dereferencing the returned pointer should not > require an include, IIRC. Yeah, but the browser_context() will return TestBrowserContext* which is derived from BrowserContext. I'm not sure but the compiler will have to know that TestBrowserContext is derived from the BrowserContext. If we don't include the header, we will meet the following error: ../../content/browser/payments/payment_app_content_unittest_base.cc:45:15: error: no matching constructor for initialization of 'content::StoragePartitionImpl' new StoragePartitionImpl(embedded_worker_helper_->browser_context(), ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../content/browser/storage_partition_impl.h:169:3: note: candidate constructor not viable: cannot convert argument of incomplete type 'content::TestBrowserContext *' to 'content::BrowserContext *' for 1st argument StoragePartitionImpl(BrowserContext* browser_context, ^ ../../content/browser/storage_partition_impl.h:232:28: note: candidate constructor not viable: requires 1 argument, but 3 were provided DISALLOW_COPY_AND_ASSIGN(StoragePartitionImpl);
On 2016/12/19 15:09:29, zino wrote: > If we don't include the header, we will meet the following error: Thank you for the update. Go ahead with the patch as-is.
The CQ bit was checked by jinho.bang@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org Link to the patchset: https://codereview.chromium.org/2556433002/#ps200001 (title: "addressed comments")
The CQ bit was unchecked by jinho.bang@samsung.com
The CQ bit was checked by jinho.bang@samsung.com 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 jinho.bang@samsung.com
The CQ bit was checked by jinho.bang@samsung.com
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": 200001, "attempt_start_ts": 1482172044867940,
"parent_rev": "30f9bd9ef61bcfde241ff1922756a705fae25b80", "commit_rev":
"419822ee19dc8c628ab033b91d320decdeb0c3c1"}
Message was sent while issue was closed.
Description was changed from ========== PaymentApp: Implement GetAllManifests() in PaymentAppContext. This CL is implementing GetAllManifests() method in PaymentAppContext. It is used to query all manifests data in Chrome layer to display all registered payment apps in the payment request UI. Wrote a test for this CL in payment_app_context_impl_unittest.cc and moved a lot of existing codes in payment_app_manager_unittest.cc to payment_app_content_unittest_base.cc to share many codes between the new test and the existing test. BUG=661608 TEST=payment_app_context_impl_unittest.cc ========== to ========== PaymentApp: Implement GetAllManifests() in PaymentAppContext. This CL is implementing GetAllManifests() method in PaymentAppContext. It is used to query all manifests data in Chrome layer to display all registered payment apps in the payment request UI. Wrote a test for this CL in payment_app_context_impl_unittest.cc and moved a lot of existing codes in payment_app_manager_unittest.cc to payment_app_content_unittest_base.cc to share many codes between the new test and the existing test. BUG=661608 TEST=payment_app_context_impl_unittest.cc Review-Url: https://codereview.chromium.org/2556433002 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== PaymentApp: Implement GetAllManifests() in PaymentAppContext. This CL is implementing GetAllManifests() method in PaymentAppContext. It is used to query all manifests data in Chrome layer to display all registered payment apps in the payment request UI. Wrote a test for this CL in payment_app_context_impl_unittest.cc and moved a lot of existing codes in payment_app_manager_unittest.cc to payment_app_content_unittest_base.cc to share many codes between the new test and the existing test. BUG=661608 TEST=payment_app_context_impl_unittest.cc Review-Url: https://codereview.chromium.org/2556433002 ========== to ========== PaymentApp: Implement GetAllManifests() in PaymentAppContext. This CL is implementing GetAllManifests() method in PaymentAppContext. It is used to query all manifests data in Chrome layer to display all registered payment apps in the payment request UI. Wrote a test for this CL in payment_app_context_impl_unittest.cc and moved a lot of existing codes in payment_app_manager_unittest.cc to payment_app_content_unittest_base.cc to share many codes between the new test and the existing test. BUG=661608 TEST=payment_app_context_impl_unittest.cc Committed: https://crrev.com/e7f7d131d832a62ea5e480c7781736acfba90f62 Cr-Commit-Position: refs/heads/master@{#439546} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e7f7d131d832a62ea5e480c7781736acfba90f62 Cr-Commit-Position: refs/heads/master@{#439546} |
