Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1508293006:
Check url path as well as document title to detect formless autofill page (Closed)
|
Created:
5 years ago by Joe Mason Modified:
4 years, 11 months ago CC:
bondd+autofillwatch_chromium.org, browser-components-watch_chromium.org, chromium-reviews, darin-cc_chromium.org, estade+watch_chromium.org, jam, jdonnelly+autofillwatch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, rouslan+autofill_chromium.org, vabr+watchlistautofill_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck url path as well as document title to detect formless autofill page
Autofill checks the document title on page load to decide whether input
elements outside forms should be considered part of a checkout flow. Also
check the path part of the url for documents without a title tag.
Originally the title check used a case-insensitive regex but
http://crrev.com/1419373006 replaced that with a string compare for
performance. As a side effect this made the comparison case-sensitive.
This patch also restores the case-insensitive title comparison.
BUG=555010
Committed: https://crrev.com/46bebf29983b58a58732216a7ed5740a43024397
Cr-Commit-Position: refs/heads/master@{#369436}
Patch Set 1 #Patch Set 2 : Fix non-English detection #
Total comments: 4
Patch Set 3 : Get rid of UTF8 conversion #Patch Set 4 : Fix typo in comment #
Total comments: 8
Patch Set 5 : Add non-ASCII tests #
Total comments: 7
Patch Set 6 : Rebase and fix nit #Patch Set 7 : Fix more nits: use ternary operator, make param name match doc comment[H #Patch Set 8 : Fix trybot warnings #Patch Set 9 : More trybot warnings #Patch Set 10 : Rebaseline heuristic test output #
Messages
Total messages: 56 (21 generated)
joenotcharles@chromium.org changed reviewers: + estade@chromium.org, sky@chromium.org
estade@chromium.org: Please review all changes. sky@chromium.org: Please review changes in content/public/test/render_view_test.*
https://codereview.chromium.org/1508293006/diff/20001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/1508293006/diff/20001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1446: if (title.find(keyword) != base::string16::npos || I think you should use std::find_if with base::LowerCaseEqualsASCII
https://codereview.chromium.org/1508293006/diff/20001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/1508293006/diff/20001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1446: if (title.find(keyword) != base::string16::npos || On 2015/12/10 19:34:00, Evan Stade wrote: > I think you should use std::find_if with base::LowerCaseEqualsASCII That would be less efficient since we need to convert title each time through the loop. It probably doesn't make much difference, but what's the benefit?
https://codereview.chromium.org/1508293006/diff/20001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/1508293006/diff/20001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1446: if (title.find(keyword) != base::string16::npos || On 2015/12/10 20:06:58, joenotcharles wrote: > On 2015/12/10 19:34:00, Evan Stade wrote: > > I think you should use std::find_if with base::LowerCaseEqualsASCII > > That would be less efficient since we need to convert title each time through > the loop. It probably doesn't make much difference, but what's the benefit? You're right, probably not a huge difference, but fewer lines of code and no utf16->utf8 conversion (which is much more costly than Z->z).
On 2015/12/10 21:21:11, Evan Stade wrote: > You're right, probably not a huge difference, but fewer lines of code and no > utf16->utf8 conversion (which is much more costly than Z->z). Good point. I didn't realize the utf16->utf8 converison could be avoided. I gave it a try, but I can't get the various string conversions + iterator magic to work out and I don't think it's worth spending much more time on. I suggest committing like this with a TODO.
On 2015/12/10 23:10:09, joenotcharles wrote: > On 2015/12/10 21:21:11, Evan Stade wrote: > > You're right, probably not a huge difference, but fewer lines of code and no > > utf16->utf8 conversion (which is much more costly than Z->z). > > Good point. I didn't realize the utf16->utf8 converison could be avoided. > > I gave it a try, but I can't get the various string conversions + iterator magic > to work out and I don't think it's worth spending much more time on. I suggest > committing like this with a TODO. Sorry, I don't think that's a good idea. It doesn't seem like something we'd come back to any time soon.
https://codereview.chromium.org/1508293006/diff/20001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/1508293006/diff/20001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1446: if (title.find(keyword) != base::string16::npos || On 2015/12/10 21:21:11, Evan Stade wrote: > On 2015/12/10 20:06:58, joenotcharles wrote: > > On 2015/12/10 19:34:00, Evan Stade wrote: > > > I think you should use std::find_if with base::LowerCaseEqualsASCII > > > > That would be less efficient since we need to convert title each time through > > the loop. It probably doesn't make much difference, but what's the benefit? > > You're right, probably not a huge difference, but fewer lines of code and no > utf16->utf8 conversion (which is much more costly than Z->z). Done.
estade@chromium.org changed reviewers: + jshin@chromium.org
OK, so I guess my squeamishness comes from trying to convert to ASCII when a string may not be ASCII. Some functions CHECK when this happens. Perhaps jshin can comment whether if this code is acceptable/best. Adding tests to cover a wider range of characters would also be useful. https://codereview.chromium.org/1508293006/diff/60001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/1508293006/diff/60001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:202: remove extra newline https://codereview.chromium.org/1508293006/diff/60001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:2534: "<HEAD><TITLE>delivery address</TITLE></HEAD>" add test that would have caught this regression? (i.e. ADDRESS) https://codereview.chromium.org/1508293006/diff/60001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:2689: "<TITLE>This title has nothing to do with autofill</TITLE>"); please add some non-ascii test cases https://codereview.chromium.org/1508293006/diff/60001/content/public/test/ren... File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/1508293006/diff/60001/content/public/test/ren... content/public/test/render_view_test.cc:131: wrapped_loader_->loadSynchronously(request, response, error, data); ternary operator https://codereview.chromium.org/1508293006/diff/60001/content/public/test/ren... content/public/test/render_view_test.cc:142: if (html.empty()) ternary operator
On 2015/12/11 23:44:45, Evan Stade wrote: > OK, so I guess my squeamishness comes from trying to convert to ASCII when a > string may not be ASCII. Some functions CHECK when this happens. Perhaps jshin > can comment whether if this code is acceptable/best. Adding tests to cover a > wider range of characters would also be useful. ToLowerASCII doesn't convert anything - it just lowercases all characters with code points in the ASCII range, and leaves all others untouched. (Which is all we need for this function, because the strings we're comparing to are all lowercase ASCII.) Note that's "code points in the ASCII range", not "bytes in the ASCII range" so for 16-bit strings it works on char16's without conversion.
https://codereview.chromium.org/1508293006/diff/60001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/1508293006/diff/60001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:202: On 2015/12/11 23:44:45, Evan Stade wrote: > remove extra newline Done. https://codereview.chromium.org/1508293006/diff/60001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:2534: "<HEAD><TITLE>delivery address</TITLE></HEAD>" On 2015/12/11 23:44:45, Evan Stade wrote: > add test that would have caught this regression? (i.e. ADDRESS) I already added a mixed-case title to kUnownedFormHtml to catch the regression. https://codereview.chromium.org/1508293006/diff/60001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:2689: "<TITLE>This title has nothing to do with autofill</TITLE>"); On 2015/12/11 23:44:45, Evan Stade wrote: > please add some non-ascii test cases Done.
Description was changed from ========== Check url path as well as document title to detect formless autofill page Autofill checks the document title on page load to decide whether input elements outside forms should be considered part of a checkout flow. Also check the path part of the url for documents without a title tag. Originally the title check used a case-insensitive regex but http://crrev.com/1419373006 replaced that with a string compare for performance. As a side effect this made the comparison case-sensitive. This patch also restores the case-insensitive title comparison. BUG=555010 ========== to ========== Check url path as well as document title to detect formless autofill page Autofill checks the document title on page load to decide whether input elements outside forms should be considered part of a checkout flow. Also check the path part of the url for documents without a title tag. Originally the title check used a case-insensitive regex but http://crrev.com/1419373006 replaced that with a string compare for performance. As a side effect this made the comparison case-sensitive. This patch also restores the case-insensitive title comparison. BUG=555010 ==========
joenotcharles@chromium.org changed reviewers: + phajdan.jr@chromium.org - sky@chromium.org
Added phajdan.jr since sky is OOO - please review changes in content/public/test/render_view_test.*
ping jshin, see comment 10 and previous discussion about form_autofill_util.cc
Sorry for the delay. I'll look at what Evan talked about in the next comment. https://codereview.chromium.org/1508293006/diff/80001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/1508293006/diff/80001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1412: // counts as English. It'd better to treat an empty 'lang' as in the current UI language. In the future, we may also consider detecting the language (using CLD(2)) . If translation feature is on, the language detection must have been run (already) . If that can be retrieved here, it'd be nice. https://codereview.chromium.org/1508293006/diff/80001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1417: !base::StartsWith(lang, "en", base::CompareCase::INSENSITIVE_ASCII)) { This assumes that there is no language code (3-letter) that starts wtih 'en' (other than English). To be more robust, either use ui/base/l10n_util.h (GetLanguage) or use ICU's locale API.
Sorry for the delay. I'll look at what Evan talked about in the next comment.
https://codereview.chromium.org/1508293006/diff/80001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/1508293006/diff/80001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:373: expected.value = ASCIIToUTF16(field_cases[i].initial_value); It's not in this CL, but why is ASCIIToUTF16 used here instead of UTF8ToUTF16? Is there a guarantee that field_cases[i].{name, initial_value} are always ASCII? https://codereview.chromium.org/1508293006/diff/80001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:377: form_data.fields[i].value = ASCIIToUTF16(field_cases[i].autofill_value); ditto here and elsewhere (where the input to ASCIIToUTF16 is a variable instead of an ASCII constant string literal). https://codereview.chromium.org/1508293006/diff/80001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/1508293006/diff/80001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1408: WebElement html_element = document.documentElement(); Although not very common, there are documents with different sections tagged with different languages. If we know the form element to deal with, we should get 'lang' corresponding to that element instead of 'html' element.
https://codereview.chromium.org/1508293006/diff/80001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/1508293006/diff/80001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:377: form_data.fields[i].value = ASCIIToUTF16(field_cases[i].autofill_value); On 2015/12/16 21:14:04, jshin (jungshik at g) wrote: > ditto here and elsewhere (where the input to ASCIIToUTF16 is a variable instead > of an ASCII constant string literal). I assume it's because this is a test file and all test cases are ASCII :o
On 2015/12/16 22:21:35, Evan Stade wrote: > https://codereview.chromium.org/1508293006/diff/80001/chrome/renderer/autofil... > File chrome/renderer/autofill/form_autofill_browsertest.cc (right): > > https://codereview.chromium.org/1508293006/diff/80001/chrome/renderer/autofil... > chrome/renderer/autofill/form_autofill_browsertest.cc:377: > form_data.fields[i].value = ASCIIToUTF16(field_cases[i].autofill_value); > On 2015/12/16 21:14:04, jshin (jungshik at g) wrote: > > ditto here and elsewhere (where the input to ASCIIToUTF16 is a variable > instead > > of an ASCII constant string literal). > > I assume it's because this is a test file and all test cases are ASCII :o ok. If there's a non-ascii input, debug build will crash anyway.
On 2015/12/16 21:14:04, jshin (out until Jan 3) wrote: > https://codereview.chromium.org/1508293006/diff/80001/chrome/renderer/autofil... > File chrome/renderer/autofill/form_autofill_browsertest.cc (right): > > https://codereview.chromium.org/1508293006/diff/80001/chrome/renderer/autofil... > chrome/renderer/autofill/form_autofill_browsertest.cc:373: expected.value = > ASCIIToUTF16(field_cases[i].initial_value); > It's not in this CL, but why is ASCIIToUTF16 used here instead of UTF8ToUTF16? > Is there a guarantee that field_cases[i].{name, initial_value} are always ASCII? > > https://codereview.chromium.org/1508293006/diff/80001/chrome/renderer/autofil... > chrome/renderer/autofill/form_autofill_browsertest.cc:377: > form_data.fields[i].value = ASCIIToUTF16(field_cases[i].autofill_value); > ditto here and elsewhere (where the input to ASCIIToUTF16 is a variable instead > of an ASCII constant string literal). > > https://codereview.chromium.org/1508293006/diff/80001/components/autofill/con... > File components/autofill/content/renderer/form_autofill_util.cc (right): > > https://codereview.chromium.org/1508293006/diff/80001/components/autofill/con... > components/autofill/content/renderer/form_autofill_util.cc:1408: WebElement > html_element = document.documentElement(); > Although not very common, there are documents with different sections tagged > with different languages. If we know the form element to deal with, we should > get 'lang' corresponding to that element instead of 'html' element. I filed crbug.com/572973 for these issues. But since they're in existing code, they're out of scope for this fix. I don't want to block an improvement which will fix existing sites (m.staples.com) while waiting for further improvements that could be made, and I've moved on to other tasks so I don't have time to make those improvements. Do you have any other concerns that are blocking this commit?
> I filed crbug.com/572973 for these issues. Sorry, I replied to the wrong comment. crbug.com/572973 covers the fragile lang handling jshin noted.
Another friendly ping.
sorry, lgtm w/nit https://codereview.chromium.org/1508293006/diff/80001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/1508293006/diff/80001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1458: return UnownedFormElementsAndFieldSetsToFormData( seems like you can just stick this inside the loop (where you have found = true) and get rid of |found|
content/public/test LGTM
The CQ bit was checked by joenotcharles@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508293006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508293006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by joenotcharles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1508293006/#ps120001 (title: "Fix more nits: use ternary operator, make param name match doc comment[H")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508293006/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508293006/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by joenotcharles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1508293006/#ps140001 (title: "Fix trybot warnings")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508293006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508293006/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by joenotcharles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1508293006/#ps160001 (title: "More trybot warnings")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508293006/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508293006/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by joenotcharles@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508293006/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508293006/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by joenotcharles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from phajdan.jr@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1508293006/#ps180001 (title: " Rebaseline heuristic test output")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508293006/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508293006/180001
Message was sent while issue was closed.
Description was changed from ========== Check url path as well as document title to detect formless autofill page Autofill checks the document title on page load to decide whether input elements outside forms should be considered part of a checkout flow. Also check the path part of the url for documents without a title tag. Originally the title check used a case-insensitive regex but http://crrev.com/1419373006 replaced that with a string compare for performance. As a side effect this made the comparison case-sensitive. This patch also restores the case-insensitive title comparison. BUG=555010 ========== to ========== Check url path as well as document title to detect formless autofill page Autofill checks the document title on page load to decide whether input elements outside forms should be considered part of a checkout flow. Also check the path part of the url for documents without a title tag. Originally the title check used a case-insensitive regex but http://crrev.com/1419373006 replaced that with a string compare for performance. As a side effect this made the comparison case-sensitive. This patch also restores the case-insensitive title comparison. BUG=555010 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Check url path as well as document title to detect formless autofill page Autofill checks the document title on page load to decide whether input elements outside forms should be considered part of a checkout flow. Also check the path part of the url for documents without a title tag. Originally the title check used a case-insensitive regex but http://crrev.com/1419373006 replaced that with a string compare for performance. As a side effect this made the comparison case-sensitive. This patch also restores the case-insensitive title comparison. BUG=555010 ========== to ========== Check url path as well as document title to detect formless autofill page Autofill checks the document title on page load to decide whether input elements outside forms should be considered part of a checkout flow. Also check the path part of the url for documents without a title tag. Originally the title check used a case-insensitive regex but http://crrev.com/1419373006 replaced that with a string compare for performance. As a side effect this made the comparison case-sensitive. This patch also restores the case-insensitive title comparison. BUG=555010 Committed: https://crrev.com/46bebf29983b58a58732216a7ed5740a43024397 Cr-Commit-Position: refs/heads/master@{#369436} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/46bebf29983b58a58732216a7ed5740a43024397 Cr-Commit-Position: refs/heads/master@{#369436} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
