Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2730383003:
Improve autofill form matching. (Closed)
|
Created:
3 years, 9 months ago by sense (YandexTeam) Modified:
3 years, 9 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove autofill form matching.
* Fix form updating logic if the field heuristics failed but the server returned correct field types.
* Remove the check for the <option> value attribute length because some websites can use it as a storage for JS-backed logic.
* Add form matching using form_signature, this improves matching of forms with the empty action attribute.
BUG=699004
TEST=components_unittests --gtest_filter=Autofill*
R=vabr@chromium.org, mathp@chromium.org
Review-Url: https://codereview.chromium.org/2730383003
Cr-Commit-Position: refs/heads/master@{#456707}
Committed: https://chromium.googlesource.com/chromium/src/+/7eaf2657fde72f7cc64482cfb0d701bbaf34648a
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address review issues. #
Total comments: 1
Patch Set 3 : Move DetermineHeuristicTypes after the add. Rebase. #
Messages
Total messages: 30 (19 generated)
https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.cc (left): https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1781: // Add the new or updated form to our cache. This logic is replaced by the call to ParseForm(). https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1788: std::map<base::string16, const AutofillField*> cached_fields; This update logic is replaced by the call to FormStructure::UpdateFromCache(). This code didn't perform the call to UpdateAutofillCount(). Without it the update breaks the count of autofillable fields and the whole form becomes non-autofillable if there were only server defined field types. Also we should keep form_signature here as does UpdateFromCache(). https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1909: if (form_structures_.size() >= kMaxFormCacheSize) Previously this check was only in UpdateCachedForm. Now it also applies to the initial ParseForms() call (OnFormsSeen->ParseForms). https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... File components/autofill/core/browser/form_structure.cc (left): https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... components/autofill/core/browser/form_structure.cc:665: DCHECK_EQ(cached_form.form_name_, form_name_); These DCHECKs were removed because the form now can be matched using form_signature. https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... components/autofill/core/browser/form_structure.cc:633: cached_fields[field->unique_name()] = field; unique_name() is logically the same as FormSignatureAsStr(), but it doesn't perform calculation each time it's called. This variant was used in AutofillManager::UpdateCachedForm(). If there is some hidden quirks that I don't know about, please correct me. https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/co... File components/autofill/core/common/autofill_data_validation.cc (left): https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/co... components/autofill/core/common/autofill_data_validation.cc:38: IsValidString16Vector(field.option_values) && This is described in the issue: https://bugs.chromium.org/p/chromium/issues/detail?id=699004
I'm leaving for holiday shortly, so to avoid blocking this review I'm explicitly punting to mathp@ or the rest of the autofill team. I'm happy to review next week if you want me to, but do not want to delay you for that. Cheers, Vaclav
Description was changed from ========== Improve autofill form matching. * Fix form updating logic if the field heuristics failed but the server returned correct field types. * Remove the check for the <option> value attribute length because some websites can use it as a storage for JS-backed logic. * Add form matching using form_signature, this improves matching of forms with the empty action attribute. BUG=699004 TEST=components_unittests --gtest_filter=Autofill* R=vabr@chromium.org, mathp@chromium.org ========== to ========== Improve autofill form matching. * Fix form updating logic if the field heuristics failed but the server returned correct field types. * Remove the check for the <option> value attribute length because some websites can use it as a storage for JS-backed logic. * Add form matching using form_signature, this improves matching of forms with the empty action attribute. BUG=699004 TEST=components_unittests --gtest_filter=Autofill* R=vabr@chromium.org, mathp@chromium.org ==========
vabr@chromium.org changed reviewers: - vabr@chromium.org
mathp@chromium.org changed reviewers: + sebsg@chromium.org - mathp@chromium.org
mathp@chromium.org changed reviewers: + mathp@chromium.org
Over to +sebsg, client-side goto person!
Overall looks good. I have a 2 questions see inline. Also could you please run the tests? in your terminal where you uploaded this patch just type "git cl try" Thanks! https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1785: return cached_form != nullptr; Can this change the return value compared to before? Before, if (form_structures_.size() >= kMaxFormCacheSize) we returned false but here if (form_structures_.size() >= kMaxFormCacheSize) but cached_form != nullptr we will return true? https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1854: if (!ParseForm(form, true, &form_structure)) I'm not sure why this needs to be true here? It means that this can continue if we already have the form which is a logic that I do not see in the current code? Could you explain why it is better to have it? Thanks!
https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1785: return cached_form != nullptr; On 2017/03/08 18:32:08, sebsg wrote: > Can this change the return value compared to before? Before, if > (form_structures_.size() >= kMaxFormCacheSize) we returned false but here if > (form_structures_.size() >= kMaxFormCacheSize) but cached_form != nullptr we > will return true? Yes, this change is incorrect. We shouldn't return the form if it's need to be updated but we failed to do that. https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... components/autofill/core/browser/autofill_manager.cc:1854: if (!ParseForm(form, true, &form_structure)) On 2017/03/08 18:32:08, sebsg wrote: > I'm not sure why this needs to be true here? It means that this can continue if > we already have the form which is a logic that I do not see in the current code? > Could you explain why it is better to have it? Thanks! The check for duplicates is from our internal improvements. It's not meant to be in this CL. I'll remove it, sorry for that.
The CQ bit was checked by sense@yandex-team.ru 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.
On 2017/03/08 18:32:09, sebsg wrote: > Overall looks good. I have a 2 questions see inline. Also could you please run > the tests? in your terminal where you uploaded this patch just type "git cl try" > > Thanks! > > https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... > components/autofill/core/browser/autofill_manager.cc:1785: return cached_form != > nullptr; > Can this change the return value compared to before? Before, if > (form_structures_.size() >= kMaxFormCacheSize) we returned false but here if > (form_structures_.size() >= kMaxFormCacheSize) but cached_form != nullptr we > will return true? > > https://codereview.chromium.org/2730383003/diff/1/components/autofill/core/br... > components/autofill/core/browser/autofill_manager.cc:1854: if (!ParseForm(form, > true, &form_structure)) > I'm not sure why this needs to be true here? It means that this can continue if > we already have the form which is a logic that I do not see in the current code? > Could you explain why it is better to have it? Thanks! I made required fixes to the CL and ran tests, everything looks good. PTAL, thanks!
One last small comment :) https://codereview.chromium.org/2730383003/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2730383003/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1914: form_structure->DetermineHeuristicTypes(); Would you mind moving this after the add like in the original code? Thanks!
The CQ bit was checked by sense@yandex-team.ru 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 sense@yandex-team.ru 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.
On 2017/03/13 12:51:22, sebsg wrote: > One last small comment :) > > https://codereview.chromium.org/2730383003/diff/20001/components/autofill/cor... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/2730383003/diff/20001/components/autofill/cor... > components/autofill/core/browser/autofill_manager.cc:1914: > form_structure->DetermineHeuristicTypes(); > Would you mind moving this after the add like in the original code? Thanks! Done!
LGTM!
The CQ bit was checked by sense@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489505220379720,
"parent_rev": "96ff8f1f6f00e57db01f58b3bbc4ee0fe3e90ee7", "commit_rev":
"7eaf2657fde72f7cc64482cfb0d701bbaf34648a"}
Message was sent while issue was closed.
Description was changed from ========== Improve autofill form matching. * Fix form updating logic if the field heuristics failed but the server returned correct field types. * Remove the check for the <option> value attribute length because some websites can use it as a storage for JS-backed logic. * Add form matching using form_signature, this improves matching of forms with the empty action attribute. BUG=699004 TEST=components_unittests --gtest_filter=Autofill* R=vabr@chromium.org, mathp@chromium.org ========== to ========== Improve autofill form matching. * Fix form updating logic if the field heuristics failed but the server returned correct field types. * Remove the check for the <option> value attribute length because some websites can use it as a storage for JS-backed logic. * Add form matching using form_signature, this improves matching of forms with the empty action attribute. BUG=699004 TEST=components_unittests --gtest_filter=Autofill* R=vabr@chromium.org, mathp@chromium.org Review-Url: https://codereview.chromium.org/2730383003 Cr-Commit-Position: refs/heads/master@{#456707} Committed: https://chromium.googlesource.com/chromium/src/+/7eaf2657fde72f7cc64482cfb0d7... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/7eaf2657fde72f7cc64482cfb0d7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
