Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2126213002:
[Payments] Record use of profiles and credit cards in Payment Request. (Closed)
|
Created:
4 years, 5 months ago by sebsg Modified:
4 years, 5 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, estade+watch_chromium.org, vabr+watchlistautofill_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRecord and log the use of a profile and/or credit card when they get used in in Payment Request. Calls the RecordAndLogUse() method that is currently used in Autofill.
BUG=625643
TEST=PersonalDataManagerTest, PaymentRequestUseStatsTest
Committed: https://crrev.com/0b85d558587cb6195338161ca6ebcf77f6b7166d
Cr-Commit-Position: refs/heads/master@{#406692}
Patch Set 1 #
Total comments: 64
Patch Set 2 : Addressed comments #
Total comments: 8
Patch Set 3 : Removed newlines #
Total comments: 17
Patch Set 4 : Addressed comments #
Total comments: 2
Patch Set 5 : Rebase #Patch Set 6 : Fixed temp card bug + nits #Messages
Total messages: 39 (23 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== [Payments] Record use of profiles and credit cards in Payment Request. Record and log the use of a profile and/or credit card when it is used in in Payment Request. Calls the RecordAndLogUse() method that is currently used in Autofill. BUG=625643 TEST=PersonalDataManagerTest ========== to ========== Record and log the use of a profile and/or credit card when they get used in in Payment Request. Calls the RecordAndLogUse() method that is currently used in Autofill. BUG=625643 TEST=PersonalDataManagerTest, PaymentRequestUseStatsTest ==========
Patchset #1 (id:60001) has been deleted
sebsg@chromium.org changed reviewers: + mathp@chromium.org, rouslan@chromium.org
sebsg@chromium.org changed reviewers: - mathp@chromium.org
Hi Rouslan, could you please take a look?
Patchset #1 (id:80001) has been deleted
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Hi Math could you please take a look at personal_data_manager_android.* files? Thanks!
personal_data_manager lgtm, but haven't looked at the rest. Sorry for the delay.
Good patch. Mostly minor comments about comments. (We must go deeper.) https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:641: * the use count of the profile and set its use date to the current time. Also logs usage s/set/sets/ https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:643: * @param guid The GUID of the profile. newline before @param https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:657: protected int getProfileUseCountForTesting(String guid) { Remove "protected". This will make the access of this method "package". AutofillTestHelper is in the same package "org.chromium.chrome.browser.autofill", which will allow AutofillTestHelper to access these methods. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:663: protected long getProfileUseDateForTesting(String guid) { Ditto https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:672: * @param guid The GUID of the credit card. newline before @param https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:686: protected int getCreditCardUseCountForTesting(String guid) { Remove "protected" https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:692: protected long getCreditCardUseDateForTesting(String guid) { ditto https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:698: protected long getCurrentDateForTesting() { ditto https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:65: public String getCardGUID() { This information is already returned from getIdentifier(). Use that instead. Remove this method. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:827: ((AutofillPaymentInstrument) mPaymentMethodsSection.getSelectedItem()) Remove the cast to AutofillPaymentInstrument on this line. getIdentifier() is defined in PaymentOption.java, so there's no need to cast it. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:828: .getCardGUID()); getIdentifier() https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:157: * @param guid The GUID of the profile. newline before @param https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:191: * @param guid The GUID of the profile to modify. 1) newline before @param 2) s/to modify/to query/ https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:192: * @return count The use count to assign to the profile. It should be non-negative. 1) Remove "count". @return The use count to assign... 2) Remove "It should be". This phrasing is reserved for inputs. You can tell the user of your API that they should pass in non-negative numbers. Usually this is enforced with an assert inside of your method. In contrast, you're returning a value here. You are in control of it. Provide a guarantee: @return The non-negative use count to assign... https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:206: * @param guid The GUID of the profile to modify. 1) newline before @param 2) s/to modify/to query/ https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:207: * @return date The use date to assign to the profile. It represents an absolute point in Remove "date" after "@return". https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:224: * increments the use count of the credit card and set its use date to the current time. Also s/set/sets/ https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:226: * @param guid The GUID of the credit card. newline before @param https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:242: * @return date The use date to assign to the credit card. It represents an absolute point in Remove "date" after "@return". https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:261: * @param guid The GUID of the credit card to modify. 1) newline before @param 2) s/modify/query/ https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:262: * @return count The use count to assign to the credit card. It should be non-negative. 1) Remove "count". 2) Remove "It should be". https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:276: * @param guid The GUID of the credit card to modify. Ditto https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:294: * @return date The current use date. It represents an absolute point in coordinated universal Ditto https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:501: if (!profile) Early returns are only useful when the rest of the function is huge. Here, the rest of the function is a single line. So you can save 2 lines of code by doing this instead: if (profile) profile->RecordAndLogUse(); https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:530: if (!profile) Ternary operator saves space for very short lines. return profile ? profile->use_count() : -1; https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:542: if (!profile) Ditto https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:554: if (!card) Ditto https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:583: if (!card) Ditto https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:595: if (!card) Ditto https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:144: // Record the use and log usage metrics for the profile associated with the Records https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:147: void RecordAndLogProfileUse( Line 78 above says "These functions act on local credit cards." You may want to place profile functions above that line or create your own section for usage stats only. https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:179: // Record the use and log usage metrics for the credit card associated with Records
Patchset #2 (id:120001) has been deleted
Thanks for the comments. Another look? Thanks! https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:641: * the use count of the profile and set its use date to the current time. Also logs usage On 2016/07/12 15:41:57, Rouslan (ツ) wrote: > s/set/sets/ Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:643: * @param guid The GUID of the profile. On 2016/07/12 15:41:57, Rouslan (ツ) wrote: > newline before @param Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:657: protected int getProfileUseCountForTesting(String guid) { On 2016/07/12 15:41:57, Rouslan (ツ) wrote: > Remove "protected". This will make the access of this method "package". > AutofillTestHelper is in the same package > "org.chromium.chrome.browser.autofill", which will allow AutofillTestHelper to > access these methods. Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:663: protected long getProfileUseDateForTesting(String guid) { On 2016/07/12 15:41:57, Rouslan (ツ) wrote: > Ditto Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:672: * @param guid The GUID of the credit card. On 2016/07/12 15:41:57, Rouslan (ツ) wrote: > newline before @param Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:686: protected int getCreditCardUseCountForTesting(String guid) { On 2016/07/12 15:41:57, Rouslan (ツ) wrote: > Remove "protected" Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:692: protected long getCreditCardUseDateForTesting(String guid) { On 2016/07/12 15:41:57, Rouslan (ツ) wrote: > ditto Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:698: protected long getCurrentDateForTesting() { On 2016/07/12 15:41:57, Rouslan (ツ) wrote: > ditto Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java (right): https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentInstrument.java:65: public String getCardGUID() { On 2016/07/12 15:41:57, Rouslan (ツ) wrote: > This information is already returned from getIdentifier(). Use that instead. > Remove this method. Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:827: ((AutofillPaymentInstrument) mPaymentMethodsSection.getSelectedItem()) On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > Remove the cast to AutofillPaymentInstrument on this line. getIdentifier() is > defined in PaymentOption.java, so there's no need to cast it. Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:828: .getCardGUID()); On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > getIdentifier() Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:157: * @param guid The GUID of the profile. On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > newline before @param Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:191: * @param guid The GUID of the profile to modify. On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > 1) newline before @param > > 2) s/to modify/to query/ Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:192: * @return count The use count to assign to the profile. It should be non-negative. On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > 1) Remove "count". > > @return The use count to assign... > > 2) Remove "It should be". This phrasing is reserved for inputs. You can tell the > user of your API that they should pass in non-negative numbers. Usually this is > enforced with an assert inside of your method. > > In contrast, you're returning a value here. You are in control of it. Provide a > guarantee: > > @return The non-negative use count to assign... Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:206: * @param guid The GUID of the profile to modify. On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > 1) newline before @param > > 2) s/to modify/to query/ Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:207: * @return date The use date to assign to the profile. It represents an absolute point in On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > Remove "date" after "@return". Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:224: * increments the use count of the credit card and set its use date to the current time. Also On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > s/set/sets/ Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:226: * @param guid The GUID of the credit card. On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > newline before @param Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:242: * @return date The use date to assign to the credit card. It represents an absolute point in On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > Remove "date" after "@return". Actually it should still be a param, my bad... https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:261: * @param guid The GUID of the credit card to modify. On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > 1) newline before @param > 2) s/modify/query/ Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:262: * @return count The use count to assign to the credit card. It should be non-negative. On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > 1) Remove "count". > 2) Remove "It should be". Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:276: * @param guid The GUID of the credit card to modify. On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > Ditto Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:294: * @return date The current use date. It represents an absolute point in coordinated universal On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > Ditto Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:501: if (!profile) On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > Early returns are only useful when the rest of the function is huge. Here, the > rest of the function is a single line. So you can save 2 lines of code by doing > this instead: > > if (profile) > profile->RecordAndLogUse(); Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:530: if (!profile) On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > Ternary operator saves space for very short lines. > > return profile ? profile->use_count() : -1; Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:542: if (!profile) On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > Ditto Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:554: if (!card) On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > Ditto Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:583: if (!card) On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > Ditto Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:595: if (!card) On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > Ditto Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:144: // Record the use and log usage metrics for the profile associated with the On 2016/07/12 15:41:58, Rouslan (ツ) wrote: > Records Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:147: void RecordAndLogProfileUse( On 2016/07/12 15:41:59, Rouslan (ツ) wrote: > Line 78 above says "These functions act on local credit cards." You may want to > place profile functions above that line or create your own section for usage > stats only. Done. https://codereview.chromium.org/2126213002/diff/100001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:179: // Record the use and log usage metrics for the credit card associated with On 2016/07/12 15:41:59, Rouslan (ツ) wrote: > Records Done.
lgtm % nits https://codereview.chromium.org/2126213002/diff/140001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2126213002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:528: nit: remove extra newline. https://codereview.chromium.org/2126213002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:538: Ditto https://codereview.chromium.org/2126213002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:575: Ditto https://codereview.chromium.org/2126213002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:585: Ditto
sebsg@chromium.org changed reviewers: + bauerb@chromium.org
Hi bauerb@, could you please take a look at this CL? Thanks! https://codereview.chromium.org/2126213002/diff/140001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2126213002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:528: On 2016/07/13 18:55:26, Rouslan (ツ) wrote: > nit: remove extra newline. Done. https://codereview.chromium.org/2126213002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:538: On 2016/07/13 18:55:26, Rouslan (ツ) wrote: > Ditto Done. https://codereview.chromium.org/2126213002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:575: On 2016/07/13 18:55:27, Rouslan (ツ) wrote: > Ditto Done. https://codereview.chromium.org/2126213002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:585: On 2016/07/13 18:55:27, Rouslan (ツ) wrote: > Ditto Done.
https://codereview.chromium.org/2126213002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2126213002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:640: * Records the use of the profile associated with the specified |guid|. Effectively increments I think variable names in Javadoc should be {@code name} style. https://codereview.chromium.org/2126213002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/2126213002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:335: mObserverNotified.wait(3000); This seems a bit dangerous... do we actually know this is going to run before onPersonalDataChanged() is called? If not, the notifyAll() call is going to be a no-op and this one is going to block indefinitely (well, for three seconds. But there isn't a check either whether the observer was actually notified or we ran into the timeout). Or did I miss something? Alternatively, you could use something like android.os.ConditionVariable which can deal with the condition being set before waiting. https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:501: if (profile) You have a lot of null checks in here. I didn't look through all of them, but it might be worth going over them and seeing where we would reasonably expect the profile or credit card with the given GUID not to exist. Only those cases should handle it (the others could DCHECK it, but that's not required, because you'll get a crash anyway). https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:588: return base::Time::Now().ToTimeT(); Using actual wall clock time here is a recipe for flakiness. What you probably want to do instead is use the base::Clock interface and inject a mock clock for testing. https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:169: JNIEnv* env, Indent four spaces. https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:218: // universal time (UTC) represented as microseconds since the Windows epoch. Uh, this is how the time is stored internally, but you are calling ToTimeT(), which is based on the Unix epoch. Why not use ToJavaTime()?
https://codereview.chromium.org/2126213002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/2126213002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:335: mObserverNotified.wait(3000); On 2016/07/14 10:14:19, Bernhard Bauer wrote: > This seems a bit dangerous... do we actually know this is going to run before > onPersonalDataChanged() is called? If not, the notifyAll() call is going to be a > no-op and this one is going to block indefinitely (well, for three seconds. But > there isn't a check either whether the observer was actually notified or we ran > into the timeout). Or did I miss something? > > Alternatively, you could use something like android.os.ConditionVariable which > can deal with the condition being set before waiting. I'd recommend using https://cs.chromium.org/chromium/src/content/public/test/android/javatests/sr... which has many usage examples in https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... It also deals with condition being set before waiting.
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
Patchset #4 (id:220001) has been deleted
The CQ bit was checked by sebsg@chromium.org 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...
Sorry for the delay. Another look? Thanks! https://codereview.chromium.org/2126213002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2126213002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:640: * Records the use of the profile associated with the specified |guid|. Effectively increments On 2016/07/14 10:14:19, Bernhard Bauer wrote: > I think variable names in Javadoc should be {@code name} style. Done. https://codereview.chromium.org/2126213002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/2126213002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:335: mObserverNotified.wait(3000); On 2016/07/14 10:14:19, Bernhard Bauer wrote: > This seems a bit dangerous... do we actually know this is going to run before > onPersonalDataChanged() is called? If not, the notifyAll() call is going to be a > no-op and this one is going to block indefinitely (well, for three seconds. But > there isn't a check either whether the observer was actually notified or we ran > into the timeout). Or did I miss something? > > Alternatively, you could use something like android.os.ConditionVariable which > can deal with the condition being set before waiting. Done. https://codereview.chromium.org/2126213002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:335: mObserverNotified.wait(3000); On 2016/07/14 17:51:17, Rouslan (ツ) wrote: > On 2016/07/14 10:14:19, Bernhard Bauer wrote: > > This seems a bit dangerous... do we actually know this is going to run before > > onPersonalDataChanged() is called? If not, the notifyAll() call is going to be > a > > no-op and this one is going to block indefinitely (well, for three seconds. > But > > there isn't a check either whether the observer was actually notified or we > ran > > into the timeout). Or did I miss something? > > > > Alternatively, you could use something like android.os.ConditionVariable which > > can deal with the condition being set before waiting. > > I'd recommend using > https://cs.chromium.org/chromium/src/content/public/test/android/javatests/sr... > which has many usage examples in > https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... > > It also deals with condition being set before waiting. Done. https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:501: if (profile) On 2016/07/14 10:14:19, Bernhard Bauer wrote: > You have a lot of null checks in here. I didn't look through all of them, but it > might be worth going over them and seeing where we would reasonably expect the > profile or credit card with the given GUID not to exist. Only those cases should > handle it (the others could DCHECK it, but that's not required, because you'll > get a crash anyway). We never expect not to get a profile/card with the given GUID. I removed all of the null checks. They made no sense for tests but somehow made me feel more safe for feature code. But it might have lead to undefined behavior. Thanks for the suggestion. https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:588: return base::Time::Now().ToTimeT(); On 2016/07/14 10:14:19, Bernhard Bauer wrote: > Using actual wall clock time here is a recipe for flakiness. What you probably > want to do instead is use the base::Clock interface and inject a mock clock for > testing. Good point and I agree. My investigation led me down the stack to AutofillDataModel which uses the base::Time()::Now(). I propose to do this change in another CL since it will require changes to a bunch of places (and lots of tests) in Autofill code. What do you think? https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil... https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:169: JNIEnv* env, On 2016/07/14 10:14:19, Bernhard Bauer wrote: > Indent four spaces. Done. https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:218: // universal time (UTC) represented as microseconds since the Windows epoch. On 2016/07/14 10:14:19, Bernhard Bauer wrote: > Uh, this is how the time is stored internally, but you are calling ToTimeT(), > which is based on the Unix epoch. Why not use ToJavaTime()? I did not use it because it does not seem to be paired with a FromJavaTime() to be used when setting the use stats. Also because TimeT is used in the autofill code so I wanted to be consistent with that. This method will be removed once the mock clock is implemented. I can probably make it work with JavaTime in the meantime if you prefer.
Sorry for the delay. Another look? Thanks!
LGTM w/ nits: https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:588: return base::Time::Now().ToTimeT(); On 2016/07/20 14:56:47, sebsg wrote: > On 2016/07/14 10:14:19, Bernhard Bauer wrote: > > Using actual wall clock time here is a recipe for flakiness. What you probably > > want to do instead is use the base::Clock interface and inject a mock clock > for > > testing. > > Good point and I agree. My investigation led me down the stack to > AutofillDataModel which uses the base::Time()::Now(). I propose to do this > change in another CL since it will require changes to a bunch of places (and > lots of tests) in Autofill code. What do you think? > > https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil... Ok, you added a TODO. Thanks :) https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:218: // universal time (UTC) represented as microseconds since the Windows epoch. On 2016/07/20 14:56:47, sebsg wrote: > On 2016/07/14 10:14:19, Bernhard Bauer wrote: > > Uh, this is how the time is stored internally, but you are calling ToTimeT(), > > which is based on the Unix epoch. Why not use ToJavaTime()? > > I did not use it because it does not seem to be paired with a FromJavaTime() to > be used when setting the use stats. Also because TimeT is used in the autofill > code so I wanted to be consistent with that. > > This method will be removed once the mock clock is implemented. I can probably > make it work with JavaTime in the meantime if you prefer. Hm, if you want to use time_t, can you at least update the comment to be correct? https://codereview.chromium.org/2126213002/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/2126213002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:26: mOnPersonalDataChangedHelper = new CallbackHelper(); I would initialize this where the member is declared.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #6 (id:280001) has been deleted
Thanks for the comments will send to CQ. https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/2126213002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:218: // universal time (UTC) represented as microseconds since the Windows epoch. On 2016/07/20 15:09:44, Bernhard Bauer wrote: > On 2016/07/20 14:56:47, sebsg wrote: > > On 2016/07/14 10:14:19, Bernhard Bauer wrote: > > > Uh, this is how the time is stored internally, but you are calling > ToTimeT(), > > > which is based on the Unix epoch. Why not use ToJavaTime()? > > > > I did not use it because it does not seem to be paired with a FromJavaTime() > to > > be used when setting the use stats. Also because TimeT is used in the autofill > > code so I wanted to be consistent with that. > > > > This method will be removed once the mock clock is implemented. I can probably > > make it work with JavaTime in the meantime if you prefer. > > Hm, if you want to use time_t, can you at least update the comment to be > correct? Done. https://codereview.chromium.org/2126213002/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/2126213002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:26: mOnPersonalDataChangedHelper = new CallbackHelper(); On 2016/07/20 15:09:44, Bernhard Bauer wrote: > I would initialize this where the member is declared. Done.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, rouslan@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2126213002/#ps300001 (title: "Fixed temp card bug + nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Record and log the use of a profile and/or credit card when they get used in in Payment Request. Calls the RecordAndLogUse() method that is currently used in Autofill. BUG=625643 TEST=PersonalDataManagerTest, PaymentRequestUseStatsTest ========== to ========== Record and log the use of a profile and/or credit card when they get used in in Payment Request. Calls the RecordAndLogUse() method that is currently used in Autofill. BUG=625643 TEST=PersonalDataManagerTest, PaymentRequestUseStatsTest ==========
Message was sent while issue was closed.
Committed patchset #6 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Record and log the use of a profile and/or credit card when they get used in in Payment Request. Calls the RecordAndLogUse() method that is currently used in Autofill. BUG=625643 TEST=PersonalDataManagerTest, PaymentRequestUseStatsTest ========== to ========== Record and log the use of a profile and/or credit card when they get used in in Payment Request. Calls the RecordAndLogUse() method that is currently used in Autofill. BUG=625643 TEST=PersonalDataManagerTest, PaymentRequestUseStatsTest Committed: https://crrev.com/0b85d558587cb6195338161ca6ebcf77f6b7166d Cr-Commit-Position: refs/heads/master@{#406692} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0b85d558587cb6195338161ca6ebcf77f6b7166d Cr-Commit-Position: refs/heads/master@{#406692} |
