Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2750323003:
[Password Manager] Replace WebInputElement.setValue with WebInputElement.setAutofillValue (Closed)
|
Created:
3 years, 9 months ago by kolos1 Modified:
3 years, 9 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[Password Manager] Replace WebInputElement.setValue(element, true /* send events*/) with WebInputElement.setAutofillValue
Sometimes sites doesn't recognize that the password manager changed field values. setAutofillValue sends more events than setValue.
This CL (https://codereview.chromium.org/1955963002/) makes migration for autofill code.
BUG=676267, 501614
Review-Url: https://codereview.chromium.org/2750323003
Cr-Commit-Position: refs/heads/master@{#458459}
Committed: https://chromium.googlesource.com/chromium/src/+/4a42d386112ff65fe17baa0c633e5ccc44989aa5
Patch Set 1 #Patch Set 2 : #
Total comments: 6
Patch Set 3 : Typo in comment #Patch Set 4 : Fixed Win tests #
Total comments: 2
Patch Set 5 : Fixed a comment #
Messages
Total messages: 70 (61 generated)
Description was changed from ========== [Password Manager] Replace WebInputElement.setValue(element, true /* send events*/) with WebInputElement.setAutofillValue Sometimes sites doesn't see that the password manager changed field values. setAutofillValue sends more events than setValue. BUG=676267 ========== to ========== [Password Manager] Replace WebInputElement.setValue(element, true /* send events*/) with WebInputElement.setAutofillValue Sometimes sites doesn't see that the password manager changed field values. setAutofillValue sends more events than setValue. BUG=676267 ==========
Description was changed from ========== [Password Manager] Replace WebInputElement.setValue(element, true /* send events*/) with WebInputElement.setAutofillValue Sometimes sites doesn't see that the password manager changed field values. setAutofillValue sends more events than setValue. BUG=676267 ========== to ========== [Password Manager] Replace WebInputElement.setValue(element, true /* send events*/) with WebInputElement.setAutofillValue Sometimes sites doesn't recognize that the password manager changed field values. setAutofillValue sends more events than setValue. BUG=676267 ==========
Description was changed from ========== [Password Manager] Replace WebInputElement.setValue(element, true /* send events*/) with WebInputElement.setAutofillValue Sometimes sites doesn't recognize that the password manager changed field values. setAutofillValue sends more events than setValue. BUG=676267 ========== to ========== [Password Manager] Replace WebInputElement.setValue(element, true /* send events*/) with WebInputElement.setAutofillValue Sometimes sites doesn't recognize that the password manager changed field values. setAutofillValue sends more events than setValue. This CL (https://codereview.chromium.org/1955963002/) makes migration for autofill code. BUG=676267 ==========
Description was changed from ========== [Password Manager] Replace WebInputElement.setValue(element, true /* send events*/) with WebInputElement.setAutofillValue Sometimes sites doesn't recognize that the password manager changed field values. setAutofillValue sends more events than setValue. This CL (https://codereview.chromium.org/1955963002/) makes migration for autofill code. BUG=676267 ========== to ========== [Password Manager] Replace WebInputElement.setValue(element, true /* send events*/) with WebInputElement.setAutofillValue Sometimes sites doesn't recognize that the password manager changed field values. setAutofillValue sends more events than setValue. This CL (https://codereview.chromium.org/1955963002/) makes migration for autofill code. BUG=676267, 501614 ==========
kolos@chromium.org changed reviewers: + sebsg@chromium.org, vabr@chromium.org
sebsg@: Please review a change in form_cache.cc. Seems setValue should be replaced with setAutofillValue vabr@: Please review all files except form_cache.cc. Thank you. Regards, Maxim
Thanks Maxim for the patch (and Seb for the pointer to the autofill code). This LGTM with a few comments. Cheers, Vaclav https://codereview.chromium.org/2750323003/diff/20001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2750323003/diff/20001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:542: void CheckIfEventsAreCalled(const std::vector<std::string> checkers, Please pass the vector by (const) reference, not by value. https://codereview.chromium.org/2750323003/diff/20001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:947: // TODO(isherman): Re-enable this check once http://crbug.com/333144 is fixed. This comment looks obsoleted by your changes. https://codereview.chromium.org/2750323003/diff/20001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:990: event_checkers.erase(std::remove(event_checkers.begin(), event_checkers.end(), Please use base::Erase instead (this is a recent change in style, there was a discussion on chromium-dev a few days ago).
Thanks for review Vaclav! https://codereview.chromium.org/2750323003/diff/20001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2750323003/diff/20001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:542: void CheckIfEventsAreCalled(const std::vector<std::string> checkers, On 2017/03/17 12:01:06, vabr (Chromium) wrote: > Please pass the vector by (const) reference, not by value. Done. https://codereview.chromium.org/2750323003/diff/20001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:947: // TODO(isherman): Re-enable this check once http://crbug.com/333144 is fixed. On 2017/03/17 12:01:07, vabr (Chromium) wrote: > This comment looks obsoleted by your changes. Done. https://codereview.chromium.org/2750323003/diff/20001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:990: event_checkers.erase(std::remove(event_checkers.begin(), event_checkers.end(), On 2017/03/17 12:01:06, vabr (Chromium) wrote: > Please use base::Erase instead (this is a recent change in style, there was a > discussion on chromium-dev a few days ago). Done.
The CQ bit was checked by kolos@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by kolos@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by kolos@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...
Patchset #4 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by kolos@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 kolos@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #4 (id:160001) has been deleted
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
Patchset #4 (id:220001) has been deleted
Patchset #4 (id:240001) has been deleted
Patchset #4 (id:260001) has been deleted
Patchset #4 (id:280001) has been deleted
Patchset #4 (id:300001) has been deleted
Patchset #4 (id:320001) has been deleted
Patchset #4 (id:180002) has been deleted
Patchset #4 (id:340001) has been deleted
Patchset #4 (id:360001) has been deleted
Patchset #4 (id:380001) has been deleted
Patchset #4 (id:400001) has been deleted
Patchset #4 (id:420001) has been deleted
Patchset #4 (id:440001) has been deleted
Patchset #4 (id:460001) has been deleted
Patchset #4 (id:480001) has been deleted
Patchset #4 (id:500001) has been deleted
Patchset #4 (id:520001) has been deleted
Patchset #4 (id:540001) has been deleted
The CQ bit was checked by kolos@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.
Friendly ping. There was a tricky bug in my implementation. base::StringPrintF failed only on Windows w/o any error messages (crbug.com/703506). Now it is fixed. sebsg@: Please review if you are ok with changes in form_cache.cc. Seems setValue should be replaced with setAutofillValue vabr@: Now scripts are injected into the page. Usage of StringPrintF has been changed in password_Generation_utils. Fixed some comments. Thanks. Regards, Maxim
Still LGTM. Cheers, Vaclav https://codereview.chromium.org/2750323003/diff/560001/chrome/renderer/autofi... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2750323003/diff/560001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:940: // A JavaScript events should have been triggered for the username, but not nit: Remove "A": A JavaScript events -> JavaScript events
https://codereview.chromium.org/2750323003/diff/560001/chrome/renderer/autofi... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2750323003/diff/560001/chrome/renderer/autofi... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:940: // A JavaScript events should have been triggered for the username, but not On 2017/03/21 07:57:30, vabr (Chromium) wrote: > nit: Remove "A": A JavaScript events -> JavaScript events Done.
Sweet! LGTM!
The CQ bit was checked by kolos@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.
The CQ bit was checked by kolos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org Link to the patchset: https://codereview.chromium.org/2750323003/#ps580001 (title: "Fixed a comment")
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": 580001, "attempt_start_ts": 1490117058968290,
"parent_rev": "31e38516550d24160a228adde328ec7ad9c7ac54", "commit_rev":
"4a42d386112ff65fe17baa0c633e5ccc44989aa5"}
Message was sent while issue was closed.
Description was changed from ========== [Password Manager] Replace WebInputElement.setValue(element, true /* send events*/) with WebInputElement.setAutofillValue Sometimes sites doesn't recognize that the password manager changed field values. setAutofillValue sends more events than setValue. This CL (https://codereview.chromium.org/1955963002/) makes migration for autofill code. BUG=676267, 501614 ========== to ========== [Password Manager] Replace WebInputElement.setValue(element, true /* send events*/) with WebInputElement.setAutofillValue Sometimes sites doesn't recognize that the password manager changed field values. setAutofillValue sends more events than setValue. This CL (https://codereview.chromium.org/1955963002/) makes migration for autofill code. BUG=676267, 501614 Review-Url: https://codereview.chromium.org/2750323003 Cr-Commit-Position: refs/heads/master@{#458459} Committed: https://chromium.googlesource.com/chromium/src/+/4a42d386112ff65fe17baa0c633e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:580001) as https://chromium.googlesource.com/chromium/src/+/4a42d386112ff65fe17baa0c633e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
