Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years ago by tommyt Modified:
4 years ago Reviewers:
ncarter (slow), please use gerrit instead, zino, David Trainor- moved to gerrit, agrieve CC:
chromium-reviews, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, agrieve+watch_chromium.org, zino1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPaymentApp: Add classes for supporting Web Based Payment Apps
This adds an application class, an instrument class and a skeleton
bridging class which can later be implemented to communicate with the
service worker class in C++.
The app factory class has been extended to create instances of the new
web based payment apps in addition to the existing autofill payment app.
BUG=669876
Committed: https://crrev.com/857f8a9174628615f9775f9383e0a64846d29d55
Cr-Commit-Position: refs/heads/master@{#436274}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Optimize getter function and remove useless member variable #Patch Set 3 : Use custom java types for manifests and options #
Total comments: 2
Patch Set 4 : Remove now unnecessary dependency to payment_app.mojom #
Total comments: 24
Patch Set 5 : Address review comments #
Total comments: 2
Patch Set 6 : web based -> service worker (based) #
Total comments: 2
Patch Set 7 : Manifest.id -> Manifest.scopeUrl #Patch Set 8 : Switch -> Feature #Patch Set 9 : Add unittests #Patch Set 10 : Rebase on latest master to get rid of patch errors #
Total comments: 4
Patch Set 11 : Fix some indentation and suppress some warnings #Patch Set 12 : Tweak the test cases a bit #Patch Set 13 : Make the service worker unittests work even with the feature flag off #
Created: 4 years ago
Messages
Total messages: 61 (37 generated)
tommyt@opera.com changed reviewers: + agrieve@chromium.org, jinho.bang@samsung.com, rouslan@chromium.org
Building on my previous patch, here's a set of java classes that we can use to integrate web based payment apps into the payment UI. PTAL
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.
Patch looks good to me. I left some comments. https://codereview.chromium.org/2526293003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentApp.java (right): https://codereview.chromium.org/2526293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentApp.java:11: import org.chromium.payments.mojom.PaymentAppManifest; Maybe we can not use mojom type here because the service worker data is saved in browser side. In other words, we don't need IPC for browser <--> browser. So, we should probably define a new data type for it in java side. IMHO, it's okay even if not exactly the same to PaymentAppManifest in IDL level. https://codereview.chromium.org/2526293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentApp.java:79: methods.add(methodName); This method seems just getter that always returns the same value. If so, we can move this logic to constructor and then just return the cached value here. https://codereview.chromium.org/2526293003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentInstrument.java (right): https://codereview.chromium.org/2526293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentInstrument.java:33: private final PaymentAppManifest mManifest; It's difficult to imagine use-case of this variable in this class. Could you let me know a example?
Also, we will need tests for this patch.
I've addressed all of zino's comments except for the one about tests. I'm aiming to have unit tests added for this by tomorrow. https://codereview.chromium.org/2526293003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentApp.java (right): https://codereview.chromium.org/2526293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentApp.java:11: import org.chromium.payments.mojom.PaymentAppManifest; On 2016/11/27 18:41:23, zino wrote: > Maybe we can not use mojom type here because the service worker data is saved in > browser side. > In other words, we don't need IPC for browser <--> browser. > So, we should probably define a new data type for it in java side. > IMHO, it's okay even if not exactly the same to PaymentAppManifest in IDL level. Done. https://codereview.chromium.org/2526293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentApp.java:79: methods.add(methodName); On 2016/11/27 18:41:23, zino wrote: > This method seems just getter that always returns the same value. > If so, we can move this logic to constructor and then just return the cached > value here. Done. https://codereview.chromium.org/2526293003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentInstrument.java (right): https://codereview.chromium.org/2526293003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentInstrument.java:33: private final PaymentAppManifest mManifest; On 2016/11/27 18:41:23, zino wrote: > It's difficult to imagine use-case of this variable in this class. > Could you let me know a example? I've removed this member variable. It was used in an earlier version of this code, and I forgot to remove it along with the code that used it.
looks good to me https://codereview.chromium.org/2526293003/diff/40001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2526293003/diff/40001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:175: "//components/payments:payment_app_java", nit: Not sure but this might be no longer necessary.
https://codereview.chromium.org/2526293003/diff/40001/chrome/android/BUILD.gn File chrome/android/BUILD.gn (right): https://codereview.chromium.org/2526293003/diff/40001/chrome/android/BUILD.gn... chrome/android/BUILD.gn:175: "//components/payments:payment_app_java", On 2016/11/28 14:48:08, zino wrote: > nit: Not sure but this might be no longer necessary. You're absolutely right. Thanks!
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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
just a couple nits from me. lgtm otherwise https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java (right): https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:76: Context context, WebContents webContents) { Here and in other places, you should try and say which type of context objects you expect. If you only want an Application context, then you should remove the parameter and use ContextUtils.getApplicationContext(). If you expect the contexts to have limited lifetimes (e.g. activities / services), then you should state so in order to avoid applications contexts being passed in. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java (right): https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java:22: public class Manifest { Should these inner classes be static?
See my comment on web-based payment apps and Android: http://crrev.com/2530793002
Let's call these ServiceWorker* instead of WebBased*. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java (right): https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:60: result.addAll(createWebBasedPaymentApps(context, webContents)); Put this behind a command-line flag before it's 100% implemented. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentApp.java (right): https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentApp.java:76: return mMethodNames; Collections.unmodifiableSet(). https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentApp.java:81: // TODO(tommyt): Implement this for Web Based Payment Apps. TODO should have a bug number. Please file a new bug, assign to yourself, and CC me. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java (right): https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java:23: public String id; Manifest does not have an ID field. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java:46: // Not yet implemented Add a TODO. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentInstrument.java (right): https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentInstrument.java:46: super(appId + "#" + option.id, option.label, null, option.icon); Add a comment next to null indicating the parameter name, like so: null /* paramName */ https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentInstrument.java:57: return mMethodNames; Collections.unmodifiableSet() https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentInstrument.java:64: // TODO(tommyt): Implement this for use with Web Based Payment Apps. TODO should have a bug link. File a bug, assign to yourself, CC me.
Description was changed from ========== PaymentApp: Add classes for supporting Web Based Payment Apps This adds an application class, an instrument class and a skeleton bridging class which can later be implemented to communicate with the service worker class in C++. The app factory class has been extended to create instances of the new web based payment apps in addition to the existing autofill payment app. BUG=661608 ========== to ========== PaymentApp: Add classes for supporting Web Based Payment Apps This adds an application class, an instrument class and a skeleton bridging class which can later be implemented to communicate with the service worker class in C++. The app factory class has been extended to create instances of the new web based payment apps in addition to the existing autofill payment app. BUG=669876 ==========
tommyt@opera.com changed reviewers: + dtrainor@chromium.org
Thanks agrieve and rouslan for your review comments. I've tried to address them all in the latest patchset. I have also added dtrainor as a reviewer for my change in: chrome/android/java/src/org/chromium/chrome/browser/ChromeSwitches.java What remains to be done (unless there are more review comments, of course) is to write some unit tests. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java (right): https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:60: result.addAll(createWebBasedPaymentApps(context, webContents)); On 2016/11/29 14:28:44, rouslan wrote: > Put this behind a command-line flag before it's 100% implemented. Done. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:76: Context context, WebContents webContents) { On 2016/11/28 17:59:53, agrieve wrote: > Here and in other places, you should try and say which type of context objects > you expect. If you only want an Application context, then you should remove the > parameter and use ContextUtils.getApplicationContext(). If you expect the > contexts to have limited lifetimes (e.g. activities / services), then you should > state so in order to avoid applications contexts being passed in. Yeah, this did actually turn out a bit cargo-cult-ish. I put those arguments in the constructor of my payment app class because the other types of payment app required these, and I figured I would need them sooner or later too, but that doesn't seem to be the case. I've removed these two parameters (context and webContents) from all functions I have introduced. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentApp.java (right): https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentApp.java:76: return mMethodNames; On 2016/11/29 14:28:44, rouslan wrote: > Collections.unmodifiableSet(). Done. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentApp.java:81: // TODO(tommyt): Implement this for Web Based Payment Apps. On 2016/11/29 14:28:44, rouslan wrote: > TODO should have a bug number. Please file a new bug, assign to yourself, and CC > me. Done. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java (right): https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java:22: public class Manifest { On 2016/11/28 17:59:53, agrieve wrote: > Should these inner classes be static? Done. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java:23: public String id; On 2016/11/29 14:28:44, rouslan wrote: > Manifest does not have an ID field. True, but the Payment App does have an ID, and I think we need to know this AppID in Chrome too. At the moment, I only use it to create a unique id for a ServiceWorkerPaymentInstrument, but I think we're also going to need to use this AppID to identify the app when we start actually invoking service workers. I felt this was a suitable place to put it. It would also be possible to keep it outside of this class, but then I would need to introduce another class that would keep track of this ID along with a reference to the manifest; public class AppEntry { public String id; public Manifest manifest; } This would be a bit more cumbersome, and I don't think it's that crucial that the Manifest class matches the W3C PaymentAppManifest definition exactly. What do you think? https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java:46: // Not yet implemented On 2016/11/29 14:28:44, rouslan wrote: > Add a TODO. Done. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentInstrument.java (right): https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentInstrument.java:46: super(appId + "#" + option.id, option.label, null, option.icon); On 2016/11/29 14:28:44, rouslan wrote: > Add a comment next to null indicating the parameter name, like so: > > null /* paramName */ Done. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentInstrument.java:57: return mMethodNames; On 2016/11/29 14:28:44, rouslan wrote: > Collections.unmodifiableSet() Done. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentInstrument.java:64: // TODO(tommyt): Implement this for use with Web Based Payment Apps. On 2016/11/29 14:28:44, rouslan wrote: > TODO should have a bug link. File a bug, assign to yourself, CC me. Done.
https://codereview.chromium.org/2526293003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java (right): https://codereview.chromium.org/2526293003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java:19: * This app class represents a web based payment app. nit: You probably should replace 'web based' with 'service-worker based' here and at all other places.
https://codereview.chromium.org/2526293003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java (right): https://codereview.chromium.org/2526293003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentApp.java:19: * This app class represents a web based payment app. On 2016/11/30 13:50:09, zino wrote: > nit: You probably should replace 'web based' with 'service-worker based' here > and at all other places. Done. I had tried, but apparently I wasn't thorough enough, on the first try :(
Sorry.. I left one more comment additionally. But you can ignore my comment if you and rouslan@ don't want it. Thank you. https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java (right): https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java:23: public String id; IMHO, you can rename the field to scopeUrl and then leave a comment for it.
https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java (right): https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java:23: public String id; On 2016/11/30 14:04:46, zino wrote: > IMHO, you can rename the field to scopeUrl and then leave a comment for it. This sounds like a good idea. Maybe this is better naming for this field. If rouslan agrees, I can change this to "scopeUrl"
https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java (right): https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java:23: public String id; On 2016/11/30 14:14:00, tommyt wrote: > On 2016/11/30 14:04:46, zino wrote: > > IMHO, you can rename the field to scopeUrl and then leave a comment for it. > > This sounds like a good idea. Maybe this is better naming for this field. If > rouslan agrees, I can change this to "scopeUrl" +1 https://codereview.chromium.org/2526293003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java (right): https://codereview.chromium.org/2526293003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:63: if (CommandLine.getInstance().hasSwitch( The new way to add command line flags is base::Feature. See https://codereview.chromium.org/2452243002/ for an example. There's no need to add an entry to chrome://flags at this time.
lgtm
tommyt@opera.com changed reviewers: + nick@chromium.org
Remaining review comments have been addressed and unit tests added in latest
patchset. PTAL
Also adding nick as reviewer for these files:
content/public/common/content_features.cc
content/public/common/content_features.h
https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java
(right):
https://codereview.chromium.org/2526293003/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/WebBasedPaymentAppBridge.java:23:
public String id;
On 2016/11/30 15:08:28, rouslan wrote:
> On 2016/11/30 14:14:00, tommyt wrote:
> > On 2016/11/30 14:04:46, zino wrote:
> > > IMHO, you can rename the field to scopeUrl and then leave a comment for
it.
> >
> > This sounds like a good idea. Maybe this is better naming for this field. If
> > rouslan agrees, I can change this to "scopeUrl"
>
> +1
Done.
https://codereview.chromium.org/2526293003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java
(right):
https://codereview.chromium.org/2526293003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:63:
if (CommandLine.getInstance().hasSwitch(
On 2016/11/30 15:08:28, rouslan wrote:
> The new way to add command line flags is base::Feature. See
> https://codereview.chromium.org/2452243002/ for an example. There's no need to
> add an entry to chrome://flags at this time.
Done.
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
LGTM % nits Good job! https://codereview.chromium.org/2526293003/diff/170001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java (right): https://codereview.chromium.org/2526293003/diff/170001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:95: // service worker based payment app functionality. nit: remove indent. https://codereview.chromium.org/2526293003/diff/170001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:105: // service worker based payment app functionality. nit: remove indent.
lgtm
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...)
lgtm
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tommyt@opera.com to run a CQ dry run
https://codereview.chromium.org/2526293003/diff/170001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java (right): https://codereview.chromium.org/2526293003/diff/170001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:95: // service worker based payment app functionality. On 2016/12/01 14:45:23, rouslan wrote: > nit: remove indent. Done. https://codereview.chromium.org/2526293003/diff/170001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:105: // service worker based payment app functionality. On 2016/12/01 14:45:23, rouslan wrote: > nit: remove indent. Done.
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 tommyt@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org, dtrainor@chromium.org, nick@chromium.org, rouslan@chromium.org, jinho.bang@samsung.com Link to the patchset: https://codereview.chromium.org/2526293003/#ps230001 (title: "Make the service worker unittests work even with the feature flag off")
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": 230001, "attempt_start_ts": 1480943516606590,
"parent_rev": "dedf73f1219a9e087f91dc85a18ad88c3d94490b", "commit_rev":
"f07e0957b43e2202b68c7abf22d664a90b59c826"}
Message was sent while issue was closed.
Description was changed from ========== PaymentApp: Add classes for supporting Web Based Payment Apps This adds an application class, an instrument class and a skeleton bridging class which can later be implemented to communicate with the service worker class in C++. The app factory class has been extended to create instances of the new web based payment apps in addition to the existing autofill payment app. BUG=669876 ========== to ========== PaymentApp: Add classes for supporting Web Based Payment Apps This adds an application class, an instrument class and a skeleton bridging class which can later be implemented to communicate with the service worker class in C++. The app factory class has been extended to create instances of the new web based payment apps in addition to the existing autofill payment app. BUG=669876 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:230001)
Message was sent while issue was closed.
Description was changed from ========== PaymentApp: Add classes for supporting Web Based Payment Apps This adds an application class, an instrument class and a skeleton bridging class which can later be implemented to communicate with the service worker class in C++. The app factory class has been extended to create instances of the new web based payment apps in addition to the existing autofill payment app. BUG=669876 ========== to ========== PaymentApp: Add classes for supporting Web Based Payment Apps This adds an application class, an instrument class and a skeleton bridging class which can later be implemented to communicate with the service worker class in C++. The app factory class has been extended to create instances of the new web based payment apps in addition to the existing autofill payment app. BUG=669876 Committed: https://crrev.com/857f8a9174628615f9775f9383e0a64846d29d55 Cr-Commit-Position: refs/heads/master@{#436274} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/857f8a9174628615f9775f9383e0a64846d29d55 Cr-Commit-Position: refs/heads/master@{#436274} |
