Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2917933002:
[PasswordGeneration] Improve change/confirm password field detection. (Closed)
|
Created:
3 years, 6 months ago by pkalinnikov Modified:
3 years, 6 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, darin-cc_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[PasswordGeneration] Improve change/confirm password field detection.
Use autocomplete attributes to discover change/confirm password fields when no
PasswordFormGenerationData is found for a possible password form.
BUG=716464
Review-Url: https://codereview.chromium.org/2917933002
Cr-Commit-Position: refs/heads/master@{#478250}
Committed: https://chromium.googlesource.com/chromium/src/+/6121d9064193b5d9eef9092425909b4e6cec3bcb
Patch Set 1 #Patch Set 2 : Clean up and fix test. #
Total comments: 9
Patch Set 3 : Address comments from dvadym@. #
Total comments: 10
Patch Set 4 : Address more comments. #
Total comments: 6
Messages
Total messages: 37 (25 generated)
Description was changed from ========== [PasswordGeneration] Take autofill="new-password" attribute into account. BUG=716464 ========== to ========== [PasswordGeneration] Take autocomplete="new-password" into account. TODO: Description. BUG=716464 ==========
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pkalinnikov@chromium.org changed reviewers: + dvadym@chromium.org, kolos@chromium.org
PTAL. Will add tests later.
https://codereview.chromium.org/2917933002/diff/20001/components/autofill/con... File components/autofill/content/renderer/password_generation_agent.cc (right): https://codereview.chromium.org/2917933002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_generation_agent.cc:422: password_elements = possible_form_data.password_elements; This line is equivalent to what we had before the CL. However, shouldn't we do lines 435-445 here as well? My understanding is that it is also a "local heuristic".
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, it looks good. https://codereview.chromium.org/2917933002/diff/20001/components/autofill/con... File components/autofill/content/renderer/password_generation_agent.cc (right): https://codereview.chromium.org/2917933002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_generation_agent.cc:78: std::vector<blink::WebInputElement> FindNewPasswordElementsForGeneration( It's not clear by names what's the different between this function and next function. Probably it's better to call this function something like FindMarkedBySiteNewPasswordElements https://codereview.chromium.org/2917933002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_generation_agent.cc:81: CR_DEFINE_STATIC_LOCAL(blink::WebString, kAutocomplete, ("autocomplete")); It's better to reuse function HasAutocompleteAttributeValue https://cs.chromium.org/chromium/src/components/autofill/content/renderer/pas... https://codereview.chromium.org/2917933002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_generation_agent.cc:145: bool AutocompleteAttributesSetForGeneration(const PasswordForm& form) { Nit: This function is pretty trivial, we can remove it. https://codereview.chromium.org/2917933002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_generation_agent.cc:422: password_elements = possible_form_data.password_elements; On 2017/06/06 16:49:46, pkalinnikov wrote: > This line is equivalent to what we had before the CL. > > However, shouldn't we do lines 435-445 here as well? My understanding is that it > is also a "local heuristic". Don't worry about these lines, they are not used. Probably we should remove switch at all.
Description was changed from ========== [PasswordGeneration] Take autocomplete="new-password" into account. TODO: Description. BUG=716464 ========== to ========== [PasswordGeneration] Improve change/confirm password field detection. Use autocomplete attributes to discover change/confirm password fields when no PasswordFormGenerationData is found for a possible password form. BUG=716464 ==========
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed comments. https://codereview.chromium.org/2917933002/diff/20001/components/autofill/con... File components/autofill/content/renderer/password_generation_agent.cc (right): https://codereview.chromium.org/2917933002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_generation_agent.cc:78: std::vector<blink::WebInputElement> FindNewPasswordElementsForGeneration( On 2017/06/07 17:16:43, dvadym wrote: > It's not clear by names what's the different between this function and next > function. > Probably it's better to call this function something like > FindMarkedBySiteNewPasswordElements Done. https://codereview.chromium.org/2917933002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_generation_agent.cc:81: CR_DEFINE_STATIC_LOCAL(blink::WebString, kAutocomplete, ("autocomplete")); On 2017/06/07 17:16:42, dvadym wrote: > It's better to reuse function HasAutocompleteAttributeValue > https://cs.chromium.org/chromium/src/components/autofill/content/renderer/pas... > Done. https://codereview.chromium.org/2917933002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_generation_agent.cc:145: bool AutocompleteAttributesSetForGeneration(const PasswordForm& form) { On 2017/06/07 17:16:42, dvadym wrote: > Nit: This function is pretty trivial, we can remove it. Done. https://codereview.chromium.org/2917933002/diff/20001/components/autofill/con... components/autofill/content/renderer/password_generation_agent.cc:422: password_elements = possible_form_data.password_elements; On 2017/06/07 17:16:42, dvadym wrote: > On 2017/06/06 16:49:46, pkalinnikov wrote: > > This line is equivalent to what we had before the CL. > > > > However, shouldn't we do lines 435-445 here as well? My understanding is that > it > > is also a "local heuristic". > > Don't worry about these lines, they are not used. Probably we should remove > switch at all. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2917933002/diff/60001/chrome/renderer/autofil... File chrome/renderer/autofill/password_generation_agent_browsertest.cc (right): https://codereview.chromium.org/2917933002/diff/60001/chrome/renderer/autofil... chrome/renderer/autofill/password_generation_agent_browsertest.cc:611: // Only setting one of the two attributes doesn't trigger generation. This comment doesn't true anymore https://codereview.chromium.org/2917933002/diff/60001/chrome/renderer/autofil... chrome/renderer/autofill/password_generation_agent_browsertest.cc:622: ExpectGenerationAvailable("first_password", true); Could you please add here the checking form without username with new-password autocomplete attributes https://codereview.chromium.org/2917933002/diff/60001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2917933002/diff/60001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:747: .Utf8(WebString::UTF8ConversionMode::kStrictReplacingErrorsWithFFFD); Why it's better than base::ToLowerASCII(....)? https://codereview.chromium.org/2917933002/diff/60001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:752: return std::find_if(tokens.begin(), tokens.end(), Why do we need to replace call of base::ContainsValue?
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2917933002/diff/60001/chrome/renderer/autofil... File chrome/renderer/autofill/password_generation_agent_browsertest.cc (right): https://codereview.chromium.org/2917933002/diff/60001/chrome/renderer/autofil... chrome/renderer/autofill/password_generation_agent_browsertest.cc:611: // Only setting one of the two attributes doesn't trigger generation. On 2017/06/08 14:56:59, dvadym wrote: > This comment doesn't true anymore Fixed. https://codereview.chromium.org/2917933002/diff/60001/chrome/renderer/autofil... chrome/renderer/autofill/password_generation_agent_browsertest.cc:622: ExpectGenerationAvailable("first_password", true); On 2017/06/08 14:56:59, dvadym wrote: > Could you please add here the checking form without username with new-password > autocomplete attributes Done. https://codereview.chromium.org/2917933002/diff/60001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2917933002/diff/60001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:747: .Utf8(WebString::UTF8ConversionMode::kStrictReplacingErrorsWithFFFD); On 2017/06/08 14:56:59, dvadym wrote: > Why it's better than base::ToLowerASCII(....)? ToLowerASCII makes an unnecessary allocation/copy. Overall, this code makes 2 memory allocations less. This is a big deal performance-wise given how often the function is invoked. https://codereview.chromium.org/2917933002/diff/60001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:752: return std::find_if(tokens.begin(), tokens.end(), On 2017/06/08 14:56:59, dvadym wrote: > Why do we need to replace call of base::ContainsValue? Because |autocomplete_value| is not lower-cased, hence |token| neither.
Thanks for implementing this and making small speed refactorings! LGTM with small nits https://codereview.chromium.org/2917933002/diff/60001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2917933002/diff/60001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:747: .Utf8(WebString::UTF8ConversionMode::kStrictReplacingErrorsWithFFFD); On 2017/06/09 08:02:42, pkalinnikov wrote: > On 2017/06/08 14:56:59, dvadym wrote: > > Why it's better than base::ToLowerASCII(....)? > > ToLowerASCII makes an unnecessary allocation/copy. > > Overall, this code makes 2 memory allocations less. This is a big deal > performance-wise given how often the function is invoked. Acknowledged https://codereview.chromium.org/2917933002/diff/60001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:752: return std::find_if(tokens.begin(), tokens.end(), On 2017/06/09 08:02:42, pkalinnikov wrote: > On 2017/06/08 14:56:59, dvadym wrote: > > Why do we need to replace call of base::ContainsValue? > > Because |autocomplete_value| is not lower-cased, hence |token| neither. Acknowledged https://codereview.chromium.org/2917933002/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2917933002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:753: [value_in_lowercase](base::StringPiece token) { Nit: it looks that you can avoid copying value_in_lowercase and token, by value_in_lowercase -> const value_in_lowercase& and base::StringPiece -> const base::StringPiece& https://codereview.chromium.org/2917933002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:770: tokens.begin(), tokens.end(), [](base::StringPiece token) { Nit: base::StringPiece -> const base::StringPiece&
Thanks. Leaving this as is (see comments). Maxim, do you want to take a final look? https://codereview.chromium.org/2917933002/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2917933002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:753: [value_in_lowercase](base::StringPiece token) { On 2017/06/09 11:39:48, dvadym wrote: > Nit: it looks that you can avoid copying value_in_lowercase and token, by > value_in_lowercase -> const value_in_lowercase& and base::StringPiece -> const > base::StringPiece& I assume you mean [value_in_lowercase](base::StringPiece token) -> [&value_in_lowercase](const base::StringPiece& token). I vaguely remember there has been a discussion, according to which base::StringPiece should be passed by value. Moreover, I expect this find_if code anyway optimized by the compiler. https://codereview.chromium.org/2917933002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:770: tokens.begin(), tokens.end(), [](base::StringPiece token) { On 2017/06/09 11:39:47, dvadym wrote: > Nit: base::StringPiece -> const base::StringPiece& Same argument as above.
On 2017/06/09 11:53:23, pkalinnikov wrote: > Thanks. Leaving this as is (see comments). > > Maxim, do you want to take a final look? > > https://codereview.chromium.org/2917933002/diff/80001/components/autofill/con... > File components/autofill/content/renderer/password_form_conversion_utils.cc > (right): > > https://codereview.chromium.org/2917933002/diff/80001/components/autofill/con... > components/autofill/content/renderer/password_form_conversion_utils.cc:753: > [value_in_lowercase](base::StringPiece token) { > On 2017/06/09 11:39:48, dvadym wrote: > > Nit: it looks that you can avoid copying value_in_lowercase and token, by > > value_in_lowercase -> const value_in_lowercase& and base::StringPiece -> const > > base::StringPiece& > > I assume you mean [value_in_lowercase](base::StringPiece token) -> > [&value_in_lowercase](const base::StringPiece& token). > I vaguely remember there has been a discussion, according to which > base::StringPiece should be passed by value. > > Moreover, I expect this find_if code anyway optimized by the compiler. > > https://codereview.chromium.org/2917933002/diff/80001/components/autofill/con... > components/autofill/content/renderer/password_form_conversion_utils.cc:770: > tokens.begin(), tokens.end(), [](base::StringPiece token) { > On 2017/06/09 11:39:47, dvadym wrote: > > Nit: base::StringPiece -> const base::StringPiece& > > Same argument as above. LGTM. Thanks.
The CQ bit was checked by pkalinnikov@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2917933002/diff/80001/components/autofill/con... File components/autofill/content/renderer/password_form_conversion_utils.cc (right): https://codereview.chromium.org/2917933002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:753: [value_in_lowercase](base::StringPiece token) { On 2017/06/09 11:53:23, pkalinnikov wrote: > On 2017/06/09 11:39:48, dvadym wrote: > > Nit: it looks that you can avoid copying value_in_lowercase and token, by > > value_in_lowercase -> const value_in_lowercase& and base::StringPiece -> const > > base::StringPiece& > > I assume you mean [value_in_lowercase](base::StringPiece token) -> > [&value_in_lowercase](const base::StringPiece& token). > I vaguely remember there has been a discussion, according to which > base::StringPiece should be passed by value. > > Moreover, I expect this find_if code anyway optimized by the compiler. Acknowledged https://codereview.chromium.org/2917933002/diff/80001/components/autofill/con... components/autofill/content/renderer/password_form_conversion_utils.cc:770: tokens.begin(), tokens.end(), [](base::StringPiece token) { On 2017/06/09 11:53:23, pkalinnikov wrote: > On 2017/06/09 11:39:47, dvadym wrote: > > Nit: base::StringPiece -> const base::StringPiece& > > Same argument as above. Acknowledged
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497009382902860,
"parent_rev": "1a69b3b221aed4b1ad88d7446aa22cf3d879bfcf", "commit_rev":
"6121d9064193b5d9eef9092425909b4e6cec3bcb"}
Message was sent while issue was closed.
Description was changed from ========== [PasswordGeneration] Improve change/confirm password field detection. Use autocomplete attributes to discover change/confirm password fields when no PasswordFormGenerationData is found for a possible password form. BUG=716464 ========== to ========== [PasswordGeneration] Improve change/confirm password field detection. Use autocomplete attributes to discover change/confirm password fields when no PasswordFormGenerationData is found for a possible password form. BUG=716464 Review-Url: https://codereview.chromium.org/2917933002 Cr-Commit-Position: refs/heads/master@{#478250} Committed: https://chromium.googlesource.com/chromium/src/+/6121d9064193b5d9eef909242590... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/6121d9064193b5d9eef909242590... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
