Password Manager should skip fields with credit card autocomplete attribute.

Password Manager should skip fields with credit card autocomplete attribute.

BUG=703491
Review-Url: https://codereview.chromium.org/2771833002
Cr-Commit-Position: refs/heads/master@{#459766}
Committed: https://chromium.googlesource.com/chromium/src/+/b0bfdf4c71968db2620169a3e4056a4460beb00d

[dvadym, 2017-03-23 14:41:20]

Description was changed from

==========
Password Manager should skip fields with credit card autocomplete

BUG=703491
==========

to

==========
Password Manager should skip fields with credit card autocomplete attribute.

BUG=703491
==========

[dvadym, 2017-03-23 14:41:20]

dvadym@chromium.org changed reviewers:
+ vabr@chromium.org

[dvadym, 2017-03-23 14:42:41]

Hi Vaclav,

Could you please review this CL?

Regards,
Vadym

[Mathieu, 2017-03-23 14:50:50]

mathp@chromium.org changed reviewers:
+ mathp@chromium.org

[Mathieu, 2017-03-23 14:50:50]

Good to see this! Is there a bigger plan to address this at large? autocomplete
attributes is only a tiny percentage of forms.

[dvadym, 2017-03-23 15:01:41]

Definitely, we're going to address this. Unfortunately it's hard to do in the
current Password Manager architecture. I'm planning to make refactorings of
Passsword Manager renderer part, and addressing this issue will be a part of
these refactorings. I'm going to write a design doc about this in April, but I
think the implementation will be not earlier than Q3.

[vabr (Chromium), 2017-03-23 20:04:23]

Thanks, Vadym.

LGTM with a suggestion for code simplification.

Cheers,
Vaclav

https://codereview.chromium.org/2771833002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/2771833002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:754: //
All credit cards autocomplete attributes start with "cc-". So if there is a
I wonder whether this would be simpler to code with SplitStringPiece:

for (auto attr : base::SplitStringPiece(autocomplete_value_lowercase, " \t\n\r",
base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) {
  if (base::StartsWith(attr, kAutocompleteCreditCardPrefix,
base::CompareCase::SENSITIVE)
    return true;
}
return false;

[Mathieu, 2017-03-23 21:55:14]

lgtm

[dvadym, 2017-03-24 09:34:33]

Thanks for comments Vaclav! PTAL

https://codereview.chromium.org/2771833002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/2771833002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:754: //
All credit cards autocomplete attributes start with "cc-". So if there is a
On 2017/03/23 20:04:23, vabr (Chromium) wrote:
> I wonder whether this would be simpler to code with SplitStringPiece:
> 
> for (auto attr : base::SplitStringPiece(autocomplete_value_lowercase, "
\t\n\r",
> base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) {
>   if (base::StartsWith(attr, kAutocompleteCreditCardPrefix,
> base::CompareCase::SENSITIVE)
>     return true;
> }
> return false;

Sure it's simpler. I had a dilemma because of this, to use SplitStringPiece or
not. Not it's written more effective (no creating of extra objects and not doing
split operation), but of course with splitting it's easier to understand. Anyway
HasAutocompleteAttributeValue (it's next function up) uses SplitStringPiece, but
we can make it more effective too.
So for me, any option is ok. What option would you prefer?

[Roger McFarlane (Chromium), 2017-03-24 15:47:15]

rogerm@chromium.org changed reviewers:
+ rogerm@chromium.org

[Roger McFarlane (Chromium), 2017-03-24 15:47:16]

https://codereview.chromium.org/2771833002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/2771833002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:754: //
All credit cards autocomplete attributes start with "cc-". So if there is a
On 2017/03/24 09:34:33, dvadym wrote:
> On 2017/03/23 20:04:23, vabr (Chromium) wrote:
> > I wonder whether this would be simpler to code with SplitStringPiece:
> > 
> > for (auto attr : base::SplitStringPiece(autocomplete_value_lowercase, "
> \t\n\r",
> > base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) {
> >   if (base::StartsWith(attr, kAutocompleteCreditCardPrefix,
> > base::CompareCase::SENSITIVE)
> >     return true;
> > }
> > return false;
> 
> Sure it's simpler. I had a dilemma because of this, to use SplitStringPiece or
> not. Not it's written more effective (no creating of extra objects and not
doing
> split operation), but of course with splitting it's easier to understand.
Anyway
> HasAutocompleteAttributeValue (it's next function up) uses SplitStringPiece,
but
> we can make it more effective too.
> So for me, any option is ok. What option would you prefer?

You could use a regular expression with case insensitive matching:
 (^|\s)cc\-

[vabr (Chromium), 2017-03-24 19:19:36]

Hi Vadym, 

Please find my answer below.

Cheers,
Vaclav

https://codereview.chromium.org/2771833002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/2771833002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:754: //
All credit cards autocomplete attributes start with "cc-". So if there is a
On 2017/03/24 09:34:33, dvadym wrote:
> On 2017/03/23 20:04:23, vabr (Chromium) wrote:
> > I wonder whether this would be simpler to code with SplitStringPiece:
> > 
> > for (auto attr : base::SplitStringPiece(autocomplete_value_lowercase, "
> \t\n\r",
> > base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) {
> >   if (base::StartsWith(attr, kAutocompleteCreditCardPrefix,
> > base::CompareCase::SENSITIVE)
> >     return true;
> > }
> > return false;
> 
> Sure it's simpler. I had a dilemma because of this, to use SplitStringPiece or
> not. Not it's written more effective (no creating of extra objects and not
doing
> split operation), but of course with splitting it's easier to understand.
Anyway
> HasAutocompleteAttributeValue (it's next function up) uses SplitStringPiece,
but
> we can make it more effective too.
> So for me, any option is ok. What option would you prefer?

The extra object created is a vector of StringPieces, which should not be too
expensive to create (it will also usually have very few items). I would be
worried if strings were copied, but they are not. If in doubt, feel free to do a
benchmark.

The code readability is significantly better with SplitStringPiece, without
needing 3-line explanation comments and with much shorter code.

Altogether, SplitStringPiece seems like a win to me here.

[dvadym, 2017-03-27 10:51:40]

https://codereview.chromium.org/2771833002/diff/20001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/2771833002/diff/20001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:754: //
All credit cards autocomplete attributes start with "cc-". So if there is a
On 2017/03/24 19:19:36, vabr (Chromium) wrote:
> On 2017/03/24 09:34:33, dvadym wrote:
> > On 2017/03/23 20:04:23, vabr (Chromium) wrote:
> > > I wonder whether this would be simpler to code with SplitStringPiece:
> > > 
> > > for (auto attr : base::SplitStringPiece(autocomplete_value_lowercase, "
> > \t\n\r",
> > > base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) {
> > >   if (base::StartsWith(attr, kAutocompleteCreditCardPrefix,
> > > base::CompareCase::SENSITIVE)
> > >     return true;
> > > }
> > > return false;
> > 
> > Sure it's simpler. I had a dilemma because of this, to use SplitStringPiece
or
> > not. Not it's written more effective (no creating of extra objects and not
> doing
> > split operation), but of course with splitting it's easier to understand.
> Anyway
> > HasAutocompleteAttributeValue (it's next function up) uses SplitStringPiece,
> but
> > we can make it more effective too.
> > So for me, any option is ok. What option would you prefer?
> 
> The extra object created is a vector of StringPieces, which should not be too
> expensive to create (it will also usually have very few items). I would be
> worried if strings were copied, but they are not. If in doubt, feel free to do
a
> benchmark.
> 
> The code readability is significantly better with SplitStringPiece, without
> needing 3-line explanation comments and with much shorter code.
> 
> Altogether, SplitStringPiece seems like a win to me here.

Changed to call of SplitStringPiece, in this function and in
HasAutocompleteAttributeValue.

[vabr (Chromium), 2017-03-27 12:26:36]

On 2017/03/27 10:51:40, dvadym wrote:
>
https://codereview.chromium.org/2771833002/diff/20001/components/autofill/con...
> File components/autofill/content/renderer/password_form_conversion_utils.cc
> (right):
> 
>
https://codereview.chromium.org/2771833002/diff/20001/components/autofill/con...
> components/autofill/content/renderer/password_form_conversion_utils.cc:754: //
> All credit cards autocomplete attributes start with "cc-". So if there is a
> On 2017/03/24 19:19:36, vabr (Chromium) wrote:
> > On 2017/03/24 09:34:33, dvadym wrote:
> > > On 2017/03/23 20:04:23, vabr (Chromium) wrote:
> > > > I wonder whether this would be simpler to code with SplitStringPiece:
> > > > 
> > > > for (auto attr : base::SplitStringPiece(autocomplete_value_lowercase, "
> > > \t\n\r",
> > > > base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY)) {
> > > >   if (base::StartsWith(attr, kAutocompleteCreditCardPrefix,
> > > > base::CompareCase::SENSITIVE)
> > > >     return true;
> > > > }
> > > > return false;
> > > 
> > > Sure it's simpler. I had a dilemma because of this, to use
SplitStringPiece
> or
> > > not. Not it's written more effective (no creating of extra objects and not
> > doing
> > > split operation), but of course with splitting it's easier to understand.
> > Anyway
> > > HasAutocompleteAttributeValue (it's next function up) uses
SplitStringPiece,
> > but
> > > we can make it more effective too.
> > > So for me, any option is ok. What option would you prefer?
> > 
> > The extra object created is a vector of StringPieces, which should not be
too
> > expensive to create (it will also usually have very few items). I would be
> > worried if strings were copied, but they are not. If in doubt, feel free to
do
> a
> > benchmark.
> > 
> > The code readability is significantly better with SplitStringPiece, without
> > needing 3-line explanation comments and with much shorter code.
> > 
> > Altogether, SplitStringPiece seems like a win to me here.
> 
> Changed to call of SplitStringPiece, in this function and in
> HasAutocompleteAttributeValue.

LGTM, thanks!

[dvadym, 2017-03-27 12:26:48]

The CQ bit was checked by dvadym@chromium.org

[dvadym, 2017-03-27 12:26:49]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org
Link to the patchset: https://codereview.chromium.org/2771833002/#ps60001
(title: "Do not create string in HasAutocompleteAttributeValue")

[commit-bot: I haz the power, 2017-03-27 12:26:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 13:11:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490617608962360,
"parent_rev": "7e1ceeb2be46352ef86049f7dc8166cc2384692c", "commit_rev":
"b0bfdf4c71968db2620169a3e4056a4460beb00d"}

[commit-bot: I haz the power, 2017-03-27 13:12:11]

Description was changed from

==========
Password Manager should skip fields with credit card autocomplete attribute.

BUG=703491
==========

to

==========
Password Manager should skip fields with credit card autocomplete attribute.

BUG=703491

Review-Url: https://codereview.chromium.org/2771833002
Cr-Commit-Position: refs/heads/master@{#459766}
Committed:
https://chromium.googlesource.com/chromium/src/+/b0bfdf4c71968db2620169a3e405...
==========

[commit-bot: I haz the power, 2017-03-27 13:12:12]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b0bfdf4c71968db2620169a3e405...

[battre, 2017-03-27 15:17:46]

battre@chromium.org changed reviewers:
+ battre@chromium.org

[battre, 2017-03-27 15:17:47]

Should we go one step further and look for CVV, CVC, etc
(https://en.wikipedia.org/wiki/Card_security_code) in the name attribute?

Best regards,
Dominic

[dvadym, 2017-03-27 15:57:24]

On 2017/03/27 15:17:47, battre wrote:
> Should we go one step further and look for CVV, CVC, etc
> (https://en.wikipedia.org/wiki/Card_security_code) in the name attribute?
> 
> Best regards,
> Dominic

I'll make a data analysis, how often it happens, and if it's often why not.

[Roger McFarlane (Chromium), 2017-03-27 19:06:28]

https://codereview.chromium.org/2771833002/diff/60001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/2771833002/diff/60001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:758: for
(const auto& token : base::SplitStringPiece(
Another, IMO better, alternative would be to just do startswith "cc-" or
contains " cc-" which would be clearer to read, have zero memory overhead and be
O(n) time, where n is the length of the value.

[vabr (Chromium), 2017-03-27 20:20:03]

https://codereview.chromium.org/2771833002/diff/60001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/2771833002/diff/60001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:758: for
(const auto& token : base::SplitStringPiece(
On 2017/03/27 19:06:28, Roger McFarlane (Chromium) wrote:
> Another, IMO better, alternative would be to just do startswith "cc-" or
> contains " cc-" which would be clearer to read, have zero memory overhead and
be
> O(n) time, where n is the length of the value.

It is indeed O(n) because the length of " cc-" is a constant. I'm not sure about
the zero memory overhead, because I don't know how exactly string::find is
implemented.

What you describe is roughly what Vadym had originally, and I would argue that
that was not clearer to read.

I'm fine with anything which at least reaches the bar of the current code: no
explanatory comments needed, and the code is concise.

[dvadym, 2017-03-28 08:33:08]

https://codereview.chromium.org/2771833002/diff/60001/components/autofill/con...
File components/autofill/content/renderer/password_form_conversion_utils.cc
(right):

https://codereview.chromium.org/2771833002/diff/60001/components/autofill/con...
components/autofill/content/renderer/password_form_conversion_utils.cc:758: for
(const auto& token : base::SplitStringPiece(
On 2017/03/27 20:20:03, vabr (Chromium) wrote:
> On 2017/03/27 19:06:28, Roger McFarlane (Chromium) wrote:
> > Another, IMO better, alternative would be to just do startswith "cc-" or
> > contains " cc-" which would be clearer to read, have zero memory overhead
and
> be
> > O(n) time, where n is the length of the value.
> 
> It is indeed O(n) because the length of " cc-" is a constant. I'm not sure
about
> the zero memory overhead, because I don't know how exactly string::find is
> implemented.
> 
> What you describe is roughly what Vadym had originally, and I would argue that
> that was not clearer to read.
> 
> I'm fine with anything which at least reaches the bar of the current code: no
> explanatory comments needed, and the code is concise.

When using regexp, there are 2 times: creating a regexp object (building state
machine) and matching. Since the creating of the object could take significant
time in general, usually it's better to create the object only once. Then it's
needed to use some machinery to do this for example like here
https://cs.chromium.org/chromium/src/components/autofill/content/renderer/pas...
with lazy loading. My feeling that it would be not easier than just straight
forward splitting and checking with StartsWith. Moreover now both this function
and HasAutocompleteAttributeValue function have the same logic with splitting,
and in HasAutocompleteAttributeValue it doesn't make sense to create a regexp
object since |value_in_lowercase| is different on each call.