Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(604)

Issue 2586203003: PaymentApp: Remove scope_url parameter from Get/SetManifest methods. (Closed)

Created:
4 years ago by zino
Modified:
4 years ago
CC:
chromium-reviews, qsr+mojo_chromium.org, gogerald+paymentswatch_chromium.org, viettrungluu+watch_chromium.org, jam, yzshen+watch_chromium.org, abarth-chromium, haraken, Aaron Boodman, rouslan+payments_chromium.org, darin-cc_chromium.org, blink-reviews, darin (slow to review), sebsg+paymentswatch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

PaymentApp: Remove scope_url parameter from Get/SetManifest methods. When calling setManifest()/getManifest() from Blink side, we currently pass scope_url to find associated service worker in Content layer. However, it is not a good way to pass scope_url each time. Of course, since the implementations are all private methods, there is no risk of calling it wrong from the outside. But some test classes are declared as friend class and it can be misused in the classes. Therefore, this CL is adding Init() in payment_app.mojom and then using it in Blink side PaymentAppManager when creating the object. Then, we don't need to pass scope_url each time when calling setManifest()/getManifest(). BUG=661608 TEST=existing tests Committed: https://crrev.com/f87f3d68d3a82070b599948cf6fbed3b224e0bf0 Cr-Commit-Position: refs/heads/master@{#439942}

Patch Set 1 #

Total comments: 7

Patch Set 2 : PaymentApp: Remove scope_url parameter from Get/SetManifest methods. #

Total comments: 2

Patch Set 3 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+55 lines, -41 lines) Patch
M components/payments/payment_app.mojom View 1 chunk +3 lines, -2 lines 0 comments Download
M content/browser/payments/payment_app_content_unittest_base.h View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M content/browser/payments/payment_app_content_unittest_base.cc View 1 2 4 chunks +22 lines, -5 lines 0 comments Download
M content/browser/payments/payment_app_context_impl_unittest.cc View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M content/browser/payments/payment_app_manager.h View 1 3 chunks +5 lines, -6 lines 0 comments Download
M content/browser/payments/payment_app_manager.cc View 1 2 2 chunks +8 lines, -6 lines 0 comments Download
M content/browser/payments/payment_app_manager_unittest.cc View 1 2 6 chunks +10 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/modules/payments/PaymentAppManager.cpp View 3 chunks +5 lines, -5 lines 0 comments Download

Messages

Total messages: 24 (11 generated)
zino
PTAL rouslan@ for payments Tom Sepez@ for mojom
4 years ago (2016-12-19 17:55:32 UTC) #3
Tom Sepez
mojom lgtm
4 years ago (2016-12-19 18:09:39 UTC) #4
please use gerrit instead
https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/payment_app_content_unittest_base.h File content/browser/payments/payment_app_content_unittest_base.h (right): https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/payment_app_content_unittest_base.h#newcode41 content/browser/payments/payment_app_content_unittest_base.h:41: void UnregisterServiceWorker(const GURL& scope_url); This change seems orthogonal to ...
4 years ago (2016-12-19 20:45:59 UTC) #5
zino
Thank you for review. I left comments. https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/payment_app_content_unittest_base.h File content/browser/payments/payment_app_content_unittest_base.h (right): https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/payment_app_content_unittest_base.h#newcode41 content/browser/payments/payment_app_content_unittest_base.h:41: void UnregisterServiceWorker(const ...
4 years ago (2016-12-20 11:08:55 UTC) #6
please use gerrit instead
https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/payment_app_content_unittest_base.cc File content/browser/payments/payment_app_content_unittest_base.cc (right): https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/payment_app_content_unittest_base.cc#newcode98 content/browser/payments/payment_app_content_unittest_base.cc:98: candidate_manager.first->Init(scope_url.spec()); Run the loop here as well, because Init() ...
4 years ago (2016-12-20 16:57:54 UTC) #7
zino
Thank you for review. https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/payment_app_content_unittest_base.cc File content/browser/payments/payment_app_content_unittest_base.cc (right): https://codereview.chromium.org/2586203003/diff/1/content/browser/payments/payment_app_content_unittest_base.cc#newcode98 content/browser/payments/payment_app_content_unittest_base.cc:98: candidate_manager.first->Init(scope_url.spec()); On 2016/12/20 16:57:54, rouslan-intermittent-holidays ...
4 years ago (2016-12-20 17:18:37 UTC) #8
please use gerrit instead
https://codereview.chromium.org/2586203003/diff/20001/content/browser/payments/payment_app_context_impl_unittest.cc File content/browser/payments/payment_app_context_impl_unittest.cc (right): https://codereview.chromium.org/2586203003/diff/20001/content/browser/payments/payment_app_context_impl_unittest.cc#newcode103 content/browser/payments/payment_app_context_impl_unittest.cc:103: EXPECT_EQ(manifest.second->icon.value(), "payment-app-icon"); You seem to have reversed the "expected" ...
4 years ago (2016-12-20 17:25:17 UTC) #9
zino
https://codereview.chromium.org/2586203003/diff/20001/content/browser/payments/payment_app_context_impl_unittest.cc File content/browser/payments/payment_app_context_impl_unittest.cc (right): https://codereview.chromium.org/2586203003/diff/20001/content/browser/payments/payment_app_context_impl_unittest.cc#newcode103 content/browser/payments/payment_app_context_impl_unittest.cc:103: EXPECT_EQ(manifest.second->icon.value(), "payment-app-icon"); On 2016/12/20 17:25:17, rouslan-intermittent-holidays wrote: > You ...
4 years ago (2016-12-20 17:30:53 UTC) #10
zino
I've just rebased this CL. Thank you.
4 years ago (2016-12-20 18:36:34 UTC) #11
please use gerrit instead
lgtm
4 years ago (2016-12-20 19:24:06 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2586203003/40001
4 years ago (2016-12-20 22:41:35 UTC) #19
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years ago (2016-12-21 00:07:05 UTC) #22
commit-bot: I haz the power
4 years ago (2016-12-21 00:08:58 UTC) #24
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/f87f3d68d3a82070b599948cf6fbed3b224e0bf0
Cr-Commit-Position: refs/heads/master@{#439942}

Powered by Google App Engine
This is Rietveld 408576698