Allow PasswordCredential instantiation when an input element has multiple 'autocomplete' tokens.

Allow PasswordCredential instantiation when an input element has multiple 'autocomplete' tokens.

BUG=651298
Committed: https://crrev.com/24a145b4b6485c3c8c5a45ff4019e5c7f1bf045b
Cr-Commit-Position: refs/heads/master@{#431540}

[vasilii, 2016-11-09 18:49:21]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 18:49:40]

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

[vasilii, 2016-11-09 18:52:18]

vasilii@chromium.org changed reviewers:
+ mkwst@chromium.org

[vasilii, 2016-11-09 18:52:19]

Hi Mike,

please review.

[commit-bot: I haz the power, 2016-11-09 20:30:05]

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

[commit-bot: I haz the power, 2016-11-09 20:30:05]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-11-10 02:01:51]

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

[haraken, 2016-11-10 02:01:51]

https://codereview.chromium.org/2492553002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp
(right):

https://codereview.chromium.org/2492553002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp:72:
auto hasToken = [&autofillTokens](const StringView& expectedToken) {

Nit: The lambda function is allowed in the style guide but is not commonly used
in Blink. I'd prefer using a simple helper function.

[Mike West, 2016-11-10 07:46:39]

https://codereview.chromium.org/2492553002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html
(right):

https://codereview.chromium.org/2492553002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html:197:
}, 'PasswordCredential creation from `HTMLFormElement` with no multiple
autocomplete attributes');
Nit: s/ no//

https://codereview.chromium.org/2492553002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp
(right):

https://codereview.chromium.org/2492553002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp:72:
auto hasToken = [&autofillTokens](const StringView& expectedToken) {
On 2016/11/10 at 02:01:51, haraken wrote:
> Nit: The lambda function is allowed in the style guide but is not commonly
used in Blink. I'd prefer using a simple helper function.

It also seems like it would be more efficient to loop through the list once
rather than once for each token we care about. That is, I'd prefer something
like:

```
toHTMLElement(element)
        ->fastGetAttribute(HTMLNames::autocompleteAttr)
        .getString()
        .lower() // Lowercase here once to avoid the case-folding logic below
        .split(' ', autofillTokens);
for (const auto& token : autofillTokens) {
  if (token == "current-password" || token == "new-password") {
    ...
  } else if (...) {
    ...
  }
}
```

[vasilii, 2016-11-10 09:21:55]

https://codereview.chromium.org/2492553002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html
(right):

https://codereview.chromium.org/2492553002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html:197:
}, 'PasswordCredential creation from `HTMLFormElement` with no multiple
autocomplete attributes');
On 2016/11/10 07:46:39, Mike West wrote:
> Nit: s/ no//

Done.

https://codereview.chromium.org/2492553002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp
(right):

https://codereview.chromium.org/2492553002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp:72:
auto hasToken = [&autofillTokens](const StringView& expectedToken) {
On 2016/11/10 07:46:39, Mike West wrote:
> On 2016/11/10 at 02:01:51, haraken wrote:
> > Nit: The lambda function is allowed in the style guide but is not commonly
> used in Blink. I'd prefer using a simple helper function.
> 
> It also seems like it would be more efficient to loop through the list once
> rather than once for each token we care about. That is, I'd prefer something
> like:
> 
> ```
> toHTMLElement(element)
>         ->fastGetAttribute(HTMLNames::autocompleteAttr)
>         .getString()
>         .lower() // Lowercase here once to avoid the case-folding logic below
>         .split(' ', autofillTokens);
> for (const auto& token : autofillTokens) {
>   if (token == "current-password" || token == "new-password") {
>     ...
>   } else if (...) {
>     ...
>   }
> }
> ```

Done.

[vasilii, 2016-11-10 09:21:59]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 09:22:06]

Dry run: 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, 2016-11-10 09:30:43]

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

[commit-bot: I haz the power, 2016-11-10 09:30:43]

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

[vasilii, 2016-11-10 10:20:11]

The red bots aren't my fault.

[vasilii, 2016-11-10 13:18:19]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 13:18:29]

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

[vasilii, 2016-11-10 14:25:33]

Please review.

[commit-bot: I haz the power, 2016-11-10 16:49:03]

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

[commit-bot: I haz the power, 2016-11-10 16:49:04]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-11-11 08:09:45]

https://codereview.chromium.org/2492553002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html
(right):

https://codereview.chromium.org/2492553002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html:197:
}, 'PasswordCredential creation from `HTMLFormElement` with multiple
autocomplete attributes');
Can you please add junk values to each of the `autocomplete` slots? We already
test most of the behavior above; the only new thing here is multiple values in
the `autocomplete` attribute. So, `autocomplete='current-password value1
value2'`, and so on can verify the various other branches of the `if` in your
code.

LGTM with that change.

[vasilii, 2016-11-11 09:38:09]

https://codereview.chromium.org/2492553002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html
(right):

https://codereview.chromium.org/2492553002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-basics.html:197:
}, 'PasswordCredential creation from `HTMLFormElement` with multiple
autocomplete attributes');
On 2016/11/11 08:09:45, Mike West wrote:
> Can you please add junk values to each of the `autocomplete` slots? We already
> test most of the behavior above; the only new thing here is multiple values in
> the `autocomplete` attribute. So, `autocomplete='current-password value1
> value2'`, and so on can verify the various other branches of the `if` in your
> code.
> 
> LGTM with that change.

Done.

[vasilii, 2016-11-11 09:38:33]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-11-11 09:38:33]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2492553002/#ps60001
(title: "change test")

[commit-bot: I haz the power, 2016-11-11 09:38:50]

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, 2016-11-11 11:23:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-11 11:24:30]

Description was changed from

==========
Allow PasswordCredential instantiation when an input element has multiple
'autocomplete' tokens.

BUG=651298
==========

to

==========
Allow PasswordCredential instantiation when an input element has multiple
'autocomplete' tokens.

BUG=651298

Committed: https://crrev.com/24a145b4b6485c3c8c5a45ff4019e5c7f1bf045b
Cr-Commit-Position: refs/heads/master@{#431540}
==========

[commit-bot: I haz the power, 2016-11-11 11:24:31]

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