Issue 23756007: [rac] Show amex specific cvc hint

Issue 23756007: [rac] Show amex specific cvc hint (Closed)

Created:
7 years, 3 months ago by please use gerrit instead

Modified:
7 years, 3 months ago

Reviewers:
oshima, Evan Stade

CC:
chromium-reviews, oshima+watch_chromium.org, benquan, tfarina, Dane Wallinga, dyu1, estade+watch_chromium.org, Albert Bodenhamer, Ilya Sherman, rouslan+autofillwatch_chromium.org, groby-ooo-7-16

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

[rac] Show amex specific cvc hint This patch adds an amex specific icon for cvc field. The icon refreshes when the credit card number changes. The icon does not change when cvc value changes. Manual tests for both autofill and wallet: 1) Enter 37 into cvc field: nothing should change. 2) Enter 37 into credit card number field: cvc icon should be amex specific. TEST=AutofillDialogControllerTest.FieldControlsIcons AutofillDialogControllerTest.IconsForFields_NoCreditCard AutofillDialogControllerTest.IconsForFields_CreditCardNumberOnly AutofillDialogControllerTest.IconsForFields_CvcOnly AutofillDialogControllerTest.IconsForFields_BothCreditCardAndCvc BUG=250853 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223932

Patch Set 1 #

Patch Set 2 : Make it work for wallet too #

Patch Set 3 : Move icon dependency logic into controller for testability #

Total comments: 3

Patch Set 4 : Get a group of icons for a group of fields #

Patch Set 5 : Use defined types #

Patch Set 6 : Fix a typo in a comment #

Patch Set 7 : Fix order of method definitions #

Patch Set 8 : Bring back the deprecated IconForField method, as it is still used on Mac #

Total comments: 14

Patch Set 9 : Address comments #

Total comments: 2

Patch Set 10 : Enable going from icon to no icon #

Total comments: 6

Patch Set 11 : Nits #

Patch Set 12 : Enable returning card type from wallet instrument #

Total comments: 2

Patch Set 13 : Rename StringFromType to StringIconIdentifierFromType #

Created: 7 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+180 lines, -18 lines)			Patch
M	chrome/app/theme/theme_resources.grd	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/ui/autofill/autofill_dialog_controller_impl.h	View	1 2 3 4 5 6 7 8 9 10 11	2 chunks	+6 lines, -1 line	0 comments	Download
M	chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc	View	1 2 3 4 5 6 7 8 9 10 11	3 chunks	+60 lines, -4 lines	0 comments	Download
M	chrome/browser/ui/autofill/autofill_dialog_controller_unittest.cc	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+42 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/autofill/autofill_dialog_view_delegate.h	View	1 2 3 4 5 6 7	2 chunks	+14 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/autofill/mock_autofill_dialog_view_delegate.h	View	1 2 3 4 5 6 7 8	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/autofill/autofill_dialog_views.h	View	1 2 3 4 5	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/autofill/autofill_dialog_views.cc	View	1 2 3 4 5 6 7 8 9 10 11	7 chunks	+31 lines, -11 lines	0 comments	Download
M	components/autofill/content/browser/wallet/wallet_items.cc	View	1 2 3 4 5 6 7 8 9 10 11 12	2 chunks	+18 lines, -0 lines	0 comments	Download

Messages

Total messages: 22 (0 generated)

Expand Messages | Collapse Messages

Evan Stade

this is a tough one. I can't think of a great solution off hand. I ...

7 years, 3 months ago (2013-09-11 00:17:03 UTC) #2

please use gerrit instead

https://codereview.chromium.org/23756007/diff/5001/chrome/browser/ui/views/autofill/autofill_dialog_views.cc File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://codereview.chromium.org/23756007/diff/5001/chrome/browser/ui/views/autofill/autofill_dialog_views.cc#newcode2276 chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2276: delegate_->IconForField(dependent_type, textfield->text())); On 2013/09/11 00:17:03, Evan Stade wrote: > ...

7 years, 3 months ago (2013-09-11 16:27:40 UTC) #3

Evan Stade

https://codereview.chromium.org/23756007/diff/5002/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/23756007/diff/5002/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode1518 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1518: model->GetInfo(AutofillType(CREDIT_CARD_NUMBER)); this won't work for wallet (SECTION_CC_BILLING); we don't ...

7 years, 3 months ago (2013-09-16 23:36:59 UTC) #5

please use gerrit instead

Evan: PTAL. https://codereview.chromium.org/23756007/diff/5002/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/23756007/diff/5002/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode1518 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1518: model->GetInfo(AutofillType(CREDIT_CARD_NUMBER)); On 2013/09/16 23:36:59, Evan Stade wrote: ...

7 years, 3 months ago (2013-09-17 00:38:16 UTC) #6

Evan Stade

https://codereview.chromium.org/23756007/diff/5002/chrome/browser/ui/views/autofill/autofill_dialog_views.cc File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://codereview.chromium.org/23756007/diff/5002/chrome/browser/ui/views/autofill/autofill_dialog_views.cc#newcode2353 chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2353: FieldIconMap::const_iterator field_icon_it = field_icons.find(field_type); On 2013/09/17 00:38:17, Rouslan Solomakhin ...

7 years, 3 months ago (2013-09-17 00:52:44 UTC) #7

please use gerrit instead

