Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1540423004:
Add card details and legal message to Android save credit card infobar. (Closed)
|
Created:
5 years ago by please use gerrit instead Modified:
4 years, 11 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, tfarina, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, bondd+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd card details and legal message to Android save credit card infobar.
Based on the mocks at:
https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI_Upstream#%2F11-updated-mobile%20prompts%20-%20proposal.png
This patch also updates the AutofillClient interface to pass in the
CreditCard object into ConfirmSaveCard*() methods. This allows the UI to
show some card information to the user.
TBR=estade@chromium.org
BUG=535784
Committed: https://crrev.com/acfb6cffa322cac5195cc49f573809f7426061e8
Cr-Commit-Position: refs/heads/master@{#369521}
Patch Set 1 : Initial draft #
Total comments: 18
Patch Set 2 : Edits #
Total comments: 39
Patch Set 3 : Addressed Justin's nits. #
Total comments: 21
Patch Set 4 : Mobile specific infobar delegate. #
Total comments: 58
Patch Set 5 : Fix android compiles #
Total comments: 12
Patch Set 6 : Allow copying LegalMessageLine. #
Total comments: 6
Patch Set 7 : Parameter pack version of emplace_back #Patch Set 8 : Fix android 64-bit compile. #
Total comments: 6
Patch Set 9 : Use size_t to init a range. #Patch Set 10 : components/autofill review. #Messages
Total messages: 84 (42 generated)
dfalcantara@chromium.org changed reviewers: + dfalcantara@chromium.org
I'd recommend against adding support for descriptions and details to the C++ InfoBarDelegate -- especially because other platforms aren't making any use of it (any time soon?) and because the way it's used enforces a strict ordering on how everything is laid out. To support this on the Java side, I'd suggest doing what SavePasswordInfoBar[Delegate] does and subclass the Java ConfirmInfoBar, then have your new subclass add controls as needed. https://codereview.chromium.org/1540423004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java (right): https://codereview.chromium.org/1540423004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:284: if (mDetails != null) { You should probably use only one InfoBarControlLayout and repeatedly call addIcon()/addDescription() to it. It'll result in many fewer onMeasure() and onLayout() calls. That said, this isn't the right place to do this because it'll automatically put all descriptions and details above any other controls. Adding specific controls like this is best left to subclasses because InfoBars are so free-form.
Description was changed from ========== Add card details and legal message to Android save credit card infobar. Based on the mocks at: https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI... BUG=535784 ========== to ========== Add card details and legal message to Android save credit card infobar. Based on the mocks at: https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI... BUG=535784 ==========
rouslan@chromium.org changed reviewers: + jdonnelly@chromium.org
On 2015/12/29 23:46:14, dfalcantara wrote: > I'd recommend against adding support for descriptions and details to the C++ > InfoBarDelegate -- especially because other platforms aren't making any use of > it (any time soon?) FYI, Justin will update the iOS infobar to use this information in the near future.
On 2015/12/30 16:31:48, Rouslan wrote: > On 2015/12/29 23:46:14, dfalcantara wrote: > > I'd recommend against adding support for descriptions and details to the C++ > > InfoBarDelegate -- especially because other platforms aren't making any use of > > it (any time soon?) > > > FYI, Justin will update the iOS infobar to use this information in the near > future. Yes, it'll be useful on iOS in the C++ delegate soon. But I'd I tend to agree that putting this stuff in a general class is unlikely to pay off. The odds that something in the future is going to be able to use the exact same data structures and layout seems low. I'd propose just putting the new data in autofill_cc_infobar_delegate.h and adding a UI subclass that uses it.
https://codereview.chromium.org/1540423004/diff/120001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.h (right): https://codereview.chromium.org/1540423004/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.h:61: const base::Closure& callback, As long as we're changing the signature of these methods, can you make the callback the last parameter? As best I can tell, this is the overwhelmingly common pattern: https://code.google.com/p/chromium/codesearch#search/&q=callback&%20-file... https://codereview.chromium.org/1540423004/diff/120001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right): https://codereview.chromium.org/1540423004/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:9: #include "base/i18n/message_formatter.h" Remove this and I think the next two lines as well. https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:79: legal_messages_.back().text = line.text(); When we discussed copying LegalMessage into another format, I thought there was some existing class you were considering copying to. Since there isn't some existing thing and you moved LegalMessage into components (nice), seems to me you should just use that in the delegate. https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_cc_infobar_delegate.h:36: const base::Closure& save_card_callback, Likewise I think we should maintain the callback as the last parameter in these methods. https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_client.h:124: const CreditCard& card) = 0; You'll need to also make these changes to aw_autofill_client.h. https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... File components/autofill/core/browser/credit_card.cc (right): https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... components/autofill/core/browser/credit_card.cc:501: base::string16 CreditCard::TypeAndLastFourDigits() const { Change to TypeAndLastFourDigitsForDisplay for consistency. https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... File components/autofill/core/browser/legal_message_line.h (right): https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... components/autofill/core/browser/legal_message_line.h:38: private: Add DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/1540423004/diff/120001/components/infobars/co... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/1540423004/diff/120001/components/infobars/co... components/infobars/core/infobar_delegate.h:90: struct Description { If we keep this, how about FooterText as a, um, more *descriptive* name than Description?
rouslan@chromium.org changed reviewers: + estade@chromium.org
Evan, do you have an opinion on base class vs. subclass? I was going to follow Dan and Justin's advice on using the subclass, but wanted to hear your opinion first.
If you're polling for opinions on InfoBarDelegate, you really should be asking pkasting@.
dfalcantara@chromium.org changed reviewers: + pkasting@google.com
+pkasting@ For broader context than the mocks in the CL description, here's the current, future-proofed (i.e. good for about three months) specs from the UX team on Android infobars: https://goto.google.com/infobar-spec Changes to the C++ InfoBarDelegate aside, I have to block any change that forces a specific arrangement of the controls in the base Java InfoBar UI classes. The spec is explicitly defined to allow infobars to be built from standardized components in any arrangement. At this point, infobars are basically full-on dialogs that hug the bottom of the screen.
Don't put this stuff in InfoBarDelegate. We can always refactor later to make things more generic, more cross-platform, etc. when we discover that everyone needs that sort of thing. But I don't think InfoBarDelegate should hold any data other than what all infobars on all platforms need. Define an Android or mobile-specific subclass to this delegate and have things inherit from that. Better yet, don't call these sort of complex, configurable dialogs "infobars" at all; make them their own thing. A container that can hold an arbitrary variety of subcomponents is not an infobar IMO. It's a generic UI surface.
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #1 (id:100001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #2 (id:240001) has been deleted
rouslan@chromium.org changed reviewers: + pkasting@chromium.org - pkasting@google.com
Patchset #2 (id:260001) has been deleted
Patchset #2 (id:280001) has been deleted
Patchset #2 (id:300001) has been deleted
Patchset #2 (id:320001) has been deleted
Patchset #2 (id:340001) has been deleted
Patchset #2 (id:360001) has been deleted
Patchset #2 (id:380001) has been deleted
Patchset #2 (id:400001) has been deleted
Justin, PTAL patch 2 overall. Dan, PTAL in patch 2: chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBar.java chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java chrome/browser/ui/android/infobars/autofill_cc_infobar.h chrome/browser/ui/android/infobars/autofill_cc_infobar.cc I will send out an OWNERS review request once we all agree that this looks right. On 2015/12/30 22:50:20, Peter Kasting wrote: > Don't put this stuff in InfoBarDelegate. I've placed it into AutofillCCInfoBarDelegate instead. This class is used for both iOS and Android for saving credit cards, so it should be the right place for it. https://codereview.chromium.org/1540423004/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java (right): https://codereview.chromium.org/1540423004/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/InfoBar.java:284: if (mDetails != null) { On 2015/12/29 23:46:13, dfalcantara wrote: > You should probably use only one InfoBarControlLayout and repeatedly call > addIcon()/addDescription() to it. It'll result in many fewer onMeasure() and > onLayout() calls. > > That said, this isn't the right place to do this because it'll automatically put > all descriptions and details above any other controls. Adding specific controls > like this is best left to subclasses because InfoBars are so free-form. Done. https://codereview.chromium.org/1540423004/diff/120001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.h (right): https://codereview.chromium.org/1540423004/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.h:61: const base::Closure& callback, On 2015/12/30 18:06:37, Justin Donnelly wrote: > As long as we're changing the signature of these methods, can you make the > callback the last parameter? As best I can tell, this is the overwhelmingly > common pattern: > > https://code.google.com/p/chromium/codesearch#search/&q=callback&%20-file... Done. https://codereview.chromium.org/1540423004/diff/120001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right): https://codereview.chromium.org/1540423004/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:9: #include "base/i18n/message_formatter.h" On 2015/12/30 18:06:37, Justin Donnelly wrote: > Remove this and I think the next two lines as well. Done. https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:79: legal_messages_.back().text = line.text(); On 2015/12/30 18:06:37, Justin Donnelly wrote: > When we discussed copying LegalMessage into another format, I thought there was > some existing class you were considering copying to. Since there isn't some > existing thing and you moved LegalMessage into components (nice), seems to me > you should just use that in the delegate. Done. https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_cc_infobar_delegate.h (right): https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_cc_infobar_delegate.h:36: const base::Closure& save_card_callback, On 2015/12/30 18:06:37, Justin Donnelly wrote: > Likewise I think we should maintain the callback as the last parameter in these > methods. Done. https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_client.h:124: const CreditCard& card) = 0; On 2015/12/30 18:06:37, Justin Donnelly wrote: > You'll need to also make these changes to aw_autofill_client.h. Done. https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... File components/autofill/core/browser/credit_card.cc (right): https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... components/autofill/core/browser/credit_card.cc:501: base::string16 CreditCard::TypeAndLastFourDigits() const { On 2015/12/30 18:06:37, Justin Donnelly wrote: > Change to TypeAndLastFourDigitsForDisplay for consistency. Done. https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... File components/autofill/core/browser/legal_message_line.h (right): https://codereview.chromium.org/1540423004/diff/120001/components/autofill/co... components/autofill/core/browser/legal_message_line.h:38: private: On 2015/12/30 18:06:37, Justin Donnelly wrote: > Add DISALLOW_COPY_AND_ASSIGN. Done. https://codereview.chromium.org/1540423004/diff/120001/components/infobars/co... File components/infobars/core/infobar_delegate.h (right): https://codereview.chromium.org/1540423004/diff/120001/components/infobars/co... components/infobars/core/infobar_delegate.h:90: struct Description { Not keeping this.
LGTM with nits https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/infobar... File chrome/browser/infobars/infobar_service.h (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/infobar... chrome/browser/infobars/infobar_service.h:65: scoped_ptr<infobars::InfoBar> CreateAutofillCCInfoBar( As we discussed offline, maybe this should have a platform ifdef instead of implementations on the other platforms that just delegate to CreateConfirmInfobar? Either seems fine to me, I guess we can let the OWNERS decide. https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_cc_infobar.cc (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.cc:50: base::android::ScopedJavaLocalRef<jobject> java_delegate = Move this to where you declare cc_delegate below since you don't use it in these first few lines. https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_cc_infobar.h (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.h:8: #include <jni.h> Forward declare JNIEnv instead of this include? https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/save_card_bubble_controller.h (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/auto... chrome/browser/ui/autofill/save_card_bubble_controller.h:40: virtual const std::vector<scoped_ptr<LegalMessageLine>>& Why scoped_ptr? What was wrong with the vector of values? Regardless, I think you should include legal_message_line.h and stick with "LegalMessageLines" since that's used everywhere else. https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm:162: return CreateConfirmInfoBar(std::move(delegate)); Add a TODO(jdonnelly) to add an iOS CC implementation. https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/infobars/confirm_infobar.cc (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/confirm_infobar.cc:29: return CreateConfirmInfoBar(std::move(delegate)); If we keep this, add a comment explaining that Views uses a bubble instead of an infobar for credit cards.
Peter, PTAL everything infobar related in patch 3: chrome/browser/infobars/infobar_service.cc chrome/browser/infobars/infobar_service.h chrome/browser/ui/android/infobars/autofill_cc_infobar.cc chrome/browser/ui/android/infobars/autofill_cc_infobar.h chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm chrome/browser/ui/views/infobars/confirm_infobar.cc components/autofill/core/browser/autofill_cc_infobar_delegate.cc components/autofill/core/browser/autofill_cc_infobar_delegate.h components/infobars/core/infobar_manager.h ios/chrome/browser/infobars/infobar_manager_impl.cc ios/chrome/browser/infobars/infobar_manager_impl.h Evan, PTAL everything autofill related in patch 3: android_webview/native/aw_autofill_client.cc android_webview/native/aw_autofill_client.h chrome/browser/autofill/android/personal_data_manager_android.cc chrome/browser/ui/autofill/chrome_autofill_client.cc chrome/browser/ui/autofill/chrome_autofill_client.h chrome/browser/ui/autofill/data_model_wrapper.cc chrome/browser/ui/autofill/new_credit_card_bubble_controller.cc chrome/browser/ui/autofill/save_card_bubble_controller.cc chrome/browser/ui/autofill/save_card_bubble_controller.h chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc chrome/browser/ui/autofill/save_card_bubble_controller_impl.h chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc chrome/browser/ui/views/autofill/save_card_bubble_views.cc components/autofill/content/browser/wallet/full_wallet.cc components/autofill/core/browser/autofill_cc_infobar_delegate.cc components/autofill/core/browser/autofill_cc_infobar_delegate.h components/autofill/core/browser/autofill_client.h components/autofill/core/browser/autofill_manager.cc components/autofill/core/browser/autofill_manager_unittest.cc components/autofill/core/browser/credit_card.cc components/autofill/core/browser/credit_card.h components/autofill/core/browser/credit_card_unittest.cc components/autofill/core/browser/legal_message_line.cc components/autofill/core/browser/legal_message_line.h components/autofill/core/browser/legal_message_line_unittest.cc components/autofill/core/browser/personal_data_manager.cc components/autofill/core/browser/test_autofill_client.cc components/autofill/core/browser/test_autofill_client.h components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.cc components/autofill/ios/browser/credit_card_util.mm components/autofill_strings.grdp ios/chrome/browser/ui/autofill/autofill_client_ios.h ios/chrome/browser/ui/autofill/autofill_client_ios.mm https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/infobar... File chrome/browser/infobars/infobar_service.h (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/infobar... chrome/browser/infobars/infobar_service.h:65: scoped_ptr<infobars::InfoBar> CreateAutofillCCInfoBar( On 2016/01/07 18:15:40, Justin Donnelly wrote: > As we discussed offline, maybe this should have a platform ifdef instead of > implementations on the other platforms that just delegate to > CreateConfirmInfobar? > > Either seems fine to me, I guess we can let the OWNERS decide. Acknowledged. https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_cc_infobar.cc (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.cc:50: base::android::ScopedJavaLocalRef<jobject> java_delegate = On 2016/01/07 18:15:40, Justin Donnelly wrote: > Move this to where you declare cc_delegate below since you don't use it in these > first few lines. Done. https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_cc_infobar.h (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.h:8: #include <jni.h> On 2016/01/07 18:15:40, Justin Donnelly wrote: > Forward declare JNIEnv instead of this include? This include also provides jobject and jstring. I don't think I can forward declare those. https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/save_card_bubble_controller.h (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/auto... chrome/browser/ui/autofill/save_card_bubble_controller.h:40: virtual const std::vector<scoped_ptr<LegalMessageLine>>& On 2016/01/07 18:15:40, Justin Donnelly wrote: > Why scoped_ptr? What was wrong with the vector of values? Cannot use vector of values for LegalMessageLine, because we've marked those objets uncopiable via DISALLOW_COPY_AND_ASSIGN. std::vector makes copies of objects, so it requires copyable objects. The new exception is movable types, like scoped_ptr: std::vector<scoped_ptr<>> is now legitimate. See http://crbug.com/554289. > Regardless, I think you should include legal_message_line.h and stick with > "LegalMessageLines" since that's used everywhere else. Done. https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/infobars/confirm_infobar_controller.mm:162: return CreateConfirmInfoBar(std::move(delegate)); On 2016/01/07 18:15:40, Justin Donnelly wrote: > Add a TODO(jdonnelly) to add an iOS CC implementation. Done. https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/view... File chrome/browser/ui/views/infobars/confirm_infobar.cc (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/view... chrome/browser/ui/views/infobars/confirm_infobar.cc:29: return CreateConfirmInfoBar(std::move(delegate)); On 2016/01/07 18:15:40, Justin Donnelly wrote: > If we keep this, add a comment explaining that Views uses a bubble instead of an > infobar for credit cards. Done.
https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBar.java (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBar.java:116: */ nit: Don't need a javadoc here. https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBar.java:127: for (final LegalMessageLine.Link link : line.links) { nit: don't need a final here. https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java:17: public class AutofillCCInfoBarDelegate implements AutofillCCInfoBar.LegalMessageLinkClickDelegate { This class should just be merged into the AutofillCCInfoBar. It'd detangle the JNI wrangling you're doing and make it considerably less confusing. I'm guessing you mimicked what ConfirmInfoBarDelegate.java does, but in its current state, ConfirmInfoBarDelegate.java only provides a inefficient and convoluted way to call a static method and should be nuked. + Move the pointer to the native AutofillCCInfoBar in the Java AutofillCCInfoBar where it makes the most sense. + Move the member fields there, too. + Add a method to AutofillCCInfoBar.java to create an AutofillCCInfoBar: private static AutofillCCInfoBar @CalledByNative create(); + Any calls in your CL that would call into the AutofillCCInfoBarDelegate call into the AutofillCCInfoBar that you created, instead. https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java:19: private LinkedList<AutofillCCInfoBar.CardDetail> mCardDetails = private final? https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java:29: * Creates and returns a new instance of the delegate. Should be called first. nit: newlines between the function description and the @params https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java:49: AutofillCCInfoBar.CardDetail detail = new AutofillCCInfoBar.CardDetail(); Should probably add constructors to each of the AutofillCCInfoBar.* classes instead of doing this here. https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java:107: */ nit: You generally don't need to add a javadoc if it's an overridden function and it doesn't do anything crazy. https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java:113: private native void nativeOnLegalMessageLinkClicked(long nativeAutofillCCInfoBar, String url); There's some extra convoluted routing going on here: you're in the Java AutofillCCInfoBarDelegate, but this function defines the existence of a particular function in the C++ AutofillCCInfoBar. This class actually doesn't even have a JNI counterpart. Move this function to the AutofillCCInfoBar instead, which will avoid the need for your LegalMessageLinkClicked delegate. https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/android... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/android... chrome/browser/android/chrome_jni_registrar.cc:203: {"AutofillCCInfoBarDelegate", AutofillCCInfoBar::Register}, Use the correct name: AutofillCCInfoBar, not AutofillCCInfoBarDelegate. https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_cc_infobar.cc (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.cc:76: for (const auto& line : cc_delegate->legal_messages()) { Hopping back and forth across the JNI barrier can get very expensive. How many times do you expect this to loop, on average? Does it make sense to pass java arrays, instead (see base/android/jni_array.h)? https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_cc_infobar.h (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.h:22: static bool Register(JNIEnv* env); Move to the bottom, right before private? https://codereview.chromium.org/1540423004/diff/420001/components/autofill/co... File components/autofill/core/browser/legal_message_line_unittest.cc (right): https://codereview.chromium.org/1540423004/diff/420001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:32: // Returns true if lines are the same. nit: Private & static? https://codereview.chromium.org/1540423004/diff/420001/components/infobars/co... File components/infobars/core/infobar_manager.h (right): https://codereview.chromium.org/1540423004/diff/420001/components/infobars/co... components/infobars/core/infobar_manager.h:111: // Returns an autofill credit card saving infobar that owns |delegate|. Is there no better way to do this? I'm not sure why this has to be added only for this one particular infobar when the other infobar types have gotten by without it.
https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_cc_infobar.cc (right): https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.cc:43: static_cast<autofill::AutofillCCInfoBarDelegate*>(GetDelegate()) You call GetDelegate() twice in this class, and both places you really want an AutofillCCInfoBarDelegate. I think you should define a GetDelegate() specific to this class that returns that. https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_cc_infobar.h (right): https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.h:21: // Register the JNI bindings. Function comments should be declarative ("Registers"), not imperative. https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.h:22: static bool Register(JNIEnv* env); Nit: Per Google style guide, static methods go after the constructor. https://codereview.chromium.org/1540423004/diff/440001/components/autofill/co... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/1540423004/diff/440001/components/autofill/co... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:77: if (legal_message) { Nit: {} unnecessary https://codereview.chromium.org/1540423004/diff/440001/components/infobars/co... File components/infobars/core/infobar_manager.h (right): https://codereview.chromium.org/1540423004/diff/440001/components/infobars/co... components/infobars/core/infobar_manager.h:111: // Returns an autofill credit card saving infobar that owns |delegate|. There has to be a better place for this. This sort of very specific method doesn't belong in a general class such as InfoBarManager. It's doubly concerning that major platforms like views don't even use an infobar for this. It feels like the entire idea that there is a specific "autofill infobar" shouldn't be in components/. I'm also looking at the stuff added to AutofillCCInfoBarDelegate and wondering if it's ever going to be used outside this platform-specific AutofillInfoBar. I'm beginning to think that class shouldn't be changed either, and we should either have some kind of subclass or some entirely unrelated class to handle the Android-specific needs here.
https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_cc_infobar.cc (right): https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.cc:34: AutofillCCInfoBar::AutofillCCInfoBar( s/CC/Cc https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: for (const auto& line : controller_->GetLegalMessageLines()) { nit: only use auto for long and complicated types (i.e. those with generics), otherwise it reduces readability. https://codereview.chromium.org/1540423004/diff/440001/components/autofill/co... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/1540423004/diff/440001/components/autofill/co... components/autofill/core/browser/credit_card.h:113: base::string16 LastFourDigitsForDisplay() const; Can you leave these functions alone and just prepend the midline ellipsis to LastFourDigits in the (2) places you want it? https://codereview.chromium.org/1540423004/diff/440001/components/autofill/co... components/autofill/core/browser/credit_card.h:116: // A label for this credit card formatted as 'IssuerName ***2345'. asterisk is not correct
https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: for (const auto& line : controller_->GetLegalMessageLines()) { On 2016/01/08 03:14:12, Evan Stade wrote: > nit: only use auto for long and complicated types (i.e. those with generics), > otherwise it reduces readability. I think using auto here is a win. I also wouldn't generalize as "only for long and complicated types". auto can be a win almost anywhere if the actual underlying type is irrelevant to the reader. Here knowing the type of |line| buys you nothing. The Google style guide intentionally avoids specifics like "long and complicated" and instead uses this sentence: "Use auto to avoid type names that are just clutter." There are specific examples given, and a typename with generics in it is one, but not the only one. It's a judgment call for sure though.
https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: for (const auto& line : controller_->GetLegalMessageLines()) { On 2016/01/08 03:18:43, Peter Kasting wrote: > On 2016/01/08 03:14:12, Evan Stade wrote: > > nit: only use auto for long and complicated types (i.e. those with generics), > > otherwise it reduces readability. > > I think using auto here is a win. > > I also wouldn't generalize as "only for long and complicated types". auto can > be a win almost anywhere if the actual underlying type is irrelevant to the > reader. Here knowing the type of |line| buys you nothing. > > The Google style guide intentionally avoids specifics like "long and > complicated" and instead uses this sentence: "Use auto to avoid type names that > are just clutter." There are specific examples given, and a typename with > generics in it is one, but not the only one. > > It's a judgment call for sure though. You think for (const auto& line : controller_->GetLegalMessageLines()) is more readable than for (const LegalMessageLine& line : controller_->GetLegalMessageLines()) ? the difference in length is minimal, but the auto makes my brain work hard to figure out what it resolves to. I think auto is a dangerous tool in that regard; I like strongly typed languages and this mimics weak typing.
https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: for (const auto& line : controller_->GetLegalMessageLines()) { On 2016/01/08 03:23:57, Evan Stade wrote: > On 2016/01/08 03:18:43, Peter Kasting wrote: > > On 2016/01/08 03:14:12, Evan Stade wrote: > > > nit: only use auto for long and complicated types (i.e. those with > generics), > > > otherwise it reduces readability. > > > > I think using auto here is a win. > > > > I also wouldn't generalize as "only for long and complicated types". auto can > > be a win almost anywhere if the actual underlying type is irrelevant to the > > reader. Here knowing the type of |line| buys you nothing. > > > > The Google style guide intentionally avoids specifics like "long and > > complicated" and instead uses this sentence: "Use auto to avoid type names > that > > are just clutter." There are specific examples given, and a typename with > > generics in it is one, but not the only one. > > > > It's a judgment call for sure though. > > You think > > for (const auto& line : controller_->GetLegalMessageLines()) > > is more readable than > > for (const LegalMessageLine& line : controller_->GetLegalMessageLines()) > > ? Yes. I care that I'm getting a line from GetLegalMessageLines(). The name |line| tells me everything I need to know about what the line is; the precise typename doesn't help me and simply obscures the details I care about. It's not a huge difference, but I definitely prefer auto. > the difference in length is minimal, but the auto makes my brain work hard to > figure out what it resolves to. I think auto is a dangerous tool in that regard; > I like strongly typed languages and this mimics weak typing. You may be swimming upstream on this one; I think auto usage will continue to spread widely. In a way it does mimic weak typing -- it gets the brevity benefits of weak typing but without having to use an actual weakly-typed-everywhere language. To me that's a good thing, and from what I've seen around the codebase, I'm not the only one.
Patchset #4 (id:460001) has been deleted
Patchset #4 (id:480001) has been deleted
Patchset #4 (id:500001) has been deleted
Patchset #4 (id:520001) has been deleted
Dan, Evan, Peter: PTAL patch 4. https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBar.java (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBar.java:116: */ On 2016/01/08 00:08:09, dfalcantara wrote: > nit: Don't need a javadoc here. Done. https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBar.java:127: for (final LegalMessageLine.Link link : line.links) { On 2016/01/08 00:08:09, dfalcantara wrote: > nit: don't need a final here. I need the final because "link" is accessed from within the inner class "ClickableSpan". https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java:17: public class AutofillCCInfoBarDelegate implements AutofillCCInfoBar.LegalMessageLinkClickDelegate { On 2016/01/08 00:08:09, dfalcantara wrote: > This class should just be merged into the AutofillCCInfoBar. It'd detangle the > JNI wrangling you're doing and make it considerably less confusing. > > I'm guessing you mimicked what ConfirmInfoBarDelegate.java does, but in its > current state, ConfirmInfoBarDelegate.java only provides a inefficient and > convoluted way to call a static method and should be nuked. > > + Move the pointer to the native AutofillCCInfoBar in the Java AutofillCCInfoBar > where it makes the most sense. > > + Move the member fields there, too. > > + Add a method to AutofillCCInfoBar.java to create an AutofillCCInfoBar: > private static AutofillCCInfoBar @CalledByNative create(); > > + Any calls in your CL that would call into the AutofillCCInfoBarDelegate call > into the AutofillCCInfoBar that you created, instead. Done. https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java:19: private LinkedList<AutofillCCInfoBar.CardDetail> mCardDetails = On 2016/01/08 00:08:09, dfalcantara wrote: > private final? Done. https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java:29: * Creates and returns a new instance of the delegate. Should be called first. On 2016/01/08 00:08:09, dfalcantara wrote: > nit: newlines between the function description and the @params Done. https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java:49: AutofillCCInfoBar.CardDetail detail = new AutofillCCInfoBar.CardDetail(); On 2016/01/08 00:08:09, dfalcantara wrote: > Should probably add constructors to each of the AutofillCCInfoBar.* classes > instead of doing this here. Done. https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java:107: */ On 2016/01/08 00:08:09, dfalcantara wrote: > nit: You generally don't need to add a javadoc if it's an overridden function > and it doesn't do anything crazy. Done. https://codereview.chromium.org/1540423004/diff/420001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillCCInfoBarDelegate.java:113: private native void nativeOnLegalMessageLinkClicked(long nativeAutofillCCInfoBar, String url); On 2016/01/08 00:08:09, dfalcantara wrote: > There's some extra convoluted routing going on here: you're in the Java > AutofillCCInfoBarDelegate, but this function defines the existence of a > particular function in the C++ AutofillCCInfoBar. This class actually doesn't > even have a JNI counterpart. > > Move this function to the AutofillCCInfoBar instead, which will avoid the need > for your LegalMessageLinkClicked delegate. Done. https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/android... File chrome/browser/android/chrome_jni_registrar.cc (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/android... chrome/browser/android/chrome_jni_registrar.cc:203: {"AutofillCCInfoBarDelegate", AutofillCCInfoBar::Register}, On 2016/01/08 00:08:09, dfalcantara wrote: > Use the correct name: AutofillCCInfoBar, not AutofillCCInfoBarDelegate. Done. https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_cc_infobar.cc (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.cc:76: for (const auto& line : cc_delegate->legal_messages()) { On 2016/01/08 00:08:09, dfalcantara wrote: > Hopping back and forth across the JNI barrier can get very expensive. How many > times do you expect this to loop, on average? Does it make sense to pass java > arrays, instead (see base/android/jni_array.h)? I expect the outer to loop 1 time. inner: 2 times. Probably not worth it to pass arrays. What do you think? https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_cc_infobar.h (right): https://codereview.chromium.org/1540423004/diff/420001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.h:22: static bool Register(JNIEnv* env); On 2016/01/08 00:08:09, dfalcantara wrote: > Move to the bottom, right before private? Done. https://codereview.chromium.org/1540423004/diff/420001/components/autofill/co... File components/autofill/core/browser/legal_message_line_unittest.cc (right): https://codereview.chromium.org/1540423004/diff/420001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:32: // Returns true if lines are the same. On 2016/01/08 00:08:09, dfalcantara wrote: > nit: Private & static? Done. https://codereview.chromium.org/1540423004/diff/420001/components/infobars/co... File components/infobars/core/infobar_manager.h (right): https://codereview.chromium.org/1540423004/diff/420001/components/infobars/co... components/infobars/core/infobar_manager.h:111: // Returns an autofill credit card saving infobar that owns |delegate|. On 2016/01/08 00:08:09, dfalcantara wrote: > Is there no better way to do this? I'm not sure why this has to be added only > for this one particular infobar when the other infobar types have gotten by > without it. Another way is to add detail_icon(), detail_label(), detail_sub_label(), legal_message_lines()... to confirm_infobar.h. However, infobar owners (e.g. Peter) might disagree on whether confirm_infobar.h should contain mobile-specific elements. https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_cc_infobar.cc (right): https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.cc:34: AutofillCCInfoBar::AutofillCCInfoBar( On 2016/01/08 03:14:12, Evan Stade wrote: > s/CC/Cc Renamed to AutofillSaveCardInfoBar instead. https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.cc:43: static_cast<autofill::AutofillCCInfoBarDelegate*>(GetDelegate()) On 2016/01/08 00:34:31, Peter Kasting wrote: > You call GetDelegate() twice in this class, and both places you really want an > AutofillCCInfoBarDelegate. I think you should define a GetDelegate() specific > to this class that returns that. Done. https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_cc_infobar.h (right): https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.h:21: // Register the JNI bindings. On 2016/01/08 00:34:31, Peter Kasting wrote: > Function comments should be declarative ("Registers"), not imperative. Done. https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_cc_infobar.h:22: static bool Register(JNIEnv* env); On 2016/01/08 00:34:31, Peter Kasting wrote: > Nit: Per Google style guide, static methods go after the constructor. Done. https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: for (const auto& line : controller_->GetLegalMessageLines()) { On 2016/01/08 07:50:59, Peter Kasting wrote: > On 2016/01/08 03:23:57, Evan Stade wrote: > > On 2016/01/08 03:18:43, Peter Kasting wrote: > > > On 2016/01/08 03:14:12, Evan Stade wrote: > > > > nit: only use auto for long and complicated types (i.e. those with > > generics), > > > > otherwise it reduces readability. > > > > > > I think using auto here is a win. > > > > > > I also wouldn't generalize as "only for long and complicated types". auto > can > > > be a win almost anywhere if the actual underlying type is irrelevant to the > > > reader. Here knowing the type of |line| buys you nothing. > > > > > > The Google style guide intentionally avoids specifics like "long and > > > complicated" and instead uses this sentence: "Use auto to avoid type names > > that > > > are just clutter." There are specific examples given, and a typename with > > > generics in it is one, but not the only one. > > > > > > It's a judgment call for sure though. > > > > You think > > > > for (const auto& line : controller_->GetLegalMessageLines()) > > > > is more readable than > > > > for (const LegalMessageLine& line : controller_->GetLegalMessageLines()) > > > > ? > > Yes. I care that I'm getting a line from GetLegalMessageLines(). The name > |line| tells me everything I need to know about what the line is; the precise > typename doesn't help me and simply obscures the details I care about. > > It's not a huge difference, but I definitely prefer auto. > > > the difference in length is minimal, but the auto makes my brain work hard to > > figure out what it resolves to. I think auto is a dangerous tool in that > regard; > > I like strongly typed languages and this mimics weak typing. > > You may be swimming upstream on this one; I think auto usage will continue to > spread widely. In a way it does mimic weak typing -- it gets the brevity > benefits of weak typing but without having to use an actual > weakly-typed-everywhere language. To me that's a good thing, and from what I've > seen around the codebase, I'm not the only one. Thank you both for thoughtful discussion on this topic. I am going to side with Peter here in particular, because "auto" is actually hiding "scoped_ptr<LegalMessageLine>", which is quite long. https://codereview.chromium.org/1540423004/diff/440001/components/autofill/co... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/1540423004/diff/440001/components/autofill/co... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:77: if (legal_message) { On 2016/01/08 00:34:31, Peter Kasting wrote: > Nit: {} unnecessary Done. https://codereview.chromium.org/1540423004/diff/440001/components/autofill/co... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/1540423004/diff/440001/components/autofill/co... components/autofill/core/browser/credit_card.h:113: base::string16 LastFourDigitsForDisplay() const; On 2016/01/08 03:14:12, Evan Stade wrote: > Can you leave these functions alone and just prepend the midline ellipsis to > LastFourDigits in the (2) places you want it? Done. https://codereview.chromium.org/1540423004/diff/440001/components/autofill/co... components/autofill/core/browser/credit_card.h:116: // A label for this credit card formatted as 'IssuerName ***2345'. On 2016/01/08 03:14:12, Evan Stade wrote: > asterisk is not correct Done. https://codereview.chromium.org/1540423004/diff/440001/components/infobars/co... File components/infobars/core/infobar_manager.h (right): https://codereview.chromium.org/1540423004/diff/440001/components/infobars/co... components/infobars/core/infobar_manager.h:111: // Returns an autofill credit card saving infobar that owns |delegate|. On 2016/01/08 00:34:31, Peter Kasting wrote: > There has to be a better place for this. This sort of very specific method > doesn't belong in a general class such as InfoBarManager. It's doubly > concerning that major platforms like views don't even use an infobar for this. > It feels like the entire idea that there is a specific "autofill infobar" > shouldn't be in components/. Created components/autofill/core/browser/autofill_save_card_infobar_mobile.h with CreateSaveCardInfoBarMobile() function. It's defined only on iOS and Android. > I'm also looking at the stuff added to AutofillCCInfoBarDelegate and wondering > if it's ever going to be used outside this platform-specific AutofillInfoBar. > I'm beginning to think that class shouldn't be changed either, and we should > either have some kind of subclass or some entirely unrelated class to handle the > Android-specific needs here. Moved mobile-specific modifications into AutofillSaveCardInfoBarDelegateMobile.
https://codereview.chromium.org/1540423004/diff/420001/components/infobars/co... File components/infobars/core/infobar_manager.h (right): https://codereview.chromium.org/1540423004/diff/420001/components/infobars/co... components/infobars/core/infobar_manager.h:111: // Returns an autofill credit card saving infobar that owns |delegate|. On 2016/01/12 00:47:25, Rouslan wrote: > On 2016/01/08 00:08:09, dfalcantara wrote: > > Is there no better way to do this? I'm not sure why this has to be added only > > for this one particular infobar when the other infobar types have gotten by > > without it. > > Another way is to add detail_icon(), detail_label(), detail_sub_label(), > legal_message_lines()... to confirm_infobar.h. However, infobar owners (e.g. > Peter) might disagree on whether confirm_infobar.h should contain > mobile-specific elements. Ended up making an autofill_save_card_infobar_mobile.h file with CreatSaveCardInfoBarMobile() function, which is defined in iOS and Android specific files.
Android bits l-g-t-m, but you've got some compile problems. https://chromiumcodereview.appspot.com/1540423004/diff/540001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java (right): https://chromiumcodereview.appspot.com/1540423004/diff/540001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java:26: /** nit: member fields come before methods https://chromiumcodereview.appspot.com/1540423004/diff/540001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java:65: /** nit: Ditto above. https://chromiumcodereview.appspot.com/1540423004/diff/540001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java:111: public LinkedList<Link> links = new LinkedList<Link>(); nit: public final List<Link> links = new LinkedList<Link>(); https://chromiumcodereview.appspot.com/1540423004/diff/540001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java:115: private final LinkedList<CardDetail> mCardDetails = new LinkedList<CardDetail>(); nit: List instead of LinkedList for the first one https://chromiumcodereview.appspot.com/1540423004/diff/540001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java:208: for (final LegalMessageLine.Link link : line.links) { don't need the final here, I think. https://chromiumcodereview.appspot.com/1540423004/diff/540001/components/auto... File components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.h (right): https://chromiumcodereview.appspot.com/1540423004/diff/540001/components/auto... components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.h:27: // A mobile infobar delegate for saving credit cards. nit: "An InfoBarDelegate for saving credit cards on mobile devices"? The current one makes it sound like the infobar is mobile. https://chromiumcodereview.appspot.com/1540423004/diff/540001/components/auto... File components/autofill/core/browser/legal_message_line.cc (right): https://chromiumcodereview.appspot.com/1540423004/diff/540001/components/auto... components/autofill/core/browser/legal_message_line.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 https://chromiumcodereview.appspot.com/1540423004/diff/540001/components/auto... File components/autofill/core/browser/legal_message_line.h (right): https://chromiumcodereview.appspot.com/1540423004/diff/540001/components/auto... components/autofill/core/browser/legal_message_line.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 https://chromiumcodereview.appspot.com/1540423004/diff/540001/components/auto... File components/autofill/core/browser/legal_message_line_unittest.cc (right): https://chromiumcodereview.appspot.com/1540423004/diff/540001/components/auto... components/autofill/core/browser/legal_message_line_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016
Patchset #5 (id:560001) has been deleted
Patchset #5 (id:580001) has been deleted
Dan, PTAL patch 5. https://codereview.chromium.org/1540423004/diff/540001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java (right): https://codereview.chromium.org/1540423004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java:26: /** On 2016/01/12 19:04:23, dfalcantara wrote: > nit: member fields come before methods Done. https://codereview.chromium.org/1540423004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java:65: /** On 2016/01/12 19:04:23, dfalcantara wrote: > nit: Ditto above. Done. https://codereview.chromium.org/1540423004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java:111: public LinkedList<Link> links = new LinkedList<Link>(); On 2016/01/12 19:04:23, dfalcantara wrote: > nit: public final List<Link> links = new LinkedList<Link>(); Done. https://codereview.chromium.org/1540423004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java:115: private final LinkedList<CardDetail> mCardDetails = new LinkedList<CardDetail>(); On 2016/01/12 19:04:23, dfalcantara wrote: > nit: List instead of LinkedList for the first one Done. https://codereview.chromium.org/1540423004/diff/540001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/infobar/AutofillSaveCardInfoBar.java:208: for (final LegalMessageLine.Link link : line.links) { On 2016/01/12 19:04:23, dfalcantara wrote: > don't need the final here, I think. Need it to use link inside of the ClickableSpan below. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.h (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.h:27: // A mobile infobar delegate for saving credit cards. On 2016/01/12 19:04:23, dfalcantara wrote: > nit: "An InfoBarDelegate for saving credit cards on mobile devices"? The > current one makes it sound like the infobar is mobile. Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/legal_message_line.cc (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/12 19:04:23, dfalcantara wrote: > nit: 2016 Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/legal_message_line.h (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/12 19:04:23, dfalcantara wrote: > nit: 2016 Hm, technically this s almost copy-pasta. However, it's diverged enough that I can call it a new file, I guess. Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/legal_message_line_unittest.cc (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/01/12 19:04:23, dfalcantara wrote: > nit: 2016 Done.
lgtm % successful compile
Yeah, this is much better structurally. https://codereview.chromium.org/1540423004/diff/540001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_save_card_infobar.h (right): https://codereview.chromium.org/1540423004/diff/540001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_save_card_infobar.h:32: // ConfirmInfoBar override. Nit: "// ConfirmInfoBar:" https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/credit_card.h:118: // The expiration for this credit card formatted as 'Exp: 06/17'. Nit: Might want to be clear about whether this is localized (I assume yes, but your comment doesn't really imply that) https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/legal_message_line.cc (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:38: // Both of these cases are noted in the header file, and are unlikely to What header file? https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:73: out->at(i).reset(new LegalMessageLine); Nit: Prefer (*out)[i] to using at() (2 places) (but see below comment) https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:77: out->clear(); Nit: Instead of using clear() here, do something like this: const base::ListValue* line_list; if (legal_message.GetList("line", &line_list)) { LegalMessageLines lines; lines.reserve(line_list->GetSize()); // Remainder of conditional goes here out->swap(lines); } return true; This lets you avoid the clear() call before returning false, and allows you to use reserve()/emplace_back() for a slight efficiency win (which doesn't really matter in this case). It's also safer if you add more exit points to the function in the future, because we can't forget to clear() before exiting. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:99: links_.resize(template_parameters->GetSize()); Nit: Maybe these should reserve() and the code below emplace_back()? This would only work when we haven't called ParseLine() on |this| before, but it seems like that is, in fact, how this is used. Perhaps to make this more bulletproof, we could replace ParseLine() with a private one-arg constructor, so Parse() above wouldn't construct an empty line and then parse it, it would just construct directly? If we did this, though, we'd still need to be able to signal parsing errors, so we'd need to add a private |valid_| field the caller could read. I don't know whether on net this winds up better or worse than the current code. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:109: !single_parameter->GetString("url", &url)) { Nit: No {} https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:127: &offsets)) { Nit: No {} https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/legal_message_line.h (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.h:41: FRIEND_TEST_ALL_PREFIXES(LegalMessageLineTest, NoParameters); If you have this many friend declarations, consider instead doing something like: friend class LegalMessageLineTest; Then in your unittest file, define this test base class and give it accessors that individual tests can use to access the necessary members of LegalMessageLine. This is not only shorter here, it won't require updating this list every time someone adds or removes a test. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.h:61: typedef std::vector<scoped_ptr<LegalMessageLine>> LegalMessageLines; Nit: Chromium style prefers "using" to "typedef" for new aliases. If you forward-declare the class atop this file, you can then declare this alias, and that will allow you to use it in the declaration of LegalMessageLine::Parse(). https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/legal_message_line_unittest.cc (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:29: ASSERT_TRUE(value->GetAsDictionary(&dictionary)); Nit: This one could probably be EXPECT_TRUE https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:34: // Returns true if lines are the same. Nit: Maybe "Returns whether |lines| consists solely of |line|." (This uses my param renames suggested below) https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:35: static bool CompareLegalMessages(const LegalMessageLine& single_line, Nit: I think it might be possible to declare some file-scoped non-member operator==() methods in the place of these comparison functions? The goal here would be to use EXPECT_EQ() below for better error messages. I think that would also require defining a method to convert a LegalMessageLine to a string (which hopefully could also be defined as a non-member in this file? Not sure), but maybe it's worth it? https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:36: const LegalMessageLines& b) { Nit: |b| -> |lines| (and maybe |single_line| -> |line|) https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:55: const LegalMessageLine& b) { Do we even need this method? It seems like just checking "a == b" in the caller ought to work? https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:74: TEST_F(LegalMessageLineTest, NoParameters) { Nit: It seems kinda like you could condense this file by just having an array of (input string, expected line) pairs and iterating through them. This might require implementing my next suggestion. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:83: expected_line.text_ = base::ASCIIToUTF16("This is the entire message."); Nit: Given the contents of this file, it seems like it might be worth adding a 2-arg constructor that allows you to pass in the text and links. If you want you could make it test-only.
mostly lg https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:222: #endif // defined(OS_ANDROID) || defined(OS_IOS) I never know what these comments mean, whether they're referring to the block above or some block further up the macro conditional chain. As such I think they're actively hurtful. https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: for (const auto& line : controller_->GetLegalMessageLines()) remove auto https://codereview.chromium.org/1540423004/diff/600001/components/autofill/co... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/1540423004/diff/600001/components/autofill/co... components/autofill/core/browser/credit_card.h:119: base::string16 AbbreviatedExpirationDateForDisplay() const; given that this is only used in one place (I think) can we just put it there? Especially as this is only used on mobile.
https://codereview.chromium.org/1540423004/diff/600001/components/autofill/co... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/1540423004/diff/600001/components/autofill/co... components/autofill/core/browser/credit_card.h:119: base::string16 AbbreviatedExpirationDateForDisplay() const; On 2016/01/13 04:09:19, Evan Stade wrote: > given that this is only used in one place (I think) can we just put it there? > Especially as this is only used on mobile. This will also be used on Views, Cocoa and iOS in subsequent changes.
Patchset #6 (id:620001) has been deleted
Patchset #6 (id:640001) has been deleted
rouslan@chromium.org changed reviewers: + boliu@chromium.org, rohitrao@chromium.org
Evan and Peter, PTAL patch 6. Rohit, OWNERS PTAL ios/* in patch 6. Bo, OWNERS PTAL android_webview/* in patch 6. https://codereview.chromium.org/1540423004/diff/540001/chrome/browser/ui/andr... File chrome/browser/ui/android/infobars/autofill_save_card_infobar.h (right): https://codereview.chromium.org/1540423004/diff/540001/chrome/browser/ui/andr... chrome/browser/ui/android/infobars/autofill_save_card_infobar.h:32: // ConfirmInfoBar override. On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: "// ConfirmInfoBar:" Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/credit_card.h:118: // The expiration for this credit card formatted as 'Exp: 06/17'. On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: Might want to be clear about whether this is localized (I assume yes, but > your comment doesn't really imply that) Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/legal_message_line.cc (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:38: // Both of these cases are noted in the header file, and are unlikely to On 2016/01/12 23:09:37, Peter Kasting wrote: > What header file? Fixed. See legal_message_line.h. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:73: out->at(i).reset(new LegalMessageLine); On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: Prefer (*out)[i] to using at() (2 places) (but see below comment) Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:77: out->clear(); On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: Instead of using clear() here, do something like this: > > const base::ListValue* line_list; > if (legal_message.GetList("line", &line_list)) { > LegalMessageLines lines; > lines.reserve(line_list->GetSize()); > // Remainder of conditional goes here > out->swap(lines); > } > return true; > > This lets you avoid the clear() call before returning false, and allows you to > use reserve()/emplace_back() for a slight efficiency win (which doesn't really > matter in this case). It's also safer if you add more exit points to the > function in the future, because we can't forget to clear() before exiting. Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:99: links_.resize(template_parameters->GetSize()); On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: Maybe these should reserve() and the code below emplace_back()? This would > only work when we haven't called ParseLine() on |this| before, but it seems like > that is, in fact, how this is used. Done. > Perhaps to make this more bulletproof, we could replace ParseLine() with a > private one-arg constructor, so Parse() above wouldn't construct an empty line > and then parse it, it would just construct directly? If we did this, though, > we'd still need to be able to signal parsing errors, so we'd need to add a > private |valid_| field the caller could read. I don't know whether on net this > winds up better or worse than the current code. Going to steer away from the pattern of a private member variable "valid_" to keep this code relatively simple. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:109: !single_parameter->GetString("url", &url)) { On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:127: &offsets)) { On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/legal_message_line.h (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.h:41: FRIEND_TEST_ALL_PREFIXES(LegalMessageLineTest, NoParameters); On 2016/01/12 23:09:37, Peter Kasting wrote: > If you have this many friend declarations, consider instead doing something > like: > > friend class LegalMessageLineTest; > > Then in your unittest file, define this test base class and give it accessors > that individual tests can use to access the necessary members of > LegalMessageLine. > > This is not only shorter here, it won't require updating this list every time > someone adds or removes a test. Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line.h:61: typedef std::vector<scoped_ptr<LegalMessageLine>> LegalMessageLines; On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: Chromium style prefers "using" to "typedef" for new aliases. > > If you forward-declare the class atop this file, you can then declare this > alias, and that will allow you to use it in the declaration of > LegalMessageLine::Parse(). Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/legal_message_line_unittest.cc (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:29: ASSERT_TRUE(value->GetAsDictionary(&dictionary)); On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: This one could probably be EXPECT_TRUE Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:34: // Returns true if lines are the same. On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: Maybe "Returns whether |lines| consists solely of |line|." (This uses my > param renames suggested below) Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:35: static bool CompareLegalMessages(const LegalMessageLine& single_line, On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: I think it might be possible to declare some file-scoped non-member > operator==() methods in the place of these comparison functions? The goal here > would be to use EXPECT_EQ() below for better error messages. I think that would > also require defining a method to convert a LegalMessageLine to a string (which > hopefully could also be defined as a non-member in this file? Not sure), but > maybe it's worth it? Unfortunately does not work: error: invalid operands to binary expression ('const autofill::LegalMessageLine' and 'element_type' (aka 'const autofill::LegalMessageLine')) https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:36: const LegalMessageLines& b) { On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: |b| -> |lines| (and maybe |single_line| -> |line|) Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:55: const LegalMessageLine& b) { On 2016/01/12 23:09:37, Peter Kasting wrote: > Do we even need this method? It seems like just checking "a == b" in the caller > ought to work? Unfortunately does not work: error: invalid operands to binary expression ('const autofill::LegalMessageLine' and 'element_type' (aka 'const autofill::LegalMessageLine')) This error message makes little sense to me: I should be able to compare them. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:74: TEST_F(LegalMessageLineTest, NoParameters) { On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: It seems kinda like you could condense this file by just having an array of > (input string, expected line) pairs and iterating through them. This might > require implementing my next suggestion. I've tried both array of test cases and parameterized test. Both of these approaches made the test more complex due to LegalMessageLines being std::vector<scoped_ptr<LegalMessageLine>> instead of plain old std::vector<LegalMessageLine>. Inability to copy LegalMessageLine also does not help. Let's leave the test structure as is. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:83: expected_line.text_ = base::ASCIIToUTF16("This is the entire message."); On 2016/01/12 23:09:37, Peter Kasting wrote: > Nit: Given the contents of this file, it seems like it might be worth adding a > 2-arg constructor that allows you to pass in the text and links. If you want > you could make it test-only. Done. https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:222: #endif // defined(OS_ANDROID) || defined(OS_IOS) On 2016/01/13 04:09:19, Evan Stade wrote: > I never know what these comments mean, whether they're referring to the block > above or some block further up the macro conditional chain. As such I think > they're actively hurtful. Removed. https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: for (const auto& line : controller_->GetLegalMessageLines()) On 2016/01/13 04:09:19, Evan Stade wrote: > remove auto Done. https://codereview.chromium.org/1540423004/diff/600001/components/autofill/co... File components/autofill/core/browser/credit_card.h (right): https://codereview.chromium.org/1540423004/diff/600001/components/autofill/co... components/autofill/core/browser/credit_card.h:119: base::string16 AbbreviatedExpirationDateForDisplay() const; On 2016/01/13 15:32:28, Justin Donnelly wrote: > On 2016/01/13 04:09:19, Evan Stade wrote: > > given that this is only used in one place (I think) can we just put it there? > > Especially as this is only used on mobile. > > This will also be used on Views, Cocoa and iOS in subsequent changes. Acknowledged.
android_webview rs lgtm
https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/legal_message_line_unittest.cc (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:35: static bool CompareLegalMessages(const LegalMessageLine& single_line, On 2016/01/13 21:22:09, Rouslan wrote: > On 2016/01/12 23:09:37, Peter Kasting wrote: > > Nit: I think it might be possible to declare some file-scoped non-member > > operator==() methods in the place of these comparison functions? The goal > here > > would be to use EXPECT_EQ() below for better error messages. I think that > would > > also require defining a method to convert a LegalMessageLine to a string > (which > > hopefully could also be defined as a non-member in this file? Not sure), but > > maybe it's worth it? > > Unfortunately does not work: > > error: invalid operands to binary expression ('const > autofill::LegalMessageLine' and 'element_type' (aka 'const > autofill::LegalMessageLine')) I can't diagnose this without seeing the code you tried to write. I wouldn't give up here; see below. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:55: const LegalMessageLine& b) { On 2016/01/13 21:22:09, Rouslan wrote: > On 2016/01/12 23:09:37, Peter Kasting wrote: > > Do we even need this method? It seems like just checking "a == b" in the > caller > > ought to work? > > Unfortunately does not work: > > error: invalid operands to binary expression ('const > autofill::LegalMessageLine' and 'element_type' (aka 'const > autofill::LegalMessageLine')) > > This error message makes little sense to me: I should be able to compare them. Both this and the above are because you don't have an appropriate operator==() defined. The Google style guide now encourages operator== for cases like this: "Don't go out of your way to avoid defining operator overloads. For example, prefer to define == ... rather than Equals()..." I think it ought to be possible to define == as a non-member, per http://en.cppreference.com/w/cpp/language/operators : bool operator==(const LegalMessageLine& lhs, const LegalMessageLine& rhs); https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:74: TEST_F(LegalMessageLineTest, NoParameters) { On 2016/01/13 21:22:09, Rouslan wrote: > On 2016/01/12 23:09:37, Peter Kasting wrote: > > Nit: It seems kinda like you could condense this file by just having an array > of > > (input string, expected line) pairs and iterating through them. This might > > require implementing my next suggestion. > > I've tried both array of test cases and parameterized test. Both of these > approaches made the test more complex due to LegalMessageLines being > std::vector<scoped_ptr<LegalMessageLine>> instead of plain old > std::vector<LegalMessageLine>. Inability to copy LegalMessageLine also does not > help. Let's leave the test structure as is. Why did you go with vector<scoped_ptr<LegalMessageLine>>, anyway? It seems like vector<LegalMessageLine> is the more logical type. If the only reason is to avoid adding copy/assignment support, you should definitely go ahead and make the class copyable and eliminate the scoped_ptrs. That in turn might make this change feasible. https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: for (const auto& line : controller_->GetLegalMessageLines()) On 2016/01/13 04:09:19, Evan Stade wrote: > remove auto Seems like we debated this already?: https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view...
https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: for (const auto& line : controller_->GetLegalMessageLines()) On 2016/01/13 22:10:25, Peter Kasting wrote: > On 2016/01/13 04:09:19, Evan Stade wrote: > > remove auto > > Seems like we debated this already?: > https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view... Yes, we did, and I abandoned the debate because I don't feel like it's a good use of time to argue over style issues in individual codereview issues. Since Autofill is code I own and continue to be the primary reviewer for, I feel strongly about this subject in this file. FWIW, I agree with the examples provided by the google style guide[1]. One is an iterator type (type specification is over half the length of the line) and one is a repeated type (not informative). Since auto is not here being used to hide a repetitive or even very long type specification, I don't think the style guide explicitly suggests its use. If you think there is a good case for auto here, I'd ask that you take it up with the style guide owners so they can make their advice more specific. And FWIW, using auto here is inconsistent with non-use of auto on L126. [1] https://google.github.io/styleguide/cppguide.html#auto
https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: for (const auto& line : controller_->GetLegalMessageLines()) On 2016/01/13 22:52:40, Evan Stade wrote: > On 2016/01/13 22:10:25, Peter Kasting wrote: > > On 2016/01/13 04:09:19, Evan Stade wrote: > > > remove auto > > > > Seems like we debated this already?: > > > https://codereview.chromium.org/1540423004/diff/440001/chrome/browser/ui/view... > > Yes, we did, and I abandoned the debate because I don't feel like it's a good > use of time to argue over style issues in individual codereview issues. Since > Autofill is code I own and continue to be the primary reviewer for, I feel > strongly about this subject in this file. The previous debate was about this same line in this same file. I'm confused by you saying that you were willing to abandon that debate but not this one, when they're the same debate. > FWIW, I agree with the examples provided by the google style guide[1]. One is an > iterator type (type specification is over half the length of the line) and one > is a repeated type (not informative). Since auto is not here being used to hide > a repetitive or even very long type specification, I don't think the style guide > explicitly suggests its use. But it is used to hide a repetitive and longer type: for (const scoped_ptr<LegalMessageLine>& line : controller_->GetLegalMessageLines()) Here the type specification is over half the line (just like the first example you cited) and causes a line break, reducing readability of the statement. It uses a template, just like the text in the style guide refers to: "...especially when they involve templates or namespaces." Also, the key type information, "<LegalMessageLine>", is also repeated in both the function name being called and partially in the name of the variable, as well as in the name of the call made in the body of the loop. The initialization is also taking place based only on information given in this immediate context; this is in contrast to the text and example given in the styleguide for the "con" scenario: "...especially when a variable's initialization depends on things that were declared far away. In an expression like: 'auto i = x.Lookup(key);' it may not be obvious what i's type is, if x was declared hundreds of lines earlier." So I'm confused why you don't feel this is similar to the style guide examples when it has both the properties you summarize the examples as having, and when it explicitly falls into a case described in the "Pros" section and avoids the one described in the "Cons" section. Finally, as a matter of principle, examples in the style guide are illustrative, not normative. They are not intended to limit style guide applicability to only situations exactly like the examples. The normative text is the ruling text itself, which simply reads: "auto is permitted, for local variables only. Do not use auto for file-scope or namespace-scope variables, or for class members. Never initialize an auto-typed variable with a braced initializer list." > If you think there is a good case for auto here, > I'd ask that you take it up with the style guide owners so they can make their > advice more specific. I don't see how the style guide could be more specific about this. It seems very clearly to fit the situation being described. If you still disagree, I would be willing to briefly query some fellow C++ Readability reviewers for you to see what they think, but it doesn't seem like it ought to be necessary. > And FWIW, using auto here is inconsistent with non-use of auto on L126. Yes, that should ideally use auto as well.
> So I'm confused why you don't feel this is similar to the style guide examples > when it has both the properties you summarize the examples as having, and when > it explicitly falls into a case described in the "Pros" section and avoids the > one described in the "Cons" section. 1. 28 out of 84 characters is not over half 2. there's no repetition of the same type
On 2016/01/13 23:27:43, Evan Stade wrote: > > So I'm confused why you don't feel this is similar to the style guide examples > > when it has both the properties you summarize the examples as having, and when > > it explicitly falls into a case described in the "Pros" section and avoids the > > one described in the "Cons" section. > > 1. 28 out of 84 characters is not over half The full type is 34 characters, and when linebroken to comply with the styleguide that's significantly more than half of the 48 characters in the line. This is what I was referring to. Besides this, the style guide never says "over half", let alone "over half of a non-wrapped version of the line, not including cv- and ref-qualifiers". There is nothing to indicate that this is some sort of bright-line test. > 2. there's no repetition of the same type The text "LegalMessageLine" is repeated two other places, and the text "line" an additional one. Again, there is no text in the style guide referring to repeated phrases, let alone referring to "repeated type names, excluding type names mentioned in other API names etc." There's nothing suggesting that the distinction you're drawing between what you said and what I said is meaningful. Finally, as I tried to indicate before, I've seen a lot of changes go into the codebase already to use auto, and I can't recall anyone else suggesting the sort of narrow reading you're suggesting. It seems like not only is there no explicit text supporting your position, there's not a commonality of other people. This is why I'm confused by your seeming insistence that those who disagree with you are the ones on whom the burden of proof falls. I urge you to reconsider whether this is really an issue on which being so restrictive is beneficial. If after that you would like me to escalate to other Readability reviewers, I will do so for you.
ios/ changes LGTM I was a bit surprised to see ios changes when the CL description was very Android-specific. Is it worth updating the description to have a brief summary of the changes to AutofillClient?
Description was changed from ========== Add card details and legal message to Android save credit card infobar. Based on the mocks at: https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI... BUG=535784 ========== to ========== Add card details and legal message to Android save credit card infobar. Based on the mocks at: https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI... This patch also updates the AutofillClient interface to pass in the CreditCard object into ConfirmSaveCard*() methods. This allows the UI to show some card information to the user. BUG=535784 ==========
Description was changed from ========== Add card details and legal message to Android save credit card infobar. Based on the mocks at: https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI... This patch also updates the AutofillClient interface to pass in the CreditCard object into ConfirmSaveCard*() methods. This allows the UI to show some card information to the user. BUG=535784 ========== to ========== Add card details and legal message to Android save credit card infobar. Based on the mocks at: https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI... This patch also updates the AutofillClient interface to pass in the CreditCard object into ConfirmSaveCard*() methods. This allows the UI to show some card information to the user. BUG=535784 ==========
Patchset #6 (id:660001) has been deleted
Evan and Peter, ptal patch 6. On 2016/01/14 00:12:07, rohitrao wrote: > I was a bit surprised to see ios changes when the CL description was very > Android-specific. Is it worth updating the description to have a brief summary > of the changes to AutofillClient? Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... File components/autofill/core/browser/legal_message_line_unittest.cc (right): https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:35: static bool CompareLegalMessages(const LegalMessageLine& single_line, On 2016/01/13 22:10:25, Peter Kasting wrote: > On 2016/01/13 21:22:09, Rouslan wrote: > > On 2016/01/12 23:09:37, Peter Kasting wrote: > > > Nit: I think it might be possible to declare some file-scoped non-member > > > operator==() methods in the place of these comparison functions? The goal > > here > > > would be to use EXPECT_EQ() below for better error messages. I think that > > would > > > also require defining a method to convert a LegalMessageLine to a string > > (which > > > hopefully could also be defined as a non-member in this file? Not sure), but > > > maybe it's worth it? > > > > Unfortunately does not work: > > > > error: invalid operands to binary expression ('const > > autofill::LegalMessageLine' and 'element_type' (aka 'const > > autofill::LegalMessageLine')) > > I can't diagnose this without seeing the code you tried to write. I wouldn't > give up here; see below. Parameterized. Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:55: const LegalMessageLine& b) { On 2016/01/13 22:10:25, Peter Kasting wrote: > On 2016/01/13 21:22:09, Rouslan wrote: > > On 2016/01/12 23:09:37, Peter Kasting wrote: > > > Do we even need this method? It seems like just checking "a == b" in the > > caller > > > ought to work? > > > > Unfortunately does not work: > > > > error: invalid operands to binary expression ('const > > autofill::LegalMessageLine' and 'element_type' (aka 'const > > autofill::LegalMessageLine')) > > > > This error message makes little sense to me: I should be able to compare them. > > Both this and the above are because you don't have an appropriate operator==() > defined. > > The Google style guide now encourages operator== for cases like this: > > "Don't go out of your way to avoid defining operator overloads. For example, > prefer to define == ... rather than Equals()..." > > I think it ought to be possible to define == as a non-member, per > http://en.cppreference.com/w/cpp/language/operators : > > bool operator==(const LegalMessageLine& lhs, const LegalMessageLine& rhs); Done. https://codereview.chromium.org/1540423004/diff/540001/components/autofill/co... components/autofill/core/browser/legal_message_line_unittest.cc:74: TEST_F(LegalMessageLineTest, NoParameters) { On 2016/01/13 22:10:25, Peter Kasting wrote: > On 2016/01/13 21:22:09, Rouslan wrote: > > On 2016/01/12 23:09:37, Peter Kasting wrote: > > > Nit: It seems kinda like you could condense this file by just having an > array > > of > > > (input string, expected line) pairs and iterating through them. This might > > > require implementing my next suggestion. > > > > I've tried both array of test cases and parameterized test. Both of these > > approaches made the test more complex due to LegalMessageLines being > > std::vector<scoped_ptr<LegalMessageLine>> instead of plain old > > std::vector<LegalMessageLine>. Inability to copy LegalMessageLine also does > not > > help. Let's leave the test structure as is. > > Why did you go with vector<scoped_ptr<LegalMessageLine>>, anyway? It seems like > vector<LegalMessageLine> is the more logical type. If the only reason is to > avoid adding copy/assignment support, you should definitely go ahead and make > the class copyable and eliminate the scoped_ptrs. > > That in turn might make this change feasible. Made LegalMessageLine copyable and my life is so much easier now. https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: for (const auto& line : controller_->GetLegalMessageLines()) Now that LegalMessageLines is a plain old std::vector<LegalMessageLine>, we can replace "auto" with "LegalMessageLine" and keep the single line. I think this is a good compromise, so let's go with it.
LGTM https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/1540423004/diff/600001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:232: for (const auto& line : controller_->GetLegalMessageLines()) On 2016/01/14 02:39:40, Rouslan wrote: > Now that LegalMessageLines is a plain old std::vector<LegalMessageLine>, we can > replace "auto" with "LegalMessageLine" and keep the single line. I think this is > a good compromise, so let's go with it. Yeah, in that situation I think we're good without auto. However, it's worth saying what I was coming here to post before I saw this new patch of yours: I'm reversing myself and think the old code should avoid auto as well. To be clear, I still think auto here is more readable, and compliant with the style guide, so I'm glad we had the discussion, because I hope Evan's outlook changes at some point :). But the readability difference is not enormous, and the style guide clearly _allows_ both behaviors. In that situation, it's ultimately the OWNERS' call what to do. I'm a c/b/ui/ OWNER, but Evan is specifically a c/b/ui/autofill/ OWNER, and his desires should have governed if I didn't have overwhelming reason to reverse. Sorry, Evan, for needing a bit of time away from this thread to recognize that I should have conceded sooner. https://codereview.chromium.org/1540423004/diff/680001/components/autofill/co... File components/autofill/core/browser/legal_message_line.cc (right): https://codereview.chromium.org/1540423004/diff/680001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:117: links_.emplace_back(Link{gfx::Range(), GURL(url)}); Nit: Can you just do: links_.emplace_back(gfx::Range(), GURL(url)); (using the parameter pack version of emplace_back())? If not, can you at least do without the word "Link" inside the parens? https://codereview.chromium.org/1540423004/diff/680001/components/autofill/co... File components/autofill/core/browser/legal_message_line.h (right): https://codereview.chromium.org/1540423004/diff/680001/components/autofill/co... components/autofill/core/browser/legal_message_line.h:54: // template string, e.g. "template" : "Here is a literal '{'" Nit: I think if you remove the quoted word "template" and the colon after it, this is clearer. https://codereview.chromium.org/1540423004/diff/680001/components/autofill/co... components/autofill/core/browser/legal_message_line.h:63: LegalMessageLines* out); Nit: Per the style guide, static methods go below the constructor/destructor.
Evan, ptal patch 7. https://codereview.chromium.org/1540423004/diff/680001/components/autofill/co... File components/autofill/core/browser/legal_message_line.cc (right): https://codereview.chromium.org/1540423004/diff/680001/components/autofill/co... components/autofill/core/browser/legal_message_line.cc:117: links_.emplace_back(Link{gfx::Range(), GURL(url)}); On 2016/01/14 04:19:25, Peter Kasting wrote: > Nit: Can you just do: > > links_.emplace_back(gfx::Range(), GURL(url)); > > (using the parameter pack version of emplace_back())? Done. https://codereview.chromium.org/1540423004/diff/680001/components/autofill/co... File components/autofill/core/browser/legal_message_line.h (right): https://codereview.chromium.org/1540423004/diff/680001/components/autofill/co... components/autofill/core/browser/legal_message_line.h:54: // template string, e.g. "template" : "Here is a literal '{'" On 2016/01/14 04:19:25, Peter Kasting wrote: > Nit: I think if you remove the quoted word "template" and the colon after it, > this is clearer. Done. https://codereview.chromium.org/1540423004/diff/680001/components/autofill/co... components/autofill/core/browser/legal_message_line.h:63: LegalMessageLines* out); On 2016/01/14 04:19:25, Peter Kasting wrote: > Nit: Per the style guide, static methods go below the constructor/destructor. Done.
lgtm with nits I spoke with Evan and he said it's fine to tbr him for the components/autofill changes based on his previous reviews and assuming I gave them one more pass. https://codereview.chromium.org/1540423004/diff/720001/components/autofill/co... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/1540423004/diff/720001/components/autofill/co... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:49: base::string16 AutofillCCInfoBarDelegate::GetLinkText() const { Move this below GetMessageText so they're in the same order as the header file. https://codereview.chromium.org/1540423004/diff/720001/components/autofill/co... File components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.cc (right): https://codereview.chromium.org/1540423004/diff/720001/components/autofill/co... components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.cc:33: #endif // defined(OS_IOS) I think this comment is harming more than it helps. It's pretty easy to see where this if clause starts a few lines above but the comment is cluttering this already complicated layout. https://codereview.chromium.org/1540423004/diff/720001/components/autofill/co... File components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.h (right): https://codereview.chromium.org/1540423004/diff/720001/components/autofill/co... components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.h:20: class InfoBarManager; This is no longer necessary.
Description was changed from ========== Add card details and legal message to Android save credit card infobar. Based on the mocks at: https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI... This patch also updates the AutofillClient interface to pass in the CreditCard object into ConfirmSaveCard*() methods. This allows the UI to show some card information to the user. BUG=535784 ========== to ========== Add card details and legal message to Android save credit card infobar. Based on the mocks at: https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI... This patch also updates the AutofillClient interface to pass in the CreditCard object into ConfirmSaveCard*() methods. This allows the UI to show some card information to the user. TBR=estade@chromium.org BUG=535784 ==========
TBR-ing Evan and sending to cq. https://codereview.chromium.org/1540423004/diff/720001/components/autofill/co... File components/autofill/core/browser/autofill_cc_infobar_delegate.cc (right): https://codereview.chromium.org/1540423004/diff/720001/components/autofill/co... components/autofill/core/browser/autofill_cc_infobar_delegate.cc:49: base::string16 AutofillCCInfoBarDelegate::GetLinkText() const { On 2016/01/14 18:32:10, Justin Donnelly wrote: > Move this below GetMessageText so they're in the same order as the header file. Done. https://codereview.chromium.org/1540423004/diff/720001/components/autofill/co... File components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.cc (right): https://codereview.chromium.org/1540423004/diff/720001/components/autofill/co... components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.cc:33: #endif // defined(OS_IOS) On 2016/01/14 18:32:10, Justin Donnelly wrote: > I think this comment is harming more than it helps. It's pretty easy to see > where this if clause starts a few lines above but the comment is cluttering this > already complicated layout. Done. https://codereview.chromium.org/1540423004/diff/720001/components/autofill/co... File components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.h (right): https://codereview.chromium.org/1540423004/diff/720001/components/autofill/co... components/autofill/core/browser/autofill_save_card_infobar_delegate_mobile.h:20: class InfoBarManager; On 2016/01/14 18:32:10, Justin Donnelly wrote: > This is no longer necessary. Done.
The CQ bit was checked by rouslan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org, boliu@chromium.org, rohitrao@chromium.org, pkasting@chromium.org, jdonnelly@chromium.org Link to the patchset: https://codereview.chromium.org/1540423004/#ps760001 (title: "components/autofill review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1540423004/760001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1540423004/760001
Message was sent while issue was closed.
Committed patchset #10 (id:760001)
Message was sent while issue was closed.
Description was changed from ========== Add card details and legal message to Android save credit card infobar. Based on the mocks at: https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI... This patch also updates the AutofillClient interface to pass in the CreditCard object into ConfirmSaveCard*() methods. This allows the UI to show some card information to the user. TBR=estade@chromium.org BUG=535784 ========== to ========== Add card details and legal message to Android save credit card infobar. Based on the mocks at: https://folio.googleplex.com/chrome-ux/mocks/304-payments-zero-integration/ZI... This patch also updates the AutofillClient interface to pass in the CreditCard object into ConfirmSaveCard*() methods. This allows the UI to show some card information to the user. TBR=estade@chromium.org BUG=535784 Committed: https://crrev.com/acfb6cffa322cac5195cc49f573809f7426061e8 Cr-Commit-Position: refs/heads/master@{#369521} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/acfb6cffa322cac5195cc49f573809f7426061e8 Cr-Commit-Position: refs/heads/master@{#369521}
Message was sent while issue was closed.
lgtm |
