Autofill - don't prompt to unmask a credit card if the number isn't needed.

Autofill - don't prompt to unmask a credit card if the number isn't needed.

BUG=468583
Committed: https://crrev.com/ab445c3ca9c37d45096f8114129338c68be6a975
Cr-Commit-Position: refs/heads/master@{#322284}

[Evan Stade, 2015-03-23 21:38:55]

estade@chromium.org changed reviewers:
+ brettw@chromium.org

[Evan Stade, 2015-03-23 21:40:41]

estade@chromium.org changed reviewers:
+ waltercacau@chromium.org

[Evan Stade, 2015-03-23 21:40:42]

https://codereview.chromium.org/1029233002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1029233002/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:598:
credit_card_form_event_logger_->OnDidSelectMaskedServerCardSuggestion();
+walter --- the meaning of this is changing slightly. It won't be called if a
masked card is selected, but no unmasking is needed. Thoughts?

[Walter Cacau, 2015-03-23 21:53:07]

https://codereview.chromium.org/1029233002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1029233002/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:598:
credit_card_form_event_logger_->OnDidSelectMaskedServerCardSuggestion();
On 2015/03/23 21:40:41, Evan Stade wrote:
> +walter --- the meaning of this is changing slightly. It won't be called if a
> masked card is selected, but no unmasking is needed. Thoughts?

Maybe we should create a new histogram value for this case? Do we have any idea
of how common this is?

[Evan Stade, 2015-03-23 22:09:06]

https://codereview.chromium.org/1029233002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1029233002/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:598:
credit_card_form_event_logger_->OnDidSelectMaskedServerCardSuggestion();
On 2015/03/23 21:53:07, Walter Cacau wrote:
> On 2015/03/23 21:40:41, Evan Stade wrote:
> > +walter --- the meaning of this is changing slightly. It won't be called if
a
> > masked card is selected, but no unmasking is needed. Thoughts?
> 
> Maybe we should create a new histogram value for this case? Do we have any
idea
> of how common this is?

I don't think it's very common or very interesting. There is no visible
difference between masked and unmasked and local in this situation.

[Walter Cacau, 2015-03-23 22:16:52]

On 2015/03/23 22:09:06, Evan Stade wrote:
>
https://codereview.chromium.org/1029233002/diff/1/components/autofill/core/br...
> File components/autofill/core/browser/autofill_manager.cc (right):
> 
>
https://codereview.chromium.org/1029233002/diff/1/components/autofill/core/br...
> components/autofill/core/browser/autofill_manager.cc:598:
> credit_card_form_event_logger_->OnDidSelectMaskedServerCardSuggestion();
> On 2015/03/23 21:53:07, Walter Cacau wrote:
> > On 2015/03/23 21:40:41, Evan Stade wrote:
> > > +walter --- the meaning of this is changing slightly. It won't be called
if
> a
> > > masked card is selected, but no unmasking is needed. Thoughts?
> > 
> > Maybe we should create a new histogram value for this case? Do we have any
> idea
> > of how common this is?
> 
> I don't think it's very common or very interesting. There is no visible
> difference between masked and unmasked and local in this situation.

Fair enough. Can we document in histograms.xml and/or in the comments of the
event about this caveat?

[Evan Stade, 2015-03-23 22:34:02]

estade@chromium.org changed reviewers:
+ isherman@chromium.org

[Evan Stade, 2015-03-23 22:34:02]

+isherman for histograms.xml

https://codereview.chromium.org/1029233002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1029233002/diff/1/components/autofill/core/br...
components/autofill/core/browser/autofill_manager.cc:598:
credit_card_form_event_logger_->OnDidSelectMaskedServerCardSuggestion();
On 2015/03/23 22:09:06, Evan Stade wrote:
> On 2015/03/23 21:53:07, Walter Cacau wrote:
> > On 2015/03/23 21:40:41, Evan Stade wrote:
> > > +walter --- the meaning of this is changing slightly. It won't be called
if
> a
> > > masked card is selected, but no unmasking is needed. Thoughts?
> > 
> > Maybe we should create a new histogram value for this case? Do we have any
> idea
> > of how common this is?
> 
> I don't think it's very common or very interesting. There is no visible
> difference between masked and unmasked and local in this situation.

Done.

[Walter Cacau, 2015-03-23 22:39:52]

lgtm for metrics

[Ilya Sherman, 2015-03-23 23:56:41]

histograms.xml lgtm

[Evan Stade, 2015-03-24 15:28:40]

On 2015/03/23 23:56:41, Ilya Sherman wrote:
> histograms.xml lgtm

ping brettw

[brettw, 2015-03-24 22:30:47]

LGTM if my understanding is correct and the comment is changed, otherwise we
should fix the comment a different way to correct my understanding.

https://codereview.chromium.org/1029233002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1029233002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:556: FormStructure*
form_structure = NULL;
I thought we were preferring nullptr for new code. Not critical though.

https://codereview.chromium.org/1029233002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:564: // If the relevant
section is autofilled, we'll only fill |autofill_field|.
Everything here makes sense to me except these three lines. I'm assuming is
misunderstand what is_autofilled means, but I don't see any documentation. If
this means that we already autofilled this section and now we're doing it again
so only this field will change, maybe the comment should say "If the relevant
section is already autofilled, this new operation will only fill
|autofill_field|."

[Evan Stade, 2015-03-24 22:34:01]

https://codereview.chromium.org/1029233002/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/1029233002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:556: FormStructure*
form_structure = NULL;
On 2015/03/24 22:30:47, brettw wrote:
> I thought we were preferring nullptr for new code. Not critical though.

old habits die hard

https://codereview.chromium.org/1029233002/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:564: // If the relevant
section is autofilled, we'll only fill |autofill_field|.
On 2015/03/24 22:30:47, brettw wrote:
> Everything here makes sense to me except these three lines. I'm assuming is
> misunderstand what is_autofilled means, but I don't see any documentation. If
> this means that we already autofilled this section and now we're doing it
again
> so only this field will change, maybe the comment should say "If the relevant
> section is already autofilled, this new operation will only fill
> |autofill_field|."

your understanding is correct, I'll apply your new comment

[Evan Stade, 2015-03-25 21:51:01]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-03-25 21:51:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
waltercacau@chromium.org, brettw@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1029233002/#ps40001
(title: "brett review")

[commit-bot: I haz the power, 2015-03-25 21:52:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1029233002/40001

[commit-bot: I haz the power, 2015-03-25 21:56:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-25 21:56:54]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Evan Stade, 2015-03-25 22:06:00]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-03-25 22:06:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
waltercacau@chromium.org, brettw@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1029233002/#ps60001
(title: "resolve histograms conflict")

[commit-bot: I haz the power, 2015-03-25 22:07:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1029233002/60001

[commit-bot: I haz the power, 2015-03-25 23:22:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-03-25 23:22:03]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2015-03-25 23:41:10]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-03-25 23:42:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1029233002/60001

[commit-bot: I haz the power, 2015-03-26 01:06:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-26 01:07:06]

Patchset 4 (id:??) landed as
https://crrev.com/ab445c3ca9c37d45096f8114129338c68be6a975
Cr-Commit-Position: refs/heads/master@{#322284}