Evan: PTAL. https://codereview.chromium.org/23756007/diff/5002/chrome/browser/ui/views/autofill/autofill_dialog_views.cc File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right): https://codereview.chromium.org/23756007/diff/5002/chrome/browser/ui/views/autofill/autofill_dialog_views.cc#newcode2353 chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2353: FieldIconMap::const_iterator field_icon_it = field_icons.find(field_type); On 2013/09/17 00:52:45, ...

7 years, 3 months ago (2013-09-17 17:25:04 UTC) #8

Evan: PTAL.

https://codereview.chromium.org/23756007/diff/5002/chrome/browser/ui/views/au...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/23756007/diff/5002/chrome/browser/ui/views/au...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:2353:
FieldIconMap::const_iterator field_icon_it = field_icons.find(field_type);
On 2013/09/17 00:52:45, Evan Stade wrote:
> On 2013/09/17 00:38:17, Rouslan Solomakhin wrote:
> > On 2013/09/16 23:36:59, Evan Stade wrote:
> > > since the icon map is smaller than the textfield map, you should swap the
> > > nesting of these loops.
> > 
> > That wouldn't be efficient because the icon map has ServerFieldType keys,
but
> > textfield map has DetailInput keys. I can get the ServerFieldType of a
> > DetailInput, but there's no efficient way get the DetailInput of a
> > ServerFieldType, AFAIK. Please let me know if I missed something.
> 
> Well, for the numbers we're talking about, it's actually not clear whether
> find() is faster than linear iteration.
> 
> But where this code falls down at the moment is that it doesn't allow you to
go
> from an icon to no icon (which may not be a case we run into at the moment,
but
> it's easy enough to fix).

I enabled going from an icon to no icon. Now the current loop order seems to
make even more sense:

for each textfield:
  if there's an icon for the textfield:
    set the icon on the textfield
  else:
    clear the icon on the textfield

The alternative loop order would be a bit weird, imho:

for each decorated textfield:
  clear the icon on the text field
for each icon:
  for each textfield:
    if this icon is for this textfield:
      set the icon on the textfield

Would you agree?

https://codereview.chromium.org/23756007/diff/59001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/23756007/diff/59001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:549: return
rb.GetImageNamed(IDR_CREDIT_CARD_CVC_HINT_AMEX);
On 2013/09/17 00:52:45, Evan Stade wrote:
> nit: \n

Done.

Evan Stade

lgtm https://codereview.chromium.org/23756007/diff/65001/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/23756007/diff/65001/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode1534 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1534: return gfx::Image(); \n https://codereview.chromium.org/23756007/diff/65001/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode1537 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1537: return gfx::Image(); \n ...

7 years, 3 months ago (2013-09-17 19:59:05 UTC) #9

please use gerrit instead

https://codereview.chromium.org/23756007/diff/65001/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right): https://codereview.chromium.org/23756007/diff/65001/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc#newcode1534 chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1534: return gfx::Image(); On 2013/09/17 19:59:06, Evan Stade wrote: > ...

7 years, 3 months ago (2013-09-17 20:32:23 UTC) #10

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/23756007/75001

7 years, 3 months ago (2013-09-17 20:33:47 UTC) #11

commit-bot: I haz the power

Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=25920

7 years, 3 months ago (2013-09-17 20:48:23 UTC) #12

please use gerrit instead

Mitsuru: chrome/app/theme OWNERS PTAL.

7 years, 3 months ago (2013-09-17 21:03:41 UTC) #13

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/23756007/75001

7 years, 3 months ago (2013-09-17 21:09:17 UTC) #15

commit-bot: I haz the power

Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=169807

7 years, 3 months ago (2013-09-17 22:32:13 UTC) #16

please use gerrit instead

Evan: PTAL again. I fixed WalletItems::GetInfo(CREDIT_CARD_TYPE) return a valid value.

7 years, 3 months ago (2013-09-17 23:20:12 UTC) #17

Evan Stade

https://chromiumcodereview.appspot.com/23756007/diff/59002/components/autofill/content/browser/wallet/wallet_items.cc File components/autofill/content/browser/wallet/wallet_items.cc (right): https://chromiumcodereview.appspot.com/23756007/diff/59002/components/autofill/content/browser/wallet/wallet_items.cc#newcode83 components/autofill/content/browser/wallet/wallet_items.cc:83: std::string StringFromType(WalletItems::MaskedInstrument::Type type) { can you call this something ...

7 years, 3 months ago (2013-09-18 01:34:38 UTC) #18

please use gerrit instead

Evan: PTAL. https://chromiumcodereview.appspot.com/23756007/diff/59002/components/autofill/content/browser/wallet/wallet_items.cc File components/autofill/content/browser/wallet/wallet_items.cc (right): https://chromiumcodereview.appspot.com/23756007/diff/59002/components/autofill/content/browser/wallet/wallet_items.cc#newcode83 components/autofill/content/browser/wallet/wallet_items.cc:83: std::string StringFromType(WalletItems::MaskedInstrument::Type type) { On 2013/09/18 01:34:39, ...

7 years, 3 months ago (2013-09-18 15:22:43 UTC) #19

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rouslan@chromium.org/23756007/95001

7 years, 3 months ago (2013-09-18 15:49:58 UTC) #21

Message was sent while issue was closed.

Change committed as 223932

Expand Messages | Collapse Messages