Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903273002:
Fixes formatting of the signed-in setting string on iOS. (Closed)
|
Created:
3 years, 7 months ago by macourteau Modified:
3 years, 7 months ago CC:
anthonyvd, chromium-reviews, gogerald+paymentswatch_chromium.org, ios-reviews_chromium.org, ios-reviews+chrome_chromium.org, mahmadi+paymentswatch_chromium.org, marq+watch_chromium.org, noyau+watch_chromium.org, pkl (ping after 24h if needed), rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixes formatting of the signed-in setting string on iOS.
Also adds a test to ensure that that specific string gets formatted
properly.
R=mahmadi@chromium.org, rouslan@chromium.org
BUG=602666
Review-Url: https://codereview.chromium.org/2903273002 .
Cr-Commit-Position: refs/heads/master@{#474991}
Committed: https://chromium.googlesource.com/chromium/src/+/b2c5e5f71d6b747d62709c6f9c190c1380ce8976
Patch Set 1 #Patch Set 2 : Adds missing dependency. #
Total comments: 2
Patch Set 3 : Uses l10n_util::GetStringFUTF8. #
Total comments: 6
Patch Set 4 : . #
Messages
Total messages: 25 (13 generated)
The CQ bit was checked by macourteau@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...
macourteau@chromium.org changed reviewers: + rouslan@chromium.org
+rouslan for the payments_strings.grdp change. base::StringPrintf seems to expect the same format on iOS as it does on Android, so that's something to keep in mind for strings that are used on both platforms. AFAIK, this is the only payments-related string that gets formatted on iOS right now.
lgtm
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 on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by macourteau@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...
https://codereview.chromium.org/2903273002/diff/20001/ios/chrome/browser/ui/p... File ios/chrome/browser/ui/payments/payment_request_view_controller_unittest.mm (right): https://codereview.chromium.org/2903273002/diff/20001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_view_controller_unittest.mm:208: const std::string unformattedString = l10n_util::GetStringUTF8( Shouldn't you be using l10n_util::GetStringFUTF8(IDS_PAYMENTS_CARD_AND_ADDRESS_SETTINGS_SIGNED_IN, "example@gmail.com") instead?
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/2903273002/diff/20001/ios/chrome/browser/ui/p... File ios/chrome/browser/ui/payments/payment_request_view_controller_unittest.mm (right): https://codereview.chromium.org/2903273002/diff/20001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_view_controller_unittest.mm:208: const std::string unformattedString = l10n_util::GetStringUTF8( On 2017/05/25 19:05:04, ಠ_ಠ wrote: > Shouldn't you be using > l10n_util::GetStringFUTF8(IDS_PAYMENTS_CARD_AND_ADDRESS_SETTINGS_SIGNED_IN, > mailto:"example@gmail.com") instead? Sure. Seems like it's _not_ using the same format as Android strings when using that API though, so reverted the change to the gdrp file, and changed the code in payment_request_view_controller.mm.
lgtm
lgtm % nits https://codereview.chromium.org/2903273002/diff/80001/ios/chrome/browser/ui/p... File ios/chrome/browser/ui/payments/payment_request_view_controller.mm (right): https://codereview.chromium.org/2903273002/diff/80001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_view_controller.mm:9: #include "base/strings/stringprintf.h" Remove. https://codereview.chromium.org/2903273002/diff/80001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_view_controller.mm:381: base::SysNSStringToUTF16([_dataSource authenticatedAccountName]); I would be more comfortable with using std::string here as well (via base::SysNSStringToUTF8). https://codereview.chromium.org/2903273002/diff/80001/ios/chrome/browser/ui/p... File ios/chrome/browser/ui/payments/payment_request_view_controller_unittest.mm (right): https://codereview.chromium.org/2903273002/diff/80001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_view_controller_unittest.mm:212: base::ASCIIToUTF16("example@gmail.com")); I would prefer to use ASCIIToUTF8 here for consistency, because you seem to be using UTF8 throughout the test file.
https://codereview.chromium.org/2903273002/diff/80001/ios/chrome/browser/ui/p... File ios/chrome/browser/ui/payments/payment_request_view_controller.mm (right): https://codereview.chromium.org/2903273002/diff/80001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_view_controller.mm:9: #include "base/strings/stringprintf.h" On 2017/05/25 19:42:44, ಠ_ಠ wrote: > Remove. Done. https://codereview.chromium.org/2903273002/diff/80001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_view_controller.mm:381: base::SysNSStringToUTF16([_dataSource authenticatedAccountName]); On 2017/05/25 19:42:44, ಠ_ಠ wrote: > I would be more comfortable with using std::string here as well (via > base::SysNSStringToUTF8). There's no variant of GetStringFUTF8 that takes an std::string: https://cs.chromium.org/chromium/src/ui/base/l10n/l10n_util.h?l=138 https://codereview.chromium.org/2903273002/diff/80001/ios/chrome/browser/ui/p... File ios/chrome/browser/ui/payments/payment_request_view_controller_unittest.mm (right): https://codereview.chromium.org/2903273002/diff/80001/ios/chrome/browser/ui/p... ios/chrome/browser/ui/payments/payment_request_view_controller_unittest.mm:212: base::ASCIIToUTF16("example@gmail.com")); On 2017/05/25 19:42:44, ಠ_ಠ wrote: > I would prefer to use ASCIIToUTF8 here for consistency, because you seem to be > using UTF8 throughout the test file. ditto.
The CQ bit was checked by macourteau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, mahmadi@chromium.org Link to the patchset: https://codereview.chromium.org/2903273002/#ps100001 (title: ".")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Fixes formatting of the signed-in setting string on iOS. Also adds a test to ensure that that specific string gets formatted properly. R=mahmadi@chromium.org BUG=602666 ========== to ========== Fixes formatting of the signed-in setting string on iOS. Also adds a test to ensure that that specific string gets formatted properly. R=mahmadi@chromium.org, rouslan@chromium.org BUG=602666 Review-Url: https://codereview.chromium.org/2903273002 . Cr-Commit-Position: refs/heads/master@{#474991} Committed: https://chromium.googlesource.com/chromium/src/+/b2c5e5f71d6b747d62709c6f9c19... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001) manually as b2c5e5f71d6b747d62709c6f9c190c1380ce8976 (presubmit successful).
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:100001) has been created in https://codereview.chromium.org/2907883002/ by macourteau@chromium.org. The reason for reverting is: Seems like this broke ios-simulator tests on the waterfall. Reverting and will re-land once fixed.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
