Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2586213002:
PaymentApp: Implement nativeGetAllAppManifests (Closed)
|
Created:
4 years ago by tommyt Modified:
3 years, 11 months ago CC:
chromium-reviews, wjmaclean, jam, gogerald+paymentswatch_chromium.org, rouslan+payments_chromium.org, darin-cc_chromium.org, agrieve+watch_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPaymentApp: Implement nativeGetAllAppManifests
This change connects the ServiceWorkerPaymentAppBridge to the service
worker payment app code in content/browser/payments. This allows
installed service worker payment apps to show up in the Payment Request
dialog.
I also replace "String scopeUrl" with "long registrationId" as the
identifier for service worker payment apps.
This code is protected by the kServiceWorkerPaymentApps feature flag.
BUG=661608
Committed: https://crrev.com/7bff1efb9ab1ee6e7e451e697d79fb49ee5fd811
Cr-Commit-Position: refs/heads/master@{#440746}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Fix compile errors #Patch Set 4 : int64_t -> jlong #
Total comments: 19
Patch Set 5 : Rebase #Patch Set 6 : Address review comments #
Total comments: 10
Patch Set 7 : Address Bernhard's comments #
Total comments: 2
Patch Set 8 : Address comments from Bernhard and Jinho #Patch Set 9 : Rebase #Messages
Total messages: 50 (31 generated)
The CQ bit was checked by tommyt@opera.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: 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 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 tommyt@opera.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_chromium_rel_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 tommyt@opera.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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by tommyt@opera.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.
The CQ bit was checked by jinho.bang@samsung.com
The CQ bit was unchecked by jinho.bang@samsung.com
Description was changed from ========== PaymentApp: Implement nativeGetAllAppManifests This change connects the ServiceWorkerPaymentAppBridge to the service worker payment app code in content/browser/payments. This allows installed service worker payment apps to show up in the Payment Request dialog. I also replace "String scopeUrl" with "long registrationId" as the identifier for service worker payment apps. BUG=661608 ========== to ========== PaymentApp: Implement nativeGetAllAppManifests This change connects the ServiceWorkerPaymentAppBridge to the service worker payment app code in content/browser/payments. This allows installed service worker payment apps to show up in the Payment Request dialog. I also replace "String scopeUrl" with "long registrationId" as the identifier for service worker payment apps. BUG=661608 ==========
jinho.bang@samsung.com changed reviewers: - jinho.bang@samsung.com
Sorry, I mis-checked commit button :P
tommyt@opera.com changed reviewers: + jinho.bang@samsung.com, rouslan@chromium.org
PTAL!
https://codereview.chromium.org/2586213002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java (right): https://codereview.chromium.org/2586213002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:35: * The scope url of the service worker. Update comments. https://codereview.chromium.org/2586213002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:66: */ Need javadoc for parameters. https://codereview.chromium.org/2586213002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:101: if (callback instanceof PaymentAppFactory.PaymentAppCreatedCallback) { assert instead of silently failing. https://codereview.chromium.org/2586213002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:109: if (callback instanceof PaymentAppFactory.PaymentAppCreatedCallback) { Ditto https://codereview.chromium.org/2586213002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java (right): https://codereview.chromium.org/2586213002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:40: private void installServiceWorkerPaymentApp(final int instrumentPresence) { installMockServiceWorkerPaymentApp Use the world "Mock" to signify that ServiceWorker infrastructure is not actually being invoked here. https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/... File chrome/browser/android/payments/service_worker_payment_app_bridge.cc (right): https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:23: Put private methods in anonymous namespace. https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:60: content::WebContents::FromJavaWebContents(jweb_contents); Check all these raw pointers for null. https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:71: content::BrowserThread::PostTask( You should be on UI thread already. Assert that you're on UI thread and call payment_app_context->GetAllManifests() directly instead.
To reassure everyone that the incomplete feature is still behind a flag, please put the flag name in CL description.
Description was changed from ========== PaymentApp: Implement nativeGetAllAppManifests This change connects the ServiceWorkerPaymentAppBridge to the service worker payment app code in content/browser/payments. This allows installed service worker payment apps to show up in the Payment Request dialog. I also replace "String scopeUrl" with "long registrationId" as the identifier for service worker payment apps. BUG=661608 ========== to ========== PaymentApp: Implement nativeGetAllAppManifests This change connects the ServiceWorkerPaymentAppBridge to the service worker payment app code in content/browser/payments. This allows installed service worker payment apps to show up in the Payment Request dialog. I also replace "String scopeUrl" with "long registrationId" as the identifier for service worker payment apps. This code is protected by the kServiceWorkerPaymentApps feature flag. BUG=661608 ==========
tommyt@opera.com changed reviewers: + avi@chromium.org, bauerb@chromium.org
Thank you Rouslan for a really helpful review. I have tried to address your
comments in the latest patchset.
I'm also adding bauerb@ as reviewer for:
chrome/browser/android/payments/service_worker_payment_app_bridge.cc
chrome/browser/browsing_data/browsing_data_remover_unittest.cc
... and avi@ for:
content/browser/storage_partition_impl.h
content/public/browser/storage_partition.h
I hope you have time to take a look, although I realize this is the holiday
season.
https://codereview.chromium.org/2586213002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java
(right):
https://codereview.chromium.org/2586213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:35:
* The scope url of the service worker.
On 2016/12/20 16:35:42, rouslan-intermittent-holidays wrote:
> Update comments.
Done.
https://codereview.chromium.org/2586213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:66:
*/
On 2016/12/20 16:35:41, rouslan-intermittent-holidays wrote:
> Need javadoc for parameters.
Done.
https://codereview.chromium.org/2586213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:101:
if (callback instanceof PaymentAppFactory.PaymentAppCreatedCallback) {
On 2016/12/20 16:35:41, rouslan-intermittent-holidays wrote:
> assert instead of silently failing.
Done.
https://codereview.chromium.org/2586213002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:109:
if (callback instanceof PaymentAppFactory.PaymentAppCreatedCallback) {
On 2016/12/20 16:35:41, rouslan-intermittent-holidays wrote:
> Ditto
Done.
https://codereview.chromium.org/2586213002/diff/60001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java
(right):
https://codereview.chromium.org/2586213002/diff/60001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:40:
private void installServiceWorkerPaymentApp(final int instrumentPresence) {
On 2016/12/20 16:35:42, rouslan-intermittent-holidays wrote:
> installMockServiceWorkerPaymentApp
>
> Use the world "Mock" to signify that ServiceWorker infrastructure is not
> actually being invoked here.
Done.
https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/...
File chrome/browser/android/payments/service_worker_payment_app_bridge.cc
(right):
https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/...
chrome/browser/android/payments/service_worker_payment_app_bridge.cc:23:
On 2016/12/20 16:35:42, rouslan-intermittent-holidays wrote:
> Put private methods in anonymous namespace.
Done.
https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/...
chrome/browser/android/payments/service_worker_payment_app_bridge.cc:60:
content::WebContents::FromJavaWebContents(jweb_contents);
On 2016/12/20 16:35:42, rouslan-intermittent-holidays wrote:
> Check all these raw pointers for null.
Done.
https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/...
chrome/browser/android/payments/service_worker_payment_app_bridge.cc:71:
content::BrowserThread::PostTask(
On 2016/12/20 16:35:42, rouslan-intermittent-holidays wrote:
> You should be on UI thread already. Assert that you're on UI thread and call
> payment_app_context->GetAllManifests() directly instead.
Done.
https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/... File chrome/browser/android/payments/service_worker_payment_app_bridge.cc (right): https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:60: content::WebContents::FromJavaWebContents(jweb_contents); On 2016/12/20 16:35:42, rouslan-intermittent-holidays wrote: > Check all these raw pointers for null. Sorry for the back and forth, but I'm going to have to disagree here. Unless you know that these can legitimately be null in the absence of bugs, don't check them for null. (If you don't know whether they can be null, leave the check out as well! If the assumption turns out to be false, you will get crashes, which manifest in crash reports or test failures, so you'll learn about it. If you would err on the side on leaving the check in, you'd never find out if you're wrong.) https://codereview.chromium.org/2586213002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java (right): https://codereview.chromium.org/2586213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:68: * @param registrationId The service worker registration if of the Payment App. "[…] service worker registration ID […]"? https://codereview.chromium.org/2586213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:69: * @param optionId The id of the PaymentOption that was selected by the user. Nit: Capitalize ID. https://codereview.chromium.org/2586213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:106: private static void onGotManifest(Manifest manifest, WebContents webContents, Object callback) { You can declare |callback| as a PaymentAppFactory.PaymentAppCreatedCallback. https://codereview.chromium.org/2586213002/diff/100001/chrome/browser/android... File chrome/browser/android/payments/service_worker_payment_app_bridge.cc (right): https://codereview.chromium.org/2586213002/diff/100001/chrome/browser/android... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:102: &OnGotAllManifests, env, ScopedJavaGlobalRef<jobject>(env, jweb_contents), Don't bind the JNIEnv into a callback (or hold on to it beyond the current iteration of the message loop, in general), just grab it again using AttachToCurrentThread() when the callback runs.
LGTM after you address Bernhard's comments. He has more expertise on Android, so his suggestions supersede mine.
https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/... File chrome/browser/android/payments/service_worker_payment_app_bridge.cc (right): https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:60: content::WebContents::FromJavaWebContents(jweb_contents); On 2016/12/22 14:03:07, Bernhard (OOO until Jan 3) wrote: > On 2016/12/20 16:35:42, rouslan-intermittent-holidays wrote: > > Check all these raw pointers for null. > > Sorry for the back and forth, but I'm going to have to disagree here. Unless you > know that these can legitimately be null in the absence of bugs, don't check > them for null. > > (If you don't know whether they can be null, leave the check out as well! If the > assumption turns out to be false, you will get crashes, which manifest in crash > reports or test failures, so you'll learn about it. If you would err on the side > on leaving the check in, you'd never find out if you're wrong.) web_contents: passed from Java and should never be null. browser_context: WebContents::GetBrowserContext() ends up calling NavigationController::GetBrowserContext() which states in comment that "it can never be nullptr." storage_partition: BrowserContext::GetStoragePartition() calls StoragePartitionImplMap::Get() which does not look like it can return nullptr (creates a new partition object if a matching one is not found) payment_app_context: StoragePartitionImpl::GetPaymentAppContext() returns an object that was created in StoragePartitionImpl::Create(), so it doesn't look like this should ever be nullptr. based on this, I have removed all the checks again. https://codereview.chromium.org/2586213002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java (right): https://codereview.chromium.org/2586213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:68: * @param registrationId The service worker registration if of the Payment App. On 2016/12/22 14:03:07, Bernhard (OOO until Jan 3) wrote: > "[…] service worker registration ID […]"? That was a silly mistake. Thanks for catching it! https://codereview.chromium.org/2586213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:69: * @param optionId The id of the PaymentOption that was selected by the user. On 2016/12/22 14:03:07, Bernhard (OOO until Jan 3) wrote: > Nit: Capitalize ID. Done. https://codereview.chromium.org/2586213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:106: private static void onGotManifest(Manifest manifest, WebContents webContents, Object callback) { On 2016/12/22 14:03:07, Bernhard (OOO until Jan 3) wrote: > You can declare |callback| as a PaymentAppFactory.PaymentAppCreatedCallback. That was my first (and preferred) approach, but if I declare the callback as PaymentAppFactory.PaymentAppCreatedCallback, the JNI code generator is not happy, and says: SyntaxError: Inner class (PaymentAppFactory.PaymentAppCreatedCallback) can not be used directly by JNI. Please import the outer class, probably: import org.chromium.chrome.browser.payments.PaymentAppFactory; If I do what this error says to do (namely importing the PaymentAppFactory class) JNI is happy, and everything works great, but "git cl upload" gives me the following error when trying to upload the change: ** Presubmit ERRORS ** chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:12:1: Redundant import from the same package - org.chromium.chrome.browser.payments.PaymentAppFactory. I can probably sort this out if I spend a little time thinking about it, but if you have some advice for me on how to best fix this, I would be thankful. https://codereview.chromium.org/2586213002/diff/100001/chrome/browser/android... File chrome/browser/android/payments/service_worker_payment_app_bridge.cc (right): https://codereview.chromium.org/2586213002/diff/100001/chrome/browser/android... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:102: &OnGotAllManifests, env, ScopedJavaGlobalRef<jobject>(env, jweb_contents), On 2016/12/22 14:03:07, Bernhard (OOO until Jan 3) wrote: > Don't bind the JNIEnv into a callback (or hold on to it beyond the current > iteration of the message loop, in general), just grab it again using > AttachToCurrentThread() when the callback runs. Done.
lgtm
https://codereview.chromium.org/2586213002/diff/120001/chrome/browser/android... File chrome/browser/android/payments/service_worker_payment_app_bridge.cc (right): https://codereview.chromium.org/2586213002/diff/120001/chrome/browser/android... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:73: content::BrowserContext::GetStoragePartition( I think we should use GetDefaultStoragePartition() or GetStoragePartition(browser_context, nullptr). If the site_url is extension scheme, this call will return other isolated storage partition. I'm not sure if PaymentRequest API can be used in Extension side. (Then, the site_url won't always extension scheme at least but...) But anyway, we should use GetDefaultStoragePartition() for safer code. I don't think so, but if we need to cover the service-workrer based payment app created in the extension side as well, we will have to use ForEachStoragePartition().
LGTM with one request: https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/... File chrome/browser/android/payments/service_worker_payment_app_bridge.cc (right): https://codereview.chromium.org/2586213002/diff/60001/chrome/browser/android/... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:60: content::WebContents::FromJavaWebContents(jweb_contents); On 2016/12/22 15:09:35, tommyt wrote: > On 2016/12/22 14:03:07, Bernhard (OOO until Jan 3) wrote: > > On 2016/12/20 16:35:42, rouslan-intermittent-holidays wrote: > > > Check all these raw pointers for null. > > > > Sorry for the back and forth, but I'm going to have to disagree here. Unless > you > > know that these can legitimately be null in the absence of bugs, don't check > > them for null. > > > > (If you don't know whether they can be null, leave the check out as well! If > the > > assumption turns out to be false, you will get crashes, which manifest in > crash > > reports or test failures, so you'll learn about it. If you would err on the > side > > on leaving the check in, you'd never find out if you're wrong.) > > web_contents: passed from Java and should never be null. > > browser_context: WebContents::GetBrowserContext() ends up calling > NavigationController::GetBrowserContext() which states in comment that "it can > never be nullptr." > > storage_partition: BrowserContext::GetStoragePartition() calls > StoragePartitionImplMap::Get() which does not look like it can return nullptr > (creates a new partition object if a matching one is not found) > > payment_app_context: StoragePartitionImpl::GetPaymentAppContext() returns an > object that was created in StoragePartitionImpl::Create(), so it doesn't look > like this should ever be nullptr. > > based on this, I have removed all the checks again. OK, thanks for the due diligence! https://codereview.chromium.org/2586213002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java (right): https://codereview.chromium.org/2586213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:106: private static void onGotManifest(Manifest manifest, WebContents webContents, Object callback) { On 2016/12/22 15:09:35, tommyt wrote: > On 2016/12/22 14:03:07, Bernhard (OOO until Jan 3) wrote: > > You can declare |callback| as a PaymentAppFactory.PaymentAppCreatedCallback. > > That was my first (and preferred) approach, but if I declare the callback as > PaymentAppFactory.PaymentAppCreatedCallback, the JNI code generator is not > happy, and says: > > SyntaxError: Inner class (PaymentAppFactory.PaymentAppCreatedCallback) can > not be used directly by JNI. Please import the outer class, probably: > import org.chromium.chrome.browser.payments.PaymentAppFactory; > > If I do what this error says to do (namely importing the PaymentAppFactory > class) JNI is happy, and everything works great, but "git cl upload" gives me > the following error when trying to upload the change: > > ** Presubmit ERRORS ** > > chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:12:1: > Redundant import from the same package - > org.chromium.chrome.browser.payments.PaymentAppFactory. > > I can probably sort this out if I spend a little time thinking about it, but if > you have some advice for me on how to best fix this, I would be thankful. Oh, that's annoying. It looks like this is https://crbug.com/505554. One way around it would be to move the callback to its own class; if you don't want to do that, can you add a comment here that explains this and references the bug?
Thanks everyone for reviews! https://codereview.chromium.org/2586213002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java (right): https://codereview.chromium.org/2586213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:106: private static void onGotManifest(Manifest manifest, WebContents webContents, Object callback) { On 2016/12/22 18:08:42, Bernhard (OOO until Jan 3) wrote: > On 2016/12/22 15:09:35, tommyt wrote: > > On 2016/12/22 14:03:07, Bernhard (OOO until Jan 3) wrote: > > > You can declare |callback| as a PaymentAppFactory.PaymentAppCreatedCallback. > > > > That was my first (and preferred) approach, but if I declare the callback as > > PaymentAppFactory.PaymentAppCreatedCallback, the JNI code generator is not > > happy, and says: > > > > SyntaxError: Inner class (PaymentAppFactory.PaymentAppCreatedCallback) can > > not be used directly by JNI. Please import the outer class, probably: > > import org.chromium.chrome.browser.payments.PaymentAppFactory; > > > > If I do what this error says to do (namely importing the PaymentAppFactory > > class) JNI is happy, and everything works great, but "git cl upload" gives me > > the following error when trying to upload the change: > > > > ** Presubmit ERRORS ** > > > > > chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:12:1: > > Redundant import from the same package - > > org.chromium.chrome.browser.payments.PaymentAppFactory. > > > > I can probably sort this out if I spend a little time thinking about it, but > if > > you have some advice for me on how to best fix this, I would be thankful. > > Oh, that's annoying. It looks like this is https://crbug.com/505554. One way > around it would be to move the callback to its own class; if you don't want to > do that, can you add a comment here that explains this and references the bug? I've added a TODO and CC'ed myself on that bug so that I can sort this out once 505554 gets fixed. https://codereview.chromium.org/2586213002/diff/120001/chrome/browser/android... File chrome/browser/android/payments/service_worker_payment_app_bridge.cc (right): https://codereview.chromium.org/2586213002/diff/120001/chrome/browser/android... chrome/browser/android/payments/service_worker_payment_app_bridge.cc:73: content::BrowserContext::GetStoragePartition( On 2016/12/22 17:00:25, zino wrote: > I think we should use GetDefaultStoragePartition() or > GetStoragePartition(browser_context, nullptr). > If the site_url is extension scheme, this call will return other isolated > storage partition. > > I'm not sure if PaymentRequest API can be used in Extension side. > (Then, the site_url won't always extension scheme at least but...) > But anyway, we should use GetDefaultStoragePartition() for safer code. > > I don't think so, but if we need to cover the service-workrer based > payment app created in the extension side as well, we will have to > use ForEachStoragePartition(). I have replaced GetStoragePartition() with GetDefaultStoragePartition().
lgtm
lgtm
The CQ bit was checked by tommyt@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, bauerb@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2586213002/#ps140001 (title: "Address comments from Bernhard and Jinho")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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...)
The CQ bit was checked by tommyt@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, rouslan@chromium.org, jinho.bang@samsung.com, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2586213002/#ps160001 (title: "Rebase")
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": 160001, "attempt_start_ts": 1482827832800070,
"parent_rev": "cd169d72d66e4ca3ad41e206d87549d55f764f5d", "commit_rev":
"0360c2f1c0582114e876a4f9367d1fd7344af9a3"}
Message was sent while issue was closed.
Description was changed from ========== PaymentApp: Implement nativeGetAllAppManifests This change connects the ServiceWorkerPaymentAppBridge to the service worker payment app code in content/browser/payments. This allows installed service worker payment apps to show up in the Payment Request dialog. I also replace "String scopeUrl" with "long registrationId" as the identifier for service worker payment apps. This code is protected by the kServiceWorkerPaymentApps feature flag. BUG=661608 ========== to ========== PaymentApp: Implement nativeGetAllAppManifests This change connects the ServiceWorkerPaymentAppBridge to the service worker payment app code in content/browser/payments. This allows installed service worker payment apps to show up in the Payment Request dialog. I also replace "String scopeUrl" with "long registrationId" as the identifier for service worker payment apps. This code is protected by the kServiceWorkerPaymentApps feature flag. BUG=661608 Review-Url: https://codereview.chromium.org/2586213002 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== PaymentApp: Implement nativeGetAllAppManifests This change connects the ServiceWorkerPaymentAppBridge to the service worker payment app code in content/browser/payments. This allows installed service worker payment apps to show up in the Payment Request dialog. I also replace "String scopeUrl" with "long registrationId" as the identifier for service worker payment apps. This code is protected by the kServiceWorkerPaymentApps feature flag. BUG=661608 Review-Url: https://codereview.chromium.org/2586213002 ========== to ========== PaymentApp: Implement nativeGetAllAppManifests This change connects the ServiceWorkerPaymentAppBridge to the service worker payment app code in content/browser/payments. This allows installed service worker payment apps to show up in the Payment Request dialog. I also replace "String scopeUrl" with "long registrationId" as the identifier for service worker payment apps. This code is protected by the kServiceWorkerPaymentApps feature flag. BUG=661608 Committed: https://crrev.com/7bff1efb9ab1ee6e7e451e697d79fb49ee5fd811 Cr-Commit-Position: refs/heads/master@{#440746} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/7bff1efb9ab1ee6e7e451e697d79fb49ee5fd811 Cr-Commit-Position: refs/heads/master@{#440746} |
