Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2607043002:
[Autofill] Credit Card Autofill Last Used Date Experiment (Closed)
|
Created:
3 years, 11 months ago by jiahuiguo Modified:
3 years, 10 months ago CC:
rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, asvitkine+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, mikeying Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCredit Card Autofill Last Used Date Experiment
Implemented displaying the last used date in credit card autofill suggestions.
BUG=671880
TEST=TimeFormattingTest.TimeFormatWithPattern(base_unittests),
CreditCardTest.GetLastUsedDateForDisplay(components_unittests),
PersonalDataManagerTest.*(components_unittests)
Review-Url: https://codereview.chromium.org/2607043002
Cr-Commit-Position: refs/heads/master@{#450089}
Committed: https://chromium.googlesource.com/chromium/src/+/43b1225755424b08683d7da6b99f8a695c6c86b4
Patch Set 1 : Added autofill last used date experiment and flags #Patch Set 2 : Fix histgram issue #Patch Set 3 : Show expiration date if keyboard accessory is enabled #
Total comments: 30
Patch Set 4 : Added variations to show expiration date and detail time info #
Total comments: 34
Patch Set 5 : Resolved comments #Patch Set 6 : Fixed some nits #Patch Set 7 : Add date formatter based on pattern #
Total comments: 8
Patch Set 8 : Refactored and added unittest #Patch Set 9 : Added unittest dependency #
Total comments: 16
Patch Set 10 : Unified variation unittest using variations_params_manager #Patch Set 11 : Fixed conflicts #
Total comments: 6
Patch Set 12 : Separated TimeFormatting unittests per locales #Patch Set 13 : Disable time formatting unittest on Android #Patch Set 14 : Enable unittest on android for time_formatting #
Total comments: 23
Patch Set 15 : Refactored function and unittests #Patch Set 16 : Cleaned up included header #
Total comments: 8
Patch Set 17 : Addressed reviewer's comments #
Total comments: 2
Patch Set 18 : Get rid of show_time_detail variation because msg is too long after translation #Patch Set 19 : Resolved patch conflicts #Patch Set 20 : Resolved patch conflicts #
Total comments: 6
Patch Set 21 : Addressed comments #Messages
Total messages: 152 (115 generated)
The CQ bit was checked by jiahuiguo@google.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...
Description was changed from ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last time the card was used in Chrome Autofill in format "Last used: MM/DD at HH:MM". If this is a newly added card, and never been used in Autofill, will show "Added to Chrome: MM/DD". BUG=671880 TEST=Smoke Test ========== to ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last time the card was used in Chrome Autofill in format "Last used: MM/DD at HH:MM". If this is a newly added card, and never been used in Autofill, will show "Added to Chrome: MM/DD". BUG=671880 TEST=Smoke Test ==========
jiahuiguo@google.com changed reviewers: + csashi@google.com, jsaul@google.com, mathp@chromium.org, sebsg@chromium.org
jiahuiguo@google.com changed required reviewers: + mathp@chromium.org
jiahuiguo@google.com changed required reviewers: + sebsg@chromium.org
jiahuiguo@google.com changed reviewers: + estade@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
jiahuiguo@google.com changed reviewers: + rkaplow@chromium.org
jiahuiguo@google.com changed required reviewers: + rkaplow@chromium.org
The CQ bit was checked by jiahuiguo@google.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 jiahuiguo@google.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...
Description was changed from ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last time the card was used in Chrome Autofill in format "Last used: MM/DD at HH:MM". If this is a newly added card, and never been used in Autofill, will show "Added to Chrome: MM/DD". BUG=671880 TEST=Smoke Test ========== to ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last time the card was used in Chrome Autofill in format "Last used: MM/DD at HH:MM". If this is a newly added card, and never been used in Autofill, will show "Added to Chrome: MM/DD". BUG=671880 TEST=Smoke Test, Accessibility TalkBack Test ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jiahuiguo@google.com changed reviewers: - csashi@google.com, jsaul@google.com
Description was changed from ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last time the card was used in Chrome Autofill in format "Last used: MM/DD at HH:MM". If this is a newly added card, and never been used in Autofill, will show "Added to Chrome: MM/DD". BUG=671880 TEST=Smoke Test, Accessibility TalkBack Test ========== to ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last time the card was used in Chrome Autofill in format "Last used: MM/DD at HH:MM". If this is a newly added card, and never been used in Autofill, will show "Added to Chrome: MM/DD". BUG=671880 TEST=Smoke Test, Accessibility TalkBack Test, screenshots available in https://crbug.com/671880 ==========
jiahuiguo@google.com changed reviewers: + csashi@google.com, jsaul@google.com
jiahuiguo@google.com changed required reviewers: - sebsg@chromium.org
Hi Mathieu, Sébastien, Evan, Robert, Jared, Sashi Please review a changelist for the autofill last used date experiment. https://codereview.chromium.org/2607043002
https://codereview.chromium.org/2607043002/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2607043002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:15235: + </message> Display the last used data of the the credit card in autofill. (Similarly, reword text below). https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_experiments.h (right): https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_experiments.h:77: bool IsAutofillCreditCardLastUsedDateDisplayExperimentEnabled(); Would DisplayLastUsedDateInsteadOfExpDate (or some equivalent) be a better name for this? https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... File components/autofill/core/browser/credit_card.cc (right): https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:792: base::string16 minute = FormatDateTo2Digit(exploded.minute); You do not need hour and minutes unless use_count() > 1. https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:814: return zero; You are not returning zero. May be call this padded_date (or equivalent)? https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.h:219: // Formatting month or day info to exact 2 digits exactly 2 digits https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.h:219: // Formatting month or day info to exact 2 digits Is it not "month *and* day" ? https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.h:220: base::string16 FormatDateTo2Digit(int dateInfo) const; Is it 2 digits? MM/DD is 4 digits. https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1629: suggestion->label = credit_card->LastUsedDateAsString(); Is the intent of the experiment to display last used date instead of the expiration date? https://codereview.chromium.org/2607043002/diff/40001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/40001/components/autofill_str... components/autofill_strings.grdp:251: Last used: <ph name="LAST_USED_MONTH">$1<ex>10</ex></ph>/<ph name="LAST_USED_DAY_OF_MONTH">$2<ex>17</ex></ph> at <ph name="LAST_USED_HOUR">$3<ex>10</ex></ph>:<ph name="LAST_USED_MINUTE">$4<ex>00</ex></ph> Would it be useful to make the hour and minute display optional? Could the hour and minute be information overload?
many reviewers listed here --- is there something in particular you'd like each to look at?
jiahuiguo@google.com changed reviewers: - sebsg@chromium.org
https://codereview.chromium.org/2607043002/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2607043002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:15235: + </message> On 2017/01/03 20:54:06, csashi wrote: > Display the last used data of the the credit card in autofill. (Similarly, > reword text below). This, but s/the the/a (and also reword above too) https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.h:177: // Returns the date(MM/DD) when the credit card was last used in autofill nit: Add period at end https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.h:220: base::string16 FormatDateTo2Digit(int dateInfo) const; On 2017/01/03 20:54:06, csashi wrote: > Is it 2 digits? MM/DD is 4 digits. Yeah but this function only takes a certain part of it at a time (day, month, etc.) Perhaps just rename to "FormatTo2Digits(int dateInfo)" instead? https://codereview.chromium.org/2607043002/diff/40001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/40001/components/autofill_str... components/autofill_strings.grdp:251: Last used: <ph name="LAST_USED_MONTH">$1<ex>10</ex></ph>/<ph name="LAST_USED_DAY_OF_MONTH">$2<ex>17</ex></ph> at <ph name="LAST_USED_HOUR">$3<ex>10</ex></ph>:<ph name="LAST_USED_MINUTE">$4<ex>00</ex></ph> On 2017/01/03 20:54:06, csashi wrote: > Would it be useful to make the hour and minute display optional? Could the hour > and minute be information overload? I agree and would just advocate removing it. Chrome knowing the exact minute you used your credit card seems more creepy than helpful, to me. Probably check with Alex though. https://codereview.chromium.org/2607043002/diff/40001/components/autofill_str... components/autofill_strings.grdp:251: Last used: <ph name="LAST_USED_MONTH">$1<ex>10</ex></ph>/<ph name="LAST_USED_DAY_OF_MONTH">$2<ex>17</ex></ph> at <ph name="LAST_USED_HOUR">$3<ex>10</ex></ph>:<ph name="LAST_USED_MINUTE">$4<ex>00</ex></ph> MM/DD is mainly an American thing, so it's definitely not going to work internationally. Could we maybe have it be a three-letter abbreviation? "Last used: May 3" "Last used: Apr. 21" "Last used: Sep. 30"
https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1628: !IsKeyboardAccessoryEnabled()) { if !IsKeyboardAccessoryEnabled(), do we want the user to be registered to the field trial?
The CQ bit was checked by jiahuiguo@google.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.
jiahuiguo@google.com changed reviewers: - estade@chromium.org
Hi Mathieu, Robert, Jared, Sashi, Thanks for the comments, PTAL the revision and newly added variations for the experiment. https://codereview.chromium.org/2607043002/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2607043002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:15235: + </message> On 2017/01/03 20:54:06, csashi wrote: > Display the last used data of the the credit card in autofill. (Similarly, > reword text below). Done. https://codereview.chromium.org/2607043002/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:15235: + </message> On 2017/01/04 22:01:23, jsaul wrote: > On 2017/01/03 20:54:06, csashi wrote: > > Display the last used data of the the credit card in autofill. (Similarly, > > reword text below). > > This, but s/the the/a > > (and also reword above too) Done. https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_experiments.h (right): https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_experiments.h:77: bool IsAutofillCreditCardLastUsedDateDisplayExperimentEnabled(); On 2017/01/03 20:54:06, csashi wrote: > Would DisplayLastUsedDateInsteadOfExpDate (or some equivalent) be a better name > for this? Synced with Alex, we want to show the expiration date along with the last used date in one variation. So this name may be more generic? https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... File components/autofill/core/browser/credit_card.cc (right): https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:792: base::string16 minute = FormatDateTo2Digit(exploded.minute); On 2017/01/03 20:54:06, csashi wrote: > You do not need hour and minutes unless use_count() > 1. Good point! https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:814: return zero; On 2017/01/03 20:54:06, csashi wrote: > You are not returning zero. May be call this padded_date (or equivalent)? Done. https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.h:177: // Returns the date(MM/DD) when the credit card was last used in autofill On 2017/01/04 22:01:24, jsaul wrote: > nit: Add period at end Done. https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.h:219: // Formatting month or day info to exact 2 digits On 2017/01/03 20:54:06, csashi wrote: > Is it not "month *and* day" ? Done. https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.h:219: // Formatting month or day info to exact 2 digits On 2017/01/03 20:54:06, csashi wrote: > exactly 2 digits Done. https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.h:220: base::string16 FormatDateTo2Digit(int dateInfo) const; On 2017/01/04 22:01:23, jsaul wrote: > On 2017/01/03 20:54:06, csashi wrote: > > Is it 2 digits? MM/DD is 4 digits. > > Yeah but this function only takes a certain part of it at a time (day, month, > etc.) Perhaps just rename to "FormatTo2Digits(int dateInfo)" instead? Done. https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/credit_card.h:220: base::string16 FormatDateTo2Digit(int dateInfo) const; On 2017/01/03 20:54:06, csashi wrote: > Is it 2 digits? MM/DD is 4 digits. It only formats month or day for each call, return either MM or DD. https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1628: !IsKeyboardAccessoryEnabled()) { On 2017/01/09 21:54:17, rkaplow wrote: > if !IsKeyboardAccessoryEnabled(), do we want the user to be registered to the > field trial? We will adopt the keyboard accessory in later experiment. https://codereview.chromium.org/2607043002/diff/40001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:1629: suggestion->label = credit_card->LastUsedDateAsString(); On 2017/01/03 20:54:06, csashi wrote: > Is the intent of the experiment to display last used date instead of the > expiration date? It was, but after discussion with Alex, we may not lose thes expiration date info totally. So it is added as a variation in the new patch. https://codereview.chromium.org/2607043002/diff/40001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/40001/components/autofill_str... components/autofill_strings.grdp:251: Last used: <ph name="LAST_USED_MONTH">$1<ex>10</ex></ph>/<ph name="LAST_USED_DAY_OF_MONTH">$2<ex>17</ex></ph> at <ph name="LAST_USED_HOUR">$3<ex>10</ex></ph>:<ph name="LAST_USED_MINUTE">$4<ex>00</ex></ph> On 2017/01/03 20:54:06, csashi wrote: > Would it be useful to make the hour and minute display optional? Could the hour > and minute be information overload? Done. https://codereview.chromium.org/2607043002/diff/40001/components/autofill_str... components/autofill_strings.grdp:251: Last used: <ph name="LAST_USED_MONTH">$1<ex>10</ex></ph>/<ph name="LAST_USED_DAY_OF_MONTH">$2<ex>17</ex></ph> at <ph name="LAST_USED_HOUR">$3<ex>10</ex></ph>:<ph name="LAST_USED_MINUTE">$4<ex>00</ex></ph> On 2017/01/04 22:01:24, jsaul wrote: > MM/DD is mainly an American thing, so it's definitely not going to work > internationally. Could we maybe have it be a three-letter abbreviation? > > "Last used: May 3" > "Last used: Apr. 21" > "Last used: Sep. 30" Good point! https://codereview.chromium.org/2607043002/diff/40001/components/autofill_str... components/autofill_strings.grdp:251: Last used: <ph name="LAST_USED_MONTH">$1<ex>10</ex></ph>/<ph name="LAST_USED_DAY_OF_MONTH">$2<ex>17</ex></ph> at <ph name="LAST_USED_HOUR">$3<ex>10</ex></ph>:<ph name="LAST_USED_MINUTE">$4<ex>00</ex></ph> On 2017/01/04 22:01:24, jsaul wrote: > On 2017/01/03 20:54:06, csashi wrote: > > Would it be useful to make the hour and minute display optional? Could the > hour > > and minute be information overload? > > I agree and would just advocate removing it. Chrome knowing the exact minute > you used your credit card seems more creepy than helpful, to me. Probably check > with Alex though. Yeah, I agree with you. It was added as a variation, maybe we can decide after getting some data.
lgtm histograms.xml change and experiment lg
Some comments, thanks! https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... File components/autofill/core/browser/credit_card.cc (right): https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:779: base::string16 CreditCard::LastUsedDateAsString( Could we rename this GetLastUsedDataForDisplay? https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:788: if (use_count() == 1) put curlies {} https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:791: ? IDS_AUTOFILL_CREDIT_CARD_ADDED_TO_CHROME_DATE_LABEL_AND_EXP let's not mention Chrome in the label name. "IDS_AUTOFILL_CREDIT_CARD_ADDED__DATE_LABEL_AND_EXP" https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:817: base::string16 dateIn2Digit = base::IntToString16(date_info); nit: C++ style is no camel-case https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.h:226: base::string16 PadTo2Digit(int32_t dateInfo) const; nit: date_info https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.h:226: base::string16 PadTo2Digit(int32_t dateInfo) const; can this be in the anonymous namespace in the cc file? https://codereview.chromium.org/2607043002/diff/60001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/60001/components/autofill_str... components/autofill_strings.grdp:251: , last used: <ph name="LAST_USED_MONTH">$1<ex>Feb</ex></ph> <ph name="LAST_USED_DAY_OF_MONTH">$2<ex>17</ex></ph> at <ph name="LAST_USED_HOUR">$3<ex>10</ex></ph>:<ph name="LAST_USED_MINUTE">$4<ex>00</ex></ph> Can you look at https://cs.chromium.org/chromium/src/base/i18n/time_formatting.cc?type=cs&q=i... ? I don't really like that we are reinventing date formatting
https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_experiments.cc (right): https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_experiments.cc:48: const char kAutofillCreditCardLastUsedDateShowTimeDetailKey[] = The previous file (and later in this one) calls the experiments ShowExpirationDate and ShowDetailedTimeInfo, but this line uses ShowTimeDetail instead. Would prefer consistency (ShowDetailedTimeInfoKey?) https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... File components/autofill/core/browser/credit_card.cc (right): https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:787: // use_count() is initialized as 1 when the card is just added nit: Here and in the other comment on line 795, add period at end https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:791: ? IDS_AUTOFILL_CREDIT_CARD_ADDED_TO_CHROME_DATE_LABEL_AND_EXP On 2017/01/18 14:42:15, Mathieu Perreault wrote: > let's not mention Chrome in the label name. > "IDS_AUTOFILL_CREDIT_CARD_ADDED__DATE_LABEL_AND_EXP" (But with s/ADDED__DATE/ADDED_DATE) https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.h:179: bool show_time_detail, Based on outcome of my previous comment, should this be show_detailed_time_info for consistency? https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.h:180: bool show_expiration_date, super nit: Likewise, show_expiration_date has come before show_detailed_time_info everywhere else. Can we swap these to keep the ordering consistent? https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.h:225: // Padding month and day info to exactly 2 digits nit: Add period; looks like that's the norm around here https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:303: << " performed for this version"; I assume none of these changes are you...is it because you synced...? https://codereview.chromium.org/2607043002/diff/60001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/60001/components/autofill_str... components/autofill_strings.grdp:250: <message name="IDS_AUTOFILL_CREDIT_CARD_LAST_USED_DATE_LABEL_AND_EXP_WITH_DETAIL" desc="text displayed in the Autofill Credit Card popup to indicate the date when this card is last used."> s/desc="text/desc="Text (here and below) https://codereview.chromium.org/2607043002/diff/60001/components/autofill_str... components/autofill_strings.grdp:250: <message name="IDS_AUTOFILL_CREDIT_CARD_LAST_USED_DATE_LABEL_AND_EXP_WITH_DETAIL" desc="text displayed in the Autofill Credit Card popup to indicate the date when this card is last used."> s/is last used/was last used (here and below) https://codereview.chromium.org/2607043002/diff/60001/components/autofill_str... components/autofill_strings.grdp:251: , last used: <ph name="LAST_USED_MONTH">$1<ex>Feb</ex></ph> <ph name="LAST_USED_DAY_OF_MONTH">$2<ex>17</ex></ph> at <ph name="LAST_USED_HOUR">$3<ex>10</ex></ph>:<ph name="LAST_USED_MINUTE">$4<ex>00</ex></ph> On 2017/01/18 14:42:15, Mathieu Perreault wrote: > Can you look at > https://cs.chromium.org/chromium/src/base/i18n/time_formatting.cc?type=cs&q=i... > ? I don't really like that we are reinventing date formatting I also don't like that some of these messages arbitrarily start with commas and are lowercase; the translators aren't going to know what that's all about, and it seems like something we should be able to abstract away from the translations...
The CQ bit was checked by jiahuiguo@google.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 checked by jiahuiguo@google.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...
jiahuiguo@google.com changed reviewers: - rkaplow@chromium.org
jiahuiguo@google.com changed required reviewers: - rkaplow@chromium.org
Hi Mathieu, Jared and Sashi, Thanks for the comments, PTAL. https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_experiments.cc (right): https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_experiments.cc:48: const char kAutofillCreditCardLastUsedDateShowTimeDetailKey[] = On 2017/01/18 18:43:44, Jared Saul wrote: > The previous file (and later in this one) calls the experiments > ShowExpirationDate and ShowDetailedTimeInfo, but this line uses ShowTimeDetail > instead. Would prefer consistency (ShowDetailedTimeInfoKey?) Consist to showTimeDetail, otherwise the feature variation name will exceed the column limit. https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... File components/autofill/core/browser/credit_card.cc (right): https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:779: base::string16 CreditCard::LastUsedDateAsString( On 2017/01/18 14:42:15, Mathieu Perreault wrote: > Could we rename this GetLastUsedDataForDisplay? Done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:787: // use_count() is initialized as 1 when the card is just added On 2017/01/18 18:43:44, Jared Saul wrote: > nit: Here and in the other comment on line 795, add period at end Done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:788: if (use_count() == 1) On 2017/01/18 14:42:15, Mathieu Perreault wrote: > put curlies {} Done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:791: ? IDS_AUTOFILL_CREDIT_CARD_ADDED_TO_CHROME_DATE_LABEL_AND_EXP On 2017/01/18 14:42:15, Mathieu Perreault wrote: > let's not mention Chrome in the label name. > "IDS_AUTOFILL_CREDIT_CARD_ADDED__DATE_LABEL_AND_EXP" Done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:791: ? IDS_AUTOFILL_CREDIT_CARD_ADDED_TO_CHROME_DATE_LABEL_AND_EXP On 2017/01/18 18:43:44, Jared Saul wrote: > On 2017/01/18 14:42:15, Mathieu Perreault wrote: > > let's not mention Chrome in the label name. > > "IDS_AUTOFILL_CREDIT_CARD_ADDED__DATE_LABEL_AND_EXP" > > (But with s/ADDED__DATE/ADDED_DATE) Done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.cc:817: base::string16 dateIn2Digit = base::IntToString16(date_info); On 2017/01/18 14:42:15, Mathieu Perreault wrote: > nit: C++ style is no camel-case Done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.h:179: bool show_time_detail, On 2017/01/18 18:43:44, Jared Saul wrote: > Based on outcome of my previous comment, should this be show_detailed_time_info > for consistency? Done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.h:180: bool show_expiration_date, On 2017/01/18 18:43:44, Jared Saul wrote: > super nit: Likewise, show_expiration_date has come before > show_detailed_time_info everywhere else. Can we swap these to keep the ordering > consistent? Done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.h:225: // Padding month and day info to exactly 2 digits On 2017/01/18 18:43:44, Jared Saul wrote: > nit: Add period; looks like that's the norm around here Done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.h:226: base::string16 PadTo2Digit(int32_t dateInfo) const; On 2017/01/18 14:42:15, Mathieu Perreault wrote: > nit: date_info Done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/credit_card.h:226: base::string16 PadTo2Digit(int32_t dateInfo) const; On 2017/01/18 14:42:15, Mathieu Perreault wrote: > can this be in the anonymous namespace in the cc file? Good point, done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2607043002/diff/60001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:303: << " performed for this version"; On 2017/01/18 18:43:44, Jared Saul wrote: > I assume none of these changes are you...is it because you synced...? No, I did not change this. It is in the original repo. https://codereview.chromium.org/2607043002/diff/60001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/60001/components/autofill_str... components/autofill_strings.grdp:250: <message name="IDS_AUTOFILL_CREDIT_CARD_LAST_USED_DATE_LABEL_AND_EXP_WITH_DETAIL" desc="text displayed in the Autofill Credit Card popup to indicate the date when this card is last used."> On 2017/01/18 18:43:44, Jared Saul wrote: > s/desc="text/desc="Text (here and below) Done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill_str... components/autofill_strings.grdp:250: <message name="IDS_AUTOFILL_CREDIT_CARD_LAST_USED_DATE_LABEL_AND_EXP_WITH_DETAIL" desc="text displayed in the Autofill Credit Card popup to indicate the date when this card is last used."> On 2017/01/18 18:43:44, Jared Saul wrote: > s/is last used/was last used (here and below) Done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill_str... components/autofill_strings.grdp:251: , last used: <ph name="LAST_USED_MONTH">$1<ex>Feb</ex></ph> <ph name="LAST_USED_DAY_OF_MONTH">$2<ex>17</ex></ph> at <ph name="LAST_USED_HOUR">$3<ex>10</ex></ph>:<ph name="LAST_USED_MINUTE">$4<ex>00</ex></ph> On 2017/01/18 18:43:44, Jared Saul wrote: > On 2017/01/18 14:42:15, Mathieu Perreault wrote: > > Can you look at > > > https://cs.chromium.org/chromium/src/base/i18n/time_formatting.cc?type=cs&q=i... > > ? I don't really like that we are reinventing date formatting > > I also don't like that some of these messages arbitrarily start with commas and > are lowercase; the translators aren't going to know what that's all about, and > it seems like something we should be able to abstract away from the > translations... Done. https://codereview.chromium.org/2607043002/diff/60001/components/autofill_str... components/autofill_strings.grdp:251: , last used: <ph name="LAST_USED_MONTH">$1<ex>Feb</ex></ph> <ph name="LAST_USED_DAY_OF_MONTH">$2<ex>17</ex></ph> at <ph name="LAST_USED_HOUR">$3<ex>10</ex></ph>:<ph name="LAST_USED_MINUTE">$4<ex>00</ex></ph> On 2017/01/18 14:42:15, Mathieu Perreault wrote: > Can you look at > https://cs.chromium.org/chromium/src/base/i18n/time_formatting.cc?type=cs&q=i... > ? I don't really like that we are reinventing date formatting Thanks for pointing this out, I utilize this API to format HH:MM. For the exact date, the API will show year info, which we do not need in this UI.
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 jiahuiguo@google.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...
jiahuiguo@google.com changed reviewers: + jshin@chromium.org
Hi Mathieu, I found it might be better to add a function to the time_formatting library, I added Jungshik as reviewer for this. I leave the unittest in personal_data_manager to the next patch if this is a good direction to go. Hi Jungshik, Can you take a look at the change in time_formatting? The reason I added this function is we need to have a customized date string for display, in this case to use the "pattern" would be the best way for i18n. May I know if this is a good way to approach this? And if so, can you suggest how to write the unittest, since the pattern would be A LOT... And if so, would it be better if we separate this CL to 2 CLs? Thanks,
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/20 05:36:47, jiahuiguo wrote: > Hi Mathieu, > > I found it might be better to add a function to the time_formatting library, I > added Jungshik as reviewer for this. I leave the unittest in > personal_data_manager to the next patch if this is a good direction to go. > > Hi Jungshik, > > Can you take a look at the change in time_formatting? The reason I added this > function is we need to have a customized date string for display, in this case > to use the "pattern" would be the best way for i18n. May I know if this is a > good way to approach this? And if so, can you suggest how to write the unittest, > since the pattern would be A LOT... > > And if so, would it be better if we separate this CL to 2 CLs? > > Thanks, I will let Jungshik comment on the //base addition. Generally I could see it both ways: either a general approach passing the pattern as you wrote (relies on the caller knowing about ICU patterns), or just adding the method(s) we need for our patterns. I am leaning towards the latter. +1 to doing this in a separate change, and should be well tested whatever the approach :)
https://codereview.chromium.org/2607043002/diff/120001/base/i18n/time_formatt... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2607043002/diff/120001/base/i18n/time_formatt... base/i18n/time_formatting.h:84: const char* pattern); Perhaps, add a unit test? https://codereview.chromium.org/2607043002/diff/120001/components/autofill/co... File components/autofill/core/browser/credit_card.cc (right): https://codereview.chromium.org/2607043002/diff/120001/components/autofill/co... components/autofill/core/browser/credit_card.cc:101: return padded_date; Some locales do NOT use ASCII digit. So that prepending with '0' does not work. Why don't you just use TimeFormatWithPattern(time, 'MMdd')? Well, you have to make up a 'time' corresponding to the expiration date to pass, but it'd be a small cost to pay for the locale-aware formatting. https://codereview.chromium.org/2607043002/diff/120001/components/autofill/co... components/autofill/core/browser/credit_card.cc:816: base::TimeFormatWithPattern(use_date(), "MMMdd")); There are 4 combinations here and they're combined with https://codereview.chromium.org/2607043002/diff/120001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2607043002/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1649: ShowTimeDetailInAutofillCreditCardLastUsedDate())); This kind of concatenating strings (Exp. date + LastTimeUsed) is not a good idea. There are 4 combinations and you'd better have 4 messages (added to a grd(p) file). 1. Exp. date: 04/20, last used Dec 10 at 11:35 2. Last used Dec 10 at 11:35 3. Exp. date: 04/20, last used on Dec 10 4. Last used on Dec 10.
BTW, is it ok not to show year (last time used)? What if a card is last used more than a year ago (say, Nov 2015)?
The CQ bit was checked by jiahuiguo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jiahuiguo@google.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...
On 2017/01/23 22:02:34, jungshik at Google wrote: > BTW, is it ok not to show year (last time used)? What if a card is last used > more than a year ago (say, Nov 2015)? Good point, another label "last used > 1 year" is added to address this situation, since if the card is last used year ago, it may not be worthwhile to display the exact date info.
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 jiahuiguo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2607043002/diff/160001/base/i18n/time_formatt... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2607043002/diff/160001/base/i18n/time_formatt... base/i18n/time_formatting.cc:92: NOTREACHED(); Remove NOTREACHED()? https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1703: // indicate "last used > 1 year" without showing date detail. nit wording: // If the card was last used in autofill more than a year ago, // display "last used >1 year ago" without showing date detail. https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1704: if ((base::Time::Now() - credit_card->use_date()).InDays() > 365) { Just wondering, but is there a InYears() instead of InDays() that might automatically account for leap years? https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1715: // If the card is last used in autofill within a year, show date information. nit: s/is/was https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:225: // if |show_time_detail| is true. nit, minor wording changes: // Returns the date when the credit card was last used in autofill. Shows // expiration date if |show_expiration_date| is true, and shows time detail // if |show_time_detail| is true. https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3886: ASCIIToUTF16("Added to Chrome: ") + Should "Chrome" be hard-coded in here or is there a variable that picks Chrome vs. Chromium on the fly? https://codereview.chromium.org/2607043002/diff/160001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/160001/components/autofill_st... components/autofill_strings.grdp:279: Last used > 1 year Not everyone understands or remembers which way > goes. This should probably change to simply "Last used over a year ago".
https://codereview.chromium.org/2607043002/diff/160001/base/i18n/time_formatt... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2607043002/diff/160001/base/i18n/time_formatt... base/i18n/time_formatting.cc:92: NOTREACHED(); Whoops, ignore this--I didn't realize this was a sync issue and someone else wrote it. (Maybe C++ requires a return statement even if NOTREACHED() is going to make the browser crash? Dunno.)
The CQ bit was checked by jiahuiguo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jiahuiguo@google.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...)
jiahuiguo@google.com changed reviewers: - csashi@google.com
jiahuiguo@google.com changed required reviewers: + jshin@chromium.org
Hi Mathieu, Jungshik and Jared, Thanks for the comments, PTAL. https://codereview.chromium.org/2607043002/diff/120001/base/i18n/time_formatt... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2607043002/diff/120001/base/i18n/time_formatt... base/i18n/time_formatting.h:84: const char* pattern); On 2017/01/23 21:43:24, jungshik at Google wrote: > Perhaps, add a unit test? Done. https://codereview.chromium.org/2607043002/diff/120001/components/autofill/co... File components/autofill/core/browser/credit_card.cc (right): https://codereview.chromium.org/2607043002/diff/120001/components/autofill/co... components/autofill/core/browser/credit_card.cc:101: return padded_date; On 2017/01/23 21:43:24, jungshik at Google wrote: > Some locales do NOT use ASCII digit. So that prepending with '0' > does not work. > > Why don't you just use TimeFormatWithPattern(time, 'MMdd')? Well, you have to > make up a 'time' corresponding to the expiration date to pass, but it'd be a > small cost to pay for the locale-aware formatting. Thanks for pointing this out, for the expiration date shown on a credit card, it is ISO standard to format as "MM/YY" or "MM-YY", where the former is prefered, so constructing such a string would be ok for different locales? Actually the way to padding the month is from the original code, and it was used in several places, if it is fundamentally wrong, I can fill a bug and fix it in a following CL. https://codereview.chromium.org/2607043002/diff/120001/components/autofill/co... components/autofill/core/browser/credit_card.cc:816: base::TimeFormatWithPattern(use_date(), "MMMdd")); On 2017/01/23 21:43:24, jungshik at Google wrote: > There are 4 combinations here and they're combined with Done. https://codereview.chromium.org/2607043002/diff/120001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2607043002/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1649: ShowTimeDetailInAutofillCreditCardLastUsedDate())); On 2017/01/23 21:43:24, jungshik at Google wrote: > This kind of concatenating strings (Exp. date + LastTimeUsed) is not a good > idea. There are 4 combinations and you'd better have 4 messages (added to a > grd(p) file). > > 1. Exp. date: 04/20, last used Dec 10 at 11:35 > 2. Last used Dec 10 at 11:35 > 3. Exp. date: 04/20, last used on Dec 10 > 4. Last used on Dec 10. > > Done. https://codereview.chromium.org/2607043002/diff/160001/base/i18n/time_formatt... File base/i18n/time_formatting.cc (right): https://codereview.chromium.org/2607043002/diff/160001/base/i18n/time_formatt... base/i18n/time_formatting.cc:92: NOTREACHED(); On 2017/01/26 00:15:39, Jared Saul wrote: > Whoops, ignore this--I didn't realize this was a sync issue and someone else > wrote it. (Maybe C++ requires a return statement even if NOTREACHED() is going > to make the browser crash? Dunno.) Done. https://codereview.chromium.org/2607043002/diff/160001/base/i18n/time_formatt... base/i18n/time_formatting.cc:92: NOTREACHED(); On 2017/01/25 18:22:17, Jared Saul wrote: > Remove NOTREACHED()? Done. https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1703: // indicate "last used > 1 year" without showing date detail. On 2017/01/25 18:22:17, Jared Saul wrote: > nit wording: > // If the card was last used in autofill more than a year ago, > // display "last used >1 year ago" without showing date detail. Done. https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1704: if ((base::Time::Now() - credit_card->use_date()).InDays() > 365) { On 2017/01/25 18:22:17, Jared Saul wrote: > Just wondering, but is there a InYears() instead of InDays() that might > automatically account for leap years? No https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1715: // If the card is last used in autofill within a year, show date information. On 2017/01/25 18:22:17, Jared Saul wrote: > nit: s/is/was Done. https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:225: // if |show_time_detail| is true. On 2017/01/25 18:22:17, Jared Saul wrote: > nit, minor wording changes: > // Returns the date when the credit card was last used in autofill. Shows > // expiration date if |show_expiration_date| is true, and shows time detail > // if |show_time_detail| is true. Done. https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3886: ASCIIToUTF16("Added to Chrome: ") + On 2017/01/25 18:22:17, Jared Saul wrote: > Should "Chrome" be hard-coded in here or is there a variable that picks Chrome > vs. Chromium on the fly? Good point, added platform checking to distinguish Chrome and Chromium. https://codereview.chromium.org/2607043002/diff/160001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/160001/components/autofill_st... components/autofill_strings.grdp:279: Last used > 1 year On 2017/01/25 18:22:17, Jared Saul wrote: > Not everyone understands or remembers which way > goes. This should probably > change to simply "Last used over a year ago". Done.
Thanks a lot for the revision. Here are a couple of more comments. https://codereview.chromium.org/2607043002/diff/200001/base/i18n/time_formatt... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/200001/base/i18n/time_formatt... base/i18n/time_formatting_unittest.cc:233: i18n::SetICUDefaultLocale("en_US"); Thank for adding tests. I'm sorry that I forgot to tell you about "base/test/scoped_locale.h". To avoid any potential misbehavior, we'd better limit the locale change to a given scope. That means, different locale cases need to be in separate functions or en-US, en-GB and ja-JP have to be enclosed by braces ( { en-US }, {en-GB test} , {ja-JP test} ). https://codereview.chromium.org/2607043002/diff/200001/base/i18n/time_formatt... base/i18n/time_formatting_unittest.cc:246: L"30\x65e5"), Now that every compiler we use support UTF-16 string literals and MSVC (Chromium requires) finally supports the source code in UTF-8 without BOM, you can just use u"2011年4月30日". (I know that a lot of code uses hex-escapes and WideToUTF16). https://codereview.chromium.org/2607043002/diff/200001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2607043002/diff/200001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:226: base::string16 GetLastUsedDateForDisplay( Does this need to be public? Can it be just a static helper inside an anonymous namespace in cc file?
The CQ bit was checked by jiahuiguo@google.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 jiahuiguo@google.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 jiahuiguo@google.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 checked by jiahuiguo@google.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...
Hi Mathieu, Jungshik and Jared, Thanks for the comments, PTAL. The unittest on linux_android_rel_ng is flaky though. https://codereview.chromium.org/2607043002/diff/200001/base/i18n/time_formatt... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/200001/base/i18n/time_formatt... base/i18n/time_formatting_unittest.cc:233: i18n::SetICUDefaultLocale("en_US"); On 2017/01/26 19:29:14, jungshik at Google wrote: > Thank for adding tests. > > I'm sorry that I forgot to tell you about "base/test/scoped_locale.h". > > To avoid any potential misbehavior, we'd better limit the locale change to a > given scope. > > That means, different locale cases need to be in separate functions or en-US, > en-GB and ja-JP have to be enclosed by braces ( { en-US }, {en-GB test} , {ja-JP > test} ). Thanks for the comments, separate to different unittests. Do we still need to disable the test on Android? https://codereview.chromium.org/2607043002/diff/200001/base/i18n/time_formatt... base/i18n/time_formatting_unittest.cc:246: L"30\x65e5"), On 2017/01/26 19:29:14, jungshik at Google wrote: > Now that every compiler we use support UTF-16 string literals and MSVC (Chromium > requires) finally supports the source code in UTF-8 without BOM, you can just > use u"2011年4月30日". (I know that a lot of code uses hex-escapes and > WideToUTF16). Actually if we use u"2011年4月30日" as the first param, it returns a const char16_t string, while a string16 is expected here, we still need to have a convertion. Can you provide an example or suggest how to deal with this? In this new patch I use EXPECT_EQ(WideToUTF16(L"2011年4月30日"), TimeFormatWithPattern(time, "yMMMd")) directly, will this be acceptable since we need an extra conversion anyway? https://codereview.chromium.org/2607043002/diff/200001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2607043002/diff/200001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:226: base::string16 GetLastUsedDateForDisplay( On 2017/01/26 19:29:14, jungshik at Google wrote: > Does this need to be public? Can it be just a static helper inside an anonymous > namespace in cc file? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Jason, some more comments. Really appreciate moving some logic to //base https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... base/i18n/time_formatting.h:91: // when unusual time format is needed, e.g., *when an unusual https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... base/i18n/time_formatting.h:92: // "Feb. 2, 18:00". bring to previous line https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:257: base::string16 GetLastUsedDateForDisplay(const CreditCard* credit_card, I would put this function in AutofillDataModel (which is the base class of AutofillProfile *and* CreditCard). https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:301: "MMMddjmm")) Since we are ever only using two patterns (with time detail and without), we should make them constants. https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1694: suggestion->label = GetLastUsedDateForDisplay( using the suggestion above, you will be able to do credit_card->GetLastUsedDateForDisplay(...); https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3498: variation_params_.SetVariationParams( Thanks for doing this! https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3840: TEST_F(PersonalDataManagerTest, GetCreditCardSuggestions_LastUsedDateOnly) { I think you would need to unit-test the GetLastUsedDateForDisplay in autofill_data_model_unittest. Have a look at INSTANTIATE_TEST_CASE_P [1] to see how to effectively test many variations of the same code. You may not need as many examples as this though: [1] https://cs.chromium.org/chromium/src/components/payments/currency_formatter_u... https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3861: base::TimeFormatWithPattern(credit_cards[2]->use_date(), "MMMdd"), I don't think we should call TimeFormatWithPattern in the expectation, because that's what we are trying to test. Fine to hardcode the expectation here. https://codereview.chromium.org/2607043002/diff/260001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/260001/components/autofill_st... components/autofill_strings.grdp:267: <message name="IDS_AUTOFILL_CREDIT_CARD_EXP_AND_ADDED_DATE" desc="Text displayed in the Autofill Credit Card popup to indicate the credit card expiration date and the date when this card was added to autofill."> nit: I would indent by 2 within the if blocks
https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... base/i18n/time_formatting_unittest.cc:259: TimeFormatWithPattern(time, "yMMMd")); An alternative would be EXPET_EQ(u8"2011年4月30日", UTF16ToUTF8(TImeFormatWithPattern...)); It turned out that u"...." for UTF-16 string literal is not yet allowed to use in Chromium tree (There was a thread back in September but no resolution, yet), but I'm sure 'u8' is allowed. https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3861: base::TimeFormatWithPattern(credit_cards[2]->use_date(), "MMMdd"), On 2017/01/27 15:01:44, Mathieu Perreault wrote: > I don't think we should call TimeFormatWithPattern in the expectation, because > that's what we are trying to test. Fine to hardcode the expectation here. Well, TimeFormatWithPattern is already tested in its own unittest. If a hardcoded strings are used, this test would fail in a locale other than en-US. In case of 'Last used: ' (hard-coded English string), afaik, our unit-test runs with only a single locale bundle so that it'd be still 'Last used: ' even if a test is run in a non-en-US locale.
LGTM with optional changes below and in the previous comment. https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... base/i18n/time_formatting_unittest.cc:228: test::ScopedRestoreICUDefaultLocale restore_locale; Thank you for finding this :-). With this(ScopedREstoreICUDefaultLocale), you don't need to have split tests into en-US, en-GB and ja-JP.
The CQ bit was checked by jiahuiguo@google.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...
Description was changed from ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last time the card was used in Chrome Autofill in format "Last used: MM/DD at HH:MM". If this is a newly added card, and never been used in Autofill, will show "Added to Chrome: MM/DD". BUG=671880 TEST=Smoke Test, Accessibility TalkBack Test, screenshots available in https://crbug.com/671880 ========== to ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last time the card was used in Chrome Autofill in format "Last used: MM/DD at HH:MM". If this is a newly added card, and never been used in Autofill, will show "Added to Chrome: MM/DD". BUG=671880 TEST=Unittests: TimeFormattingTest.TimeFormatWithPattern (base_unittests) and CreditCardTest.GetLastUsedDateForDisplay (components_unittests), Smoke Test, Accessibility TalkBack Test ==========
Description was changed from ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last time the card was used in Chrome Autofill in format "Last used: MM/DD at HH:MM". If this is a newly added card, and never been used in Autofill, will show "Added to Chrome: MM/DD". BUG=671880 TEST=Unittests: TimeFormattingTest.TimeFormatWithPattern (base_unittests) and CreditCardTest.GetLastUsedDateForDisplay (components_unittests), Smoke Test, Accessibility TalkBack Test ========== to ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last used date in credit card autofill suggestions. BUG=671880 TEST=Unittests: TimeFormattingTest.TimeFormatWithPattern (base_unittests) and CreditCardTest.GetLastUsedDateForDisplay (components_unittests), Smoke Test, Accessibility TalkBack Test ==========
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 jiahuiguo@google.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.
Hi Mathieu, Jungshik and Jared, PTAL. For credit_card_unittest, only en_US is tested since GetInfo() function doesn't use the parameter app_locale at all! The app_locale is used only in SetInfo() when converting a named month to expiration month. BTW, could you advise how to use "en-US" or "en_US" properly? I see "en-US" is used in credit_card_unittest.cc, and "en_US" is used in time_formatting_unittest.cc to specific the locale. Thanks, https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... File base/i18n/time_formatting.h (right): https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... base/i18n/time_formatting.h:91: // when unusual time format is needed, e.g., On 2017/01/27 15:01:44, Mathieu Perreault wrote: > *when an unusual Done. https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... base/i18n/time_formatting.h:92: // "Feb. 2, 18:00". On 2017/01/27 15:01:44, Mathieu Perreault wrote: > bring to previous line Done. https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... File base/i18n/time_formatting_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... base/i18n/time_formatting_unittest.cc:228: test::ScopedRestoreICUDefaultLocale restore_locale; On 2017/01/27 21:51:35, jungshik at Google wrote: > Thank you for finding this :-). With this(ScopedREstoreICUDefaultLocale), you > don't need to have > split tests into en-US, en-GB and ja-JP. > Cool, grouped them into one test. https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... base/i18n/time_formatting_unittest.cc:259: TimeFormatWithPattern(time, "yMMMd")); On 2017/01/27 21:43:30, jungshik at Google wrote: > An alternative would be > > EXPET_EQ(u8"2011年4月30日", UTF16ToUTF8(TImeFormatWithPattern...)); > > It turned out that u"...." for UTF-16 string literal is not yet allowed to use > in Chromium tree (There was a thread back in September but no resolution, yet), > but I'm sure 'u8' is allowed. Done. https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:301: "MMMddjmm")) On 2017/01/27 15:01:44, Mathieu Perreault wrote: > Since we are ever only using two patterns (with time detail and without), we > should make them constants. Done. https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1694: suggestion->label = GetLastUsedDateForDisplay( On 2017/01/27 15:01:44, Mathieu Perreault wrote: > using the suggestion above, you will be able to do > credit_card->GetLastUsedDateForDisplay(...); Done. https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3498: variation_params_.SetVariationParams( On 2017/01/27 15:01:44, Mathieu Perreault wrote: > Thanks for doing this! Done. https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3840: TEST_F(PersonalDataManagerTest, GetCreditCardSuggestions_LastUsedDateOnly) { On 2017/01/27 15:01:44, Mathieu Perreault wrote: > I think you would need to unit-test the GetLastUsedDateForDisplay in > autofill_data_model_unittest. > > Have a look at INSTANTIATE_TEST_CASE_P [1] to see how to effectively test many > variations of the same code. > > You may not need as many examples as this though: > [1] > https://cs.chromium.org/chromium/src/components/payments/currency_formatter_u... Done. https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3861: base::TimeFormatWithPattern(credit_cards[2]->use_date(), "MMMdd"), On 2017/01/27 21:43:30, jungshik at Google wrote: > On 2017/01/27 15:01:44, Mathieu Perreault wrote: > > I don't think we should call TimeFormatWithPattern in the expectation, because > > that's what we are trying to test. Fine to hardcode the expectation here. > > Well, TimeFormatWithPattern is already tested in its own unittest. If a > hardcoded strings are used, this test would fail in a locale other than en-US. > In case of 'Last used: ' (hard-coded English string), afaik, our unit-test runs > with only a single locale bundle so that it'd be still 'Last used: ' even if a > test is run in a non-en-US locale. Done. https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3861: base::TimeFormatWithPattern(credit_cards[2]->use_date(), "MMMdd"), On 2017/01/27 15:01:44, Mathieu Perreault wrote: > I don't think we should call TimeFormatWithPattern in the expectation, because > that's what we are trying to test. Fine to hardcode the expectation here. Done. https://codereview.chromium.org/2607043002/diff/260001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/260001/components/autofill_st... components/autofill_strings.grdp:267: <message name="IDS_AUTOFILL_CREDIT_CARD_EXP_AND_ADDED_DATE" desc="Text displayed in the Autofill Credit Card popup to indicate the credit card expiration date and the date when this card was added to autofill."> On 2017/01/27 15:01:44, Mathieu Perreault wrote: > nit: I would indent by 2 within the if blocks Done.
On 2017/02/03 07:11:17, jiahuiguo wrote: > Hi Mathieu, Jungshik and Jared, > > PTAL. For credit_card_unittest, only en_US is tested since GetInfo() function > doesn't use the parameter app_locale at all! The app_locale is used only in > SetInfo() when converting a named month to expiration month. > > BTW, could you advise how to use "en-US" or "en_US" properly? I see "en-US" is > used in credit_card_unittest.cc, and "en_US" is used in > time_formatting_unittest.cc to specific the locale. > > Thanks, > > https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... > File base/i18n/time_formatting.h (right): > > https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... > base/i18n/time_formatting.h:91: // when unusual time format is needed, e.g., > On 2017/01/27 15:01:44, Mathieu Perreault wrote: > > *when an unusual > > Done. > > https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... > base/i18n/time_formatting.h:92: // "Feb. 2, 18:00". > On 2017/01/27 15:01:44, Mathieu Perreault wrote: > > bring to previous line > > Done. > > https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... > File base/i18n/time_formatting_unittest.cc (right): > > https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... > base/i18n/time_formatting_unittest.cc:228: test::ScopedRestoreICUDefaultLocale > restore_locale; > On 2017/01/27 21:51:35, jungshik at Google wrote: > > Thank you for finding this :-). With this(ScopedREstoreICUDefaultLocale), you > > don't need to have > > split tests into en-US, en-GB and ja-JP. > > > > Cool, grouped them into one test. > > https://codereview.chromium.org/2607043002/diff/260001/base/i18n/time_formatt... > base/i18n/time_formatting_unittest.cc:259: TimeFormatWithPattern(time, > "yMMMd")); > On 2017/01/27 21:43:30, jungshik at Google wrote: > > An alternative would be > > > > EXPET_EQ(u8"2011年4月30日", UTF16ToUTF8(TImeFormatWithPattern...)); > > > > It turned out that u"...." for UTF-16 string literal is not yet allowed to use > > in Chromium tree (There was a thread back in September but no resolution, > yet), > > but I'm sure 'u8' is allowed. > > Done. > > https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... > File components/autofill/core/browser/personal_data_manager.cc (right): > > https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... > components/autofill/core/browser/personal_data_manager.cc:301: "MMMddjmm")) > On 2017/01/27 15:01:44, Mathieu Perreault wrote: > > Since we are ever only using two patterns (with time detail and without), we > > should make them constants. > > Done. > > https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... > components/autofill/core/browser/personal_data_manager.cc:1694: > suggestion->label = GetLastUsedDateForDisplay( > On 2017/01/27 15:01:44, Mathieu Perreault wrote: > > using the suggestion above, you will be able to do > > credit_card->GetLastUsedDateForDisplay(...); > > Done. > > https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... > File components/autofill/core/browser/personal_data_manager_unittest.cc (right): > > https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... > components/autofill/core/browser/personal_data_manager_unittest.cc:3498: > variation_params_.SetVariationParams( > On 2017/01/27 15:01:44, Mathieu Perreault wrote: > > Thanks for doing this! > > Done. > > https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... > components/autofill/core/browser/personal_data_manager_unittest.cc:3840: > TEST_F(PersonalDataManagerTest, GetCreditCardSuggestions_LastUsedDateOnly) { > On 2017/01/27 15:01:44, Mathieu Perreault wrote: > > I think you would need to unit-test the GetLastUsedDateForDisplay in > > autofill_data_model_unittest. > > > > Have a look at INSTANTIATE_TEST_CASE_P [1] to see how to effectively test many > > variations of the same code. > > > > You may not need as many examples as this though: > > [1] > > > https://cs.chromium.org/chromium/src/components/payments/currency_formatter_u... > > Done. > > https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... > components/autofill/core/browser/personal_data_manager_unittest.cc:3861: > base::TimeFormatWithPattern(credit_cards[2]->use_date(), "MMMdd"), > On 2017/01/27 21:43:30, jungshik at Google wrote: > > On 2017/01/27 15:01:44, Mathieu Perreault wrote: > > > I don't think we should call TimeFormatWithPattern in the expectation, > because > > > that's what we are trying to test. Fine to hardcode the expectation here. > > > > Well, TimeFormatWithPattern is already tested in its own unittest. If a > > hardcoded strings are used, this test would fail in a locale other than en-US. > > > In case of 'Last used: ' (hard-coded English string), afaik, our unit-test > runs > > with only a single locale bundle so that it'd be still 'Last used: ' even if a > > test is run in a non-en-US locale. > > Done. > > https://codereview.chromium.org/2607043002/diff/260001/components/autofill/co... > components/autofill/core/browser/personal_data_manager_unittest.cc:3861: > base::TimeFormatWithPattern(credit_cards[2]->use_date(), "MMMdd"), > On 2017/01/27 15:01:44, Mathieu Perreault wrote: > > I don't think we should call TimeFormatWithPattern in the expectation, because > > that's what we are trying to test. Fine to hardcode the expectation here. > > Done. > > https://codereview.chromium.org/2607043002/diff/260001/components/autofill_st... > File components/autofill_strings.grdp (right): > > https://codereview.chromium.org/2607043002/diff/260001/components/autofill_st... > components/autofill_strings.grdp:267: <message > name="IDS_AUTOFILL_CREDIT_CARD_EXP_AND_ADDED_DATE" desc="Text displayed in the > Autofill Credit Card popup to indicate the credit card expiration date and the > date when this card was added to autofill."> > On 2017/01/27 15:01:44, Mathieu Perreault wrote: > > nit: I would indent by 2 within the if blocks > > Done. Hi Jason, Looks good! I don't understand your comment around GetInfo... You are calling the new function like so : credit_card->GetLastUsedDateForDisplay(app_locale_), so different values of the app locale should be unit tested. GetInfo doesn't use the locale to change its result formatting, but your needs to, so it should be tested.
Thanks Json https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... File components/autofill/core/browser/autofill_experiments.cc (right): https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... components/autofill/core/browser/autofill_experiments.cc:31: const base::Feature kAutofillCreditCardPopupLayout{ As discussed, you can reuse this feature instead of adding your own. You can keep the param keys, of course https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... File components/autofill/core/browser/credit_card_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... components/autofill/core/browser/credit_card_unittest.cc:994: variation_params_.ClearAllVariationParams(); This should be called when |variation_params_| gets destroyed, so no need to call it here https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3489: variation_params_.ClearAllVariationParams(); same, shouldn't need to be called? https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3523: variation_params_.ClearAllVariationParams(); same
Thanks Jason
Description was changed from ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last used date in credit card autofill suggestions. BUG=671880 TEST=Unittests: TimeFormattingTest.TimeFormatWithPattern (base_unittests) and CreditCardTest.GetLastUsedDateForDisplay (components_unittests), Smoke Test, Accessibility TalkBack Test ========== to ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last used date in credit card autofill suggestions. BUG=671880 TEST=TimeFormattingTest.TimeFormatWithPattern(base_unittests), CreditCardTest.GetLastUsedDateForDisplay(components_unittests), PersonalDataManagerTest.*(components_unittests) ==========
The CQ bit was checked by jiahuiguo@google.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.
jiahuiguo@google.com changed reviewers: + rkaplow@chromium.org
Hi Mathieu and Jared, PTAL. Thanks, https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... File components/autofill/core/browser/autofill_experiments.cc (right): https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... components/autofill/core/browser/autofill_experiments.cc:31: const base::Feature kAutofillCreditCardPopupLayout{ On 2017/02/04 02:37:58, Mathieu Perreault wrote: > As discussed, you can reuse this feature instead of adding your own. You can > keep the param keys, of course Keep the feature for this version, will create a mega flag in a following up CL to incorporate Sashi's flag. https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... File components/autofill/core/browser/credit_card_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... components/autofill/core/browser/credit_card_unittest.cc:994: variation_params_.ClearAllVariationParams(); On 2017/02/04 02:37:58, Mathieu Perreault wrote: > This should be called when |variation_params_| gets destroyed, so no need to > call it here Since this is in a loop, we are reusing the same |variation_params_| instance, it has to be explicated cleared. https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3489: variation_params_.ClearAllVariationParams(); On 2017/02/04 02:37:58, Mathieu Perreault wrote: > same, shouldn't need to be called? Done. https://codereview.chromium.org/2607043002/diff/300001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:3523: variation_params_.ClearAllVariationParams(); On 2017/02/04 02:37:58, Mathieu Perreault wrote: > same Done.
lgtm
https://codereview.chromium.org/2607043002/diff/320001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/320001/components/autofill_st... components/autofill_strings.grdp:250: <message name="IDS_AUTOFILL_CREDIT_CARD_EXP_AND_LAST_USED_DATE_DETAIL" desc="Text displayed in the Autofill Credit Card popup to indicate the credit card expiration date and the date when this card was last used."> RE: limiting the size of the translation, you can use the [CHAR-LIMIT=32] annotation for translations, replacing 32 with the most sensible value. See its use elsewhere in Chrome.
lgtm (but FYI I'm having a really hard time distinguishing which files were changed and where at which patch sets, so it's highly possible I missed something)
The CQ bit was checked by jiahuiguo@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by jiahuiguo@google.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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) 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 jiahuiguo@google.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...
Hi Mahthieu and Jared, PTAL, this patch removed show_time_detail variation since the translated string is too long. Another change is making this experiment available on all platforms. Thanks, https://codereview.chromium.org/2607043002/diff/320001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/320001/components/autofill_st... components/autofill_strings.grdp:250: <message name="IDS_AUTOFILL_CREDIT_CARD_EXP_AND_LAST_USED_DATE_DETAIL" desc="Text displayed in the Autofill Credit Card popup to indicate the credit card expiration date and the date when this card was last used."> On 2017/02/08 11:51:05, Mathieu Perreault wrote: > RE: limiting the size of the translation, you can use the [CHAR-LIMIT=32] > annotation for translations, replacing 32 with the most sensible value. See its > use elsewhere in Chrome. Get rid of the show_time_detail variation and change "Added to Chrome:" to "Added:", since in these two cases the translated msg will exceed the popup width.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks https://codereview.chromium.org/2607043002/diff/380001/components/autofill/co... File components/autofill/core/browser/credit_card.cc (right): https://codereview.chromium.org/2607043002/diff/380001/components/autofill/co... components/autofill/core/browser/credit_card.cc:54: const char kTimeFormatPatternNoYearShortMonthDate[] = "MMMdd"; Can you add a comment giving an example date produced by this pattern (for the future reader)
lgtm https://codereview.chromium.org/2607043002/diff/380001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/380001/components/autofill_st... components/autofill_strings.grdp:232: Exp: <ph name="EXPIRATION_DATE_ABBR">$1<ex>06/17</ex></ph>, added: <ph name="Added_TO_AUTOFILL_MONTH">$2<ex>Jan 10</ex></ph> Should this be ADDED_TO_AUTOFILL_MONTH instead of Added_TO_AUTOFILL_MONTH? https://codereview.chromium.org/2607043002/diff/380001/components/autofill_st... components/autofill_strings.grdp:236: Added: <ph name="Added_TO_AUTOFILL_MONTH">$1<ex>Jan 10</ex></ph> ditto
The CQ bit was checked by jiahuiguo@google.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.
Thanks for all the comments. Sending to CQ. https://codereview.chromium.org/2607043002/diff/380001/components/autofill/co... File components/autofill/core/browser/credit_card.cc (right): https://codereview.chromium.org/2607043002/diff/380001/components/autofill/co... components/autofill/core/browser/credit_card.cc:54: const char kTimeFormatPatternNoYearShortMonthDate[] = "MMMdd"; On 2017/02/12 20:42:05, Mathieu Perreault wrote: > Can you add a comment giving an example date produced by this pattern (for the > future reader) Done. https://codereview.chromium.org/2607043002/diff/380001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2607043002/diff/380001/components/autofill_st... components/autofill_strings.grdp:232: Exp: <ph name="EXPIRATION_DATE_ABBR">$1<ex>06/17</ex></ph>, added: <ph name="Added_TO_AUTOFILL_MONTH">$2<ex>Jan 10</ex></ph> On 2017/02/13 19:15:49, Jared Saul wrote: > Should this be ADDED_TO_AUTOFILL_MONTH instead of Added_TO_AUTOFILL_MONTH? Done. https://codereview.chromium.org/2607043002/diff/380001/components/autofill_st... components/autofill_strings.grdp:236: Added: <ph name="Added_TO_AUTOFILL_MONTH">$1<ex>Jan 10</ex></ph> On 2017/02/13 19:15:49, Jared Saul wrote: > ditto Done.
The CQ bit was checked by jiahuiguo@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, jshin@chromium.org, mathp@chromium.org, jsaul@google.com Link to the patchset: https://codereview.chromium.org/2607043002/#ps400001 (title: "Addressed comments")
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": 400001, "attempt_start_ts": 1487019861661050,
"parent_rev": "5f02eaeb48fb777c7ce1d5e3b5cab87b7caa3387", "commit_rev":
"43b1225755424b08683d7da6b99f8a695c6c86b4"}
Message was sent while issue was closed.
Description was changed from ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last used date in credit card autofill suggestions. BUG=671880 TEST=TimeFormattingTest.TimeFormatWithPattern(base_unittests), CreditCardTest.GetLastUsedDateForDisplay(components_unittests), PersonalDataManagerTest.*(components_unittests) ========== to ========== Credit Card Autofill Last Used Date Experiment Implemented displaying the last used date in credit card autofill suggestions. BUG=671880 TEST=TimeFormattingTest.TimeFormatWithPattern(base_unittests), CreditCardTest.GetLastUsedDateForDisplay(components_unittests), PersonalDataManagerTest.*(components_unittests) Review-Url: https://codereview.chromium.org/2607043002 Cr-Commit-Position: refs/heads/master@{#450089} Committed: https://chromium.googlesource.com/chromium/src/+/43b1225755424b08683d7da6b99f... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/43b1225755424b08683d7da6b99f... |
