Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2559153002:
PaymentApp: Make the PaymentAppFactory asynchronous (Closed)
|
Created:
4 years ago by tommyt Modified:
4 years ago Reviewers:
please use gerrit instead CC:
chromium-reviews, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org, agrieve+watch_chromium.org, Rob Buis, pals, Ted C Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPaymentApp: Make the PaymentAppFactory asynchronous
The code for fetching and filtering payment instruments in
PaymentRequestImpl is asynchronous anyway, so this change is not too
intrusive. The main thing is to insert an extra asynchronous step to
populate the mApps list before payment instrument filtering starts.
If we want to take this further, a good next step would be to start
showing apps as they are discovered, instead of waiting until we have
received all the payment apps. It would also be a good thing to
refactor some of this functionality out of the PaymentRequestImpl, as
it is growing quite complex.
Other changes that went into this commit:
* Change the PaymentAppFactory into a singleton, rather than being a
holder class for static functions.
* Extend the AdditionalPaymentFactory functionality, so that the
PaymentAppFactory can have many additional factories. This lets us
make the ServiceWorkerPaymentAppBridge an additional factory, and
normalize the relationship between the two classes.
* Add two unit tests for testing delayed payment app creation.
BUG=661608
Committed: https://crrev.com/b1a60e9fbe21734a3567c7cf10f9eea6570f9655
Cr-Commit-Position: refs/heads/master@{#437843}
Patch Set 1 #
Total comments: 34
Patch Set 2 : Address review comments #
Total comments: 2
Patch Set 3 : Fix native initialization and remove the workaround from PaymentAppFactory #Patch Set 4 : Get rid of mWebContents #
Total comments: 2
Patch Set 5 : Address final review comment #Patch Set 6 : Rebase onto latest master #Dependent Patchsets: Messages
Total messages: 32 (19 generated)
tommyt@opera.com changed reviewers: + rouslan@chromium.org
PTAL
Great job! Thank you for looking into this. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java (right): https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:29: private List<PaymentAppFactoryAddition> mAdditionalFactories; final https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:36: void onAllPaymentAppsCreated(); javadoc for these two methods, please. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:47: * @param webContents The web contents that invoked PaymentRequest. @param callback The callback to invoke when apps are created. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:55: // Don't call ChromeFeatureList.isEnabled() while running unit tests. Why not? https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:64: * Returns the singleton PaymentAppFactory instance. @return The singleton PaymentAppFactory instance. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:67: if (sInstance == null) { Android style guide recommends to collapse single-line if statements, if possible: if (sInstance == null) sInstance = new PaymentAppFactory(); https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:92: * @param webContents The web contents where PaymentRequest was invoked. @param callback The callback to invoke when apps are created. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:106: for (final PaymentAppFactoryAddition additionalFactory : mAdditionalFactories) { Use this type of loop on Android: for (int i = 0; i < mAdditionalFactories.size(); i++) { final PaymentAppFactoryAddition additionalFactory = mAdditionalFactories.get(i); } It does not allocate additional objects. For-each loops allocate iterators. Object allocation is expensive on Android. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:116: if (mPendingTasks.isEmpty()) { Does line length allow you to collapse this into a single line? Android style guide recommends this. Chromium follows Android style guide for Java. if (mPendingTasks.isEmpty()) callback.onAllPaymentAppsCreated(); https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:279: mWebContents = webContents; You don't need to save webContents as a member variable if you call PaymentAppFactory.getInstance().create() from the constructor. Is there a good reason to move this call to PaymentRequestImpl.init()? https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java (right): https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:58: * This method is protected so that it can be overridden by tests. @return The installed service worker app manifests. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:59: */ @VisibleForTesting https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:68: for (Manifest manifest : getAllAppManifests()) { Use "for (int i = 0..." type loop. https://codereview.chromium.org/2559153002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2559153002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:764: * @param creationSpeed How quickly the app factory will create this instrument. Either "factory will create this app", right? https://codereview.chromium.org/2559153002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:786: }, 1000); 100 is enough to trigger the code path that you want to test. Using 1000 has the risk of timing out a test.
Description was changed from ========== PaymentApp: Make the PaymentAppFactory asynchronous The code for fetching and filtering payment instruments in PaymentRequestImpl is asynchronous anyway, so this change is not too intrusive. The main thing is to insert an extra asynchronous step in the beginning of getMatchingPaymentInstruments that populate the mApps list. If we want to take this further, a good next step would be start showing apps as they are discovered, instead of waiting until we have received all the payment apps. It would also be a good thing to refactor some of this functionality out of the PaymentRequestImpl, as it is growing quite complex. Other changes that went into this commit: * Change the PaymentAppFactory into a singleton, rather than being a holder class for static functions. * Extend the AdditionalPaymentFactory functionality, so that the PaymentAppFactory can have many additional factories. This lets us make the ServiceWorkerPaymentAppBridge an additional factory, and normalize the relationship between the two classes. * Add two unit tests for testing delayed payment app creation. BUG=661608 ========== to ========== PaymentApp: Make the PaymentAppFactory asynchronous The code for fetching and filtering payment instruments in PaymentRequestImpl is asynchronous anyway, so this change is not too intrusive. The main thing is to insert an extra asynchronous step to populate the mApps list before payment instrument filtering starts. If we want to take this further, a good next step would be start showing apps as they are discovered, instead of waiting until we have received all the payment apps. It would also be a good thing to refactor some of this functionality out of the PaymentRequestImpl, as it is growing quite complex. Other changes that went into this commit: * Change the PaymentAppFactory into a singleton, rather than being a holder class for static functions. * Extend the AdditionalPaymentFactory functionality, so that the PaymentAppFactory can have many additional factories. This lets us make the ServiceWorkerPaymentAppBridge an additional factory, and normalize the relationship between the two classes. * Add two unit tests for testing delayed payment app creation. BUG=661608 ==========
Description was changed from ========== PaymentApp: Make the PaymentAppFactory asynchronous The code for fetching and filtering payment instruments in PaymentRequestImpl is asynchronous anyway, so this change is not too intrusive. The main thing is to insert an extra asynchronous step to populate the mApps list before payment instrument filtering starts. If we want to take this further, a good next step would be start showing apps as they are discovered, instead of waiting until we have received all the payment apps. It would also be a good thing to refactor some of this functionality out of the PaymentRequestImpl, as it is growing quite complex. Other changes that went into this commit: * Change the PaymentAppFactory into a singleton, rather than being a holder class for static functions. * Extend the AdditionalPaymentFactory functionality, so that the PaymentAppFactory can have many additional factories. This lets us make the ServiceWorkerPaymentAppBridge an additional factory, and normalize the relationship between the two classes. * Add two unit tests for testing delayed payment app creation. BUG=661608 ========== to ========== PaymentApp: Make the PaymentAppFactory asynchronous The code for fetching and filtering payment instruments in PaymentRequestImpl is asynchronous anyway, so this change is not too intrusive. The main thing is to insert an extra asynchronous step to populate the mApps list before payment instrument filtering starts. If we want to take this further, a good next step would be to start showing apps as they are discovered, instead of waiting until we have received all the payment apps. It would also be a good thing to refactor some of this functionality out of the PaymentRequestImpl, as it is growing quite complex. Other changes that went into this commit: * Change the PaymentAppFactory into a singleton, rather than being a holder class for static functions. * Extend the AdditionalPaymentFactory functionality, so that the PaymentAppFactory can have many additional factories. This lets us make the ServiceWorkerPaymentAppBridge an additional factory, and normalize the relationship between the two classes. * Add two unit tests for testing delayed payment app creation. BUG=661608 ==========
https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java (right): https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:29: private List<PaymentAppFactoryAddition> mAdditionalFactories; On 2016/12/08 13:45:56, rouslan wrote: > final Done. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:36: void onAllPaymentAppsCreated(); On 2016/12/08 13:45:56, rouslan wrote: > javadoc for these two methods, please. Done. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:47: * @param webContents The web contents that invoked PaymentRequest. On 2016/12/08 13:45:56, rouslan wrote: > @param callback The callback to invoke when apps are created. Done. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:55: // Don't call ChromeFeatureList.isEnabled() while running unit tests. On 2016/12/08 13:45:56, rouslan wrote: > Why not? Because when I do, I get this exception: java.lang.UnsatisfiedLinkError: No implementation found for boolean org.chromium.chrome.browser.ChromeFeatureList.nativeIsEnabled(java.lang.String) (tried Java_org_chromium_chrome_browser_ChromeFeatureList_nativeIsEnabled and Java_org_chromium_chrome_browser_ChromeFeatureList_nativeIsEnabled__Ljava_lang_String_2) at org.chromium.chrome.browser.ChromeFeatureList.nativeIsEnabled(Native Method) at org.chromium.chrome.browser.ChromeFeatureList.isEnabled(ChromeFeatureList.java:47) at org.chromium.chrome.browser.payments.PaymentAppFactory.<init>(PaymentAppFactory.java:66) at org.chromium.chrome.browser.payments.PaymentAppFactory.getInstance(PaymentAppFactory.java:76) at org.chromium.chrome.browser.payments.PaymentRequestServiceWorkerPaymentAppTest.installServiceWorkerPaymentApp(PaymentRequestServiceWorkerPaymentAppTest.java:41) at org.chromium.chrome.browser.payments.PaymentRequestServiceWorkerPaymentAppTest.testOneOption(PaymentRequestServiceWorkerPaymentAppTest.java:93) at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) at org.chromium.chrome.test.ChromeActivityTestCaseBase.runTest(ChromeActivityTestCaseBase.java:726) at org.chromium.base.test.BaseTestResult.runParameterized(BaseTestResult.java:161) at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:124) at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555) at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1959) Is this a known problem? Is there also a known solution for this? It would actually be nice to not have the whole "sIsTesting" kludge, https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:64: * Returns the singleton PaymentAppFactory instance. On 2016/12/08 13:45:56, rouslan wrote: > @return The singleton PaymentAppFactory instance. Done. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:67: if (sInstance == null) { On 2016/12/08 13:45:56, rouslan wrote: > Android style guide recommends to collapse single-line if statements, if > possible: > > if (sInstance == null) sInstance = new PaymentAppFactory(); Done. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:92: * @param webContents The web contents where PaymentRequest was invoked. On 2016/12/08 13:45:56, rouslan wrote: > @param callback The callback to invoke when apps are created. Done. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:106: for (final PaymentAppFactoryAddition additionalFactory : mAdditionalFactories) { On 2016/12/08 13:45:56, rouslan wrote: > Use this type of loop on Android: > > for (int i = 0; i < mAdditionalFactories.size(); i++) { > final PaymentAppFactoryAddition additionalFactory = > mAdditionalFactories.get(i); > } > > It does not allocate additional objects. For-each loops allocate iterators. > Object allocation is expensive on Android. Done. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:116: if (mPendingTasks.isEmpty()) { On 2016/12/08 13:45:56, rouslan wrote: > Does line length allow you to collapse this into a single line? Android style > guide recommends this. Chromium follows Android style guide for Java. > > if (mPendingTasks.isEmpty()) callback.onAllPaymentAppsCreated(); Yes. Done. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:279: mWebContents = webContents; On 2016/12/08 13:45:56, rouslan wrote: > You don't need to save webContents as a member variable if you call > PaymentAppFactory.getInstance().create() from the constructor. Is there a good > reason to move this call to PaymentRequestImpl.init()? I tried moving the create() call to the constructor, and I got a lot of unit test failures. Presumably because some of the code in onAllPaymentAppsCreated or onInstrumentsReady does not work so well if run before init(). I can probably figure out exactly why this doesn't work, and how to fix it tomorrow. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java (right): https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:58: * This method is protected so that it can be overridden by tests. On 2016/12/08 13:45:56, rouslan wrote: > @return The installed service worker app manifests. Done. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:59: */ On 2016/12/08 13:45:57, rouslan wrote: > @VisibleForTesting Done. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:68: for (Manifest manifest : getAllAppManifests()) { On 2016/12/08 13:45:56, rouslan wrote: > Use "for (int i = 0..." type loop. Done. https://codereview.chromium.org/2559153002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2559153002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:764: * @param creationSpeed How quickly the app factory will create this instrument. Either On 2016/12/08 13:45:57, rouslan wrote: > "factory will create this app", right? Right, thanks! https://codereview.chromium.org/2559153002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:786: }, 1000); On 2016/12/08 13:45:57, rouslan wrote: > 100 is enough to trigger the code path that you want to test. Using 1000 has the > risk of timing out a test. Done.
rouslan@chromium.org changed reviewers: + tedchoc@chromium.org
Ted, is this UnsatisifedLinkError a known problem? Does not seem right. https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java (right): https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:55: // Don't call ChromeFeatureList.isEnabled() while running unit tests. On 2016/12/08 15:06:10, tommyt wrote: > On 2016/12/08 13:45:56, rouslan wrote: > > Why not? > > Because when I do, I get this exception: > > java.lang.UnsatisfiedLinkError: No implementation found for boolean > org.chromium.chrome.browser.ChromeFeatureList.nativeIsEnabled(java.lang.String) > (tried Java_org_chromium_chrome_browser_ChromeFeatureList_nativeIsEnabled and > Java_org_chromium_chrome_browser_ChromeFeatureList_nativeIsEnabled__Ljava_lang_String_2) > at org.chromium.chrome.browser.ChromeFeatureList.nativeIsEnabled(Native Method) > at > org.chromium.chrome.browser.ChromeFeatureList.isEnabled(ChromeFeatureList.java:47) > at > org.chromium.chrome.browser.payments.PaymentAppFactory.<init>(PaymentAppFactory.java:66) > at > org.chromium.chrome.browser.payments.PaymentAppFactory.getInstance(PaymentAppFactory.java:76) > at > org.chromium.chrome.browser.payments.PaymentRequestServiceWorkerPaymentAppTest.installServiceWorkerPaymentApp(PaymentRequestServiceWorkerPaymentAppTest.java:41) > at > org.chromium.chrome.browser.payments.PaymentRequestServiceWorkerPaymentAppTest.testOneOption(PaymentRequestServiceWorkerPaymentAppTest.java:93) > at > android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) > at > android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) > at > android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) > at > org.chromium.chrome.test.ChromeActivityTestCaseBase.runTest(ChromeActivityTestCaseBase.java:726) > at > org.chromium.base.test.BaseTestResult.runParameterized(BaseTestResult.java:161) > at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:124) > at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) > at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) > at > android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555) > at > android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1959) > > Is this a known problem? Is there also a known solution for this? It would > actually be nice to not have the whole "sIsTesting" kludge, That looks like a regression. Ted, are you aware of this happenning?
Description was changed from ========== PaymentApp: Make the PaymentAppFactory asynchronous The code for fetching and filtering payment instruments in PaymentRequestImpl is asynchronous anyway, so this change is not too intrusive. The main thing is to insert an extra asynchronous step to populate the mApps list before payment instrument filtering starts. If we want to take this further, a good next step would be to start showing apps as they are discovered, instead of waiting until we have received all the payment apps. It would also be a good thing to refactor some of this functionality out of the PaymentRequestImpl, as it is growing quite complex. Other changes that went into this commit: * Change the PaymentAppFactory into a singleton, rather than being a holder class for static functions. * Extend the AdditionalPaymentFactory functionality, so that the PaymentAppFactory can have many additional factories. This lets us make the ServiceWorkerPaymentAppBridge an additional factory, and normalize the relationship between the two classes. * Add two unit tests for testing delayed payment app creation. BUG=661608 ========== to ========== PaymentApp: Make the PaymentAppFactory asynchronous The code for fetching and filtering payment instruments in PaymentRequestImpl is asynchronous anyway, so this change is not too intrusive. The main thing is to insert an extra asynchronous step to populate the mApps list before payment instrument filtering starts. If we want to take this further, a good next step would be to start showing apps as they are discovered, instead of waiting until we have received all the payment apps. It would also be a good thing to refactor some of this functionality out of the PaymentRequestImpl, as it is growing quite complex. Other changes that went into this commit: * Change the PaymentAppFactory into a singleton, rather than being a holder class for static functions. * Extend the AdditionalPaymentFactory functionality, so that the PaymentAppFactory can have many additional factories. This lets us make the ServiceWorkerPaymentAppBridge an additional factory, and normalize the relationship between the two classes. * Add two unit tests for testing delayed payment app creation. BUG=661608 ==========
https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java (right): https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:55: // Don't call ChromeFeatureList.isEnabled() while running unit tests. On 2016/12/08 15:24:54, rouslan wrote: > On 2016/12/08 15:06:10, tommyt wrote: > > On 2016/12/08 13:45:56, rouslan wrote: > > > Why not? > > > > Because when I do, I get this exception: > > > > java.lang.UnsatisfiedLinkError: No implementation found for boolean > > > org.chromium.chrome.browser.ChromeFeatureList.nativeIsEnabled(java.lang.String) > > (tried Java_org_chromium_chrome_browser_ChromeFeatureList_nativeIsEnabled and > > > Java_org_chromium_chrome_browser_ChromeFeatureList_nativeIsEnabled__Ljava_lang_String_2) > > at org.chromium.chrome.browser.ChromeFeatureList.nativeIsEnabled(Native > Method) > > at > > > org.chromium.chrome.browser.ChromeFeatureList.isEnabled(ChromeFeatureList.java:47) > > at > > > org.chromium.chrome.browser.payments.PaymentAppFactory.<init>(PaymentAppFactory.java:66) > > at > > > org.chromium.chrome.browser.payments.PaymentAppFactory.getInstance(PaymentAppFactory.java:76) > > at > > > org.chromium.chrome.browser.payments.PaymentRequestServiceWorkerPaymentAppTest.installServiceWorkerPaymentApp(PaymentRequestServiceWorkerPaymentAppTest.java:41) > > at > > > org.chromium.chrome.browser.payments.PaymentRequestServiceWorkerPaymentAppTest.testOneOption(PaymentRequestServiceWorkerPaymentAppTest.java:93) > > at > > > android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) > > at > > android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) > > at > > > android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) > > at > > > org.chromium.chrome.test.ChromeActivityTestCaseBase.runTest(ChromeActivityTestCaseBase.java:726) > > at > > > org.chromium.base.test.BaseTestResult.runParameterized(BaseTestResult.java:161) > > at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:124) > > at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) > > at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) > > at > > > android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555) > > at > > > android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1959) > > > > Is this a known problem? Is there also a known solution for this? It would > > actually be nice to not have the whole "sIsTesting" kludge, > > That looks like a regression. Ted, are you aware of this happenning? That looks like you haven't initialized native. Is this an activity test case? Has it started yet? If no to either, then you need to explicitly init native first.
On 2016/12/08 15:43:17, Ted C (OOO 12.8.16) wrote: > That looks like you haven't initialized native. Is this an activity test case? > Has it started yet? If no to either, then you need to explicitly init native > first. Good to know there's a cure. We're using ChromeActivityTestCaseBase<ChromeTabbedActivity> and call into native in many other places in PersonalDataManager.java. Tommy: Please check that our initialization code is correct: https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... I'm afraid it might have a race condition or is missing a call to initialize native.
https://codereview.chromium.org/2559153002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java (right): https://codereview.chromium.org/2559153002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:84: openPageAndClickBuyAndWait(mShowFailed); I see what's going on. Line 83 needs native to be already initialized, but line 84 starts the activity. This should be split into 3 steps instead: 1) Start activity - openPage(). 2) Install service worker payment app - installServiceWorkerPaymentApp(NO_OPTIONS). 3) Invoke PaymentRequest - clickBuyAndWait(mShowFailed).
rouslan@chromium.org changed reviewers: - tedchoc@chromium.org
https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java (right): https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java:55: // Don't call ChromeFeatureList.isEnabled() while running unit tests. On 2016/12/08 15:43:17, Ted C (OOO 12.8.16) wrote: > On 2016/12/08 15:24:54, rouslan wrote: > > On 2016/12/08 15:06:10, tommyt wrote: > > > On 2016/12/08 13:45:56, rouslan wrote: > > > > Why not? > > > > > > Because when I do, I get this exception: > > > > > > java.lang.UnsatisfiedLinkError: No implementation found for boolean > > > > > > org.chromium.chrome.browser.ChromeFeatureList.nativeIsEnabled(java.lang.String) > > > (tried Java_org_chromium_chrome_browser_ChromeFeatureList_nativeIsEnabled > and > > > > > > Java_org_chromium_chrome_browser_ChromeFeatureList_nativeIsEnabled__Ljava_lang_String_2) > > > at org.chromium.chrome.browser.ChromeFeatureList.nativeIsEnabled(Native > > Method) > > > at > > > > > > org.chromium.chrome.browser.ChromeFeatureList.isEnabled(ChromeFeatureList.java:47) > > > at > > > > > > org.chromium.chrome.browser.payments.PaymentAppFactory.<init>(PaymentAppFactory.java:66) > > > at > > > > > > org.chromium.chrome.browser.payments.PaymentAppFactory.getInstance(PaymentAppFactory.java:76) > > > at > > > > > > org.chromium.chrome.browser.payments.PaymentRequestServiceWorkerPaymentAppTest.installServiceWorkerPaymentApp(PaymentRequestServiceWorkerPaymentAppTest.java:41) > > > at > > > > > > org.chromium.chrome.browser.payments.PaymentRequestServiceWorkerPaymentAppTest.testOneOption(PaymentRequestServiceWorkerPaymentAppTest.java:93) > > > at > > > > > > android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) > > > at > > > > android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) > > > at > > > > > > android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) > > > at > > > > > > org.chromium.chrome.test.ChromeActivityTestCaseBase.runTest(ChromeActivityTestCaseBase.java:726) > > > at > > > > > > org.chromium.base.test.BaseTestResult.runParameterized(BaseTestResult.java:161) > > > at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:124) > > > at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) > > > at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) > > > at > > > > > > android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:555) > > > at > > > > > > android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1959) > > > > > > Is this a known problem? Is there also a known solution for this? It would > > > actually be nice to not have the whole "sIsTesting" kludge, > > > > That looks like a regression. Ted, are you aware of this happenning? > > That looks like you haven't initialized native. Is this an activity test case? > Has it started yet? If no to either, then you need to explicitly init native > first. That's it! Thanks Ted! That was the hint I needed to solve this (by starting the activity from "startMainActivity()"). https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2559153002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:279: mWebContents = webContents; On 2016/12/08 15:06:10, tommyt wrote: > On 2016/12/08 13:45:56, rouslan wrote: > > You don't need to save webContents as a member variable if you call > > PaymentAppFactory.getInstance().create() from the constructor. Is there a good > > reason to move this call to PaymentRequestImpl.init()? > > I tried moving the create() call to the constructor, and I got a lot of unit > test failures. Presumably because some of the code in onAllPaymentAppsCreated or > onInstrumentsReady does not work so well if run before init(). I can probably > figure out exactly why this doesn't work, and how to fix it tomorrow. I found a way. This is sorted out in the latest patchset. https://codereview.chromium.org/2559153002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java (right): https://codereview.chromium.org/2559153002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java:84: openPageAndClickBuyAndWait(mShowFailed); On 2016/12/08 16:13:48, rouslan wrote: > I see what's going on. Line 83 needs native to be already initialized, but line > 84 starts the activity. This should be split into 3 steps instead: > > 1) Start activity - openPage(). > 2) Install service worker payment app - > installServiceWorkerPaymentApp(NO_OPTIONS). > 3) Invoke PaymentRequest - clickBuyAndWait(mShowFailed). This approach does work, but the error I showed you actually happens in every single payments test case, not just the ServiceWorkerPaymentApp ones. I've uploaded a patchset where I fix this in a different way; by moving the call to "startMainActivityWithURL()" to the startMainActivity() method. This seems to work pretty well. At least now all the unit tests pass without my PaymentAppFactory.setIsTesting workaround.
lgtm % comment https://codereview.chromium.org/2559153002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2559153002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:525: } No need to separate this into its own method. It's used only once in the constructor. If you put the body of this function directly into the constructor, then you will save 4 lines of code. Shorter code is easier to read, so it's easier to maintain.
https://codereview.chromium.org/2559153002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2559153002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:525: } On 2016/12/09 13:58:27, rouslan wrote: > No need to separate this into its own method. It's used only once in the > constructor. If you put the body of this function directly into the constructor, > then you will save 4 lines of code. Shorter code is easier to read, so it's > easier to maintain. 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_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...
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 rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2559153002/#ps100001 (title: "Rebase onto latest master")
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": 100001, "attempt_start_ts": 1481538992171940,
"parent_rev": "9ea52fc75eb0c3f15340efd62af91c3d25b830b1", "commit_rev":
"15611f1d6b79a3a49f9db0f9f310d67fb7427295"}
Message was sent while issue was closed.
Description was changed from ========== PaymentApp: Make the PaymentAppFactory asynchronous The code for fetching and filtering payment instruments in PaymentRequestImpl is asynchronous anyway, so this change is not too intrusive. The main thing is to insert an extra asynchronous step to populate the mApps list before payment instrument filtering starts. If we want to take this further, a good next step would be to start showing apps as they are discovered, instead of waiting until we have received all the payment apps. It would also be a good thing to refactor some of this functionality out of the PaymentRequestImpl, as it is growing quite complex. Other changes that went into this commit: * Change the PaymentAppFactory into a singleton, rather than being a holder class for static functions. * Extend the AdditionalPaymentFactory functionality, so that the PaymentAppFactory can have many additional factories. This lets us make the ServiceWorkerPaymentAppBridge an additional factory, and normalize the relationship between the two classes. * Add two unit tests for testing delayed payment app creation. BUG=661608 ========== to ========== PaymentApp: Make the PaymentAppFactory asynchronous The code for fetching and filtering payment instruments in PaymentRequestImpl is asynchronous anyway, so this change is not too intrusive. The main thing is to insert an extra asynchronous step to populate the mApps list before payment instrument filtering starts. If we want to take this further, a good next step would be to start showing apps as they are discovered, instead of waiting until we have received all the payment apps. It would also be a good thing to refactor some of this functionality out of the PaymentRequestImpl, as it is growing quite complex. Other changes that went into this commit: * Change the PaymentAppFactory into a singleton, rather than being a holder class for static functions. * Extend the AdditionalPaymentFactory functionality, so that the PaymentAppFactory can have many additional factories. This lets us make the ServiceWorkerPaymentAppBridge an additional factory, and normalize the relationship between the two classes. * Add two unit tests for testing delayed payment app creation. BUG=661608 Review-Url: https://codereview.chromium.org/2559153002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== PaymentApp: Make the PaymentAppFactory asynchronous The code for fetching and filtering payment instruments in PaymentRequestImpl is asynchronous anyway, so this change is not too intrusive. The main thing is to insert an extra asynchronous step to populate the mApps list before payment instrument filtering starts. If we want to take this further, a good next step would be to start showing apps as they are discovered, instead of waiting until we have received all the payment apps. It would also be a good thing to refactor some of this functionality out of the PaymentRequestImpl, as it is growing quite complex. Other changes that went into this commit: * Change the PaymentAppFactory into a singleton, rather than being a holder class for static functions. * Extend the AdditionalPaymentFactory functionality, so that the PaymentAppFactory can have many additional factories. This lets us make the ServiceWorkerPaymentAppBridge an additional factory, and normalize the relationship between the two classes. * Add two unit tests for testing delayed payment app creation. BUG=661608 Review-Url: https://codereview.chromium.org/2559153002 ========== to ========== PaymentApp: Make the PaymentAppFactory asynchronous The code for fetching and filtering payment instruments in PaymentRequestImpl is asynchronous anyway, so this change is not too intrusive. The main thing is to insert an extra asynchronous step to populate the mApps list before payment instrument filtering starts. If we want to take this further, a good next step would be to start showing apps as they are discovered, instead of waiting until we have received all the payment apps. It would also be a good thing to refactor some of this functionality out of the PaymentRequestImpl, as it is growing quite complex. Other changes that went into this commit: * Change the PaymentAppFactory into a singleton, rather than being a holder class for static functions. * Extend the AdditionalPaymentFactory functionality, so that the PaymentAppFactory can have many additional factories. This lets us make the ServiceWorkerPaymentAppBridge an additional factory, and normalize the relationship between the two classes. * Add two unit tests for testing delayed payment app creation. BUG=661608 Committed: https://crrev.com/b1a60e9fbe21734a3567c7cf10f9eea6570f9655 Cr-Commit-Position: refs/heads/master@{#437843} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b1a60e9fbe21734a3567c7cf10f9eea6570f9655 Cr-Commit-Position: refs/heads/master@{#437843} |
