Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2814093002:
[Password Manager] Send a request to the password store if there is a password field on a page (Closed)
|
Created:
3 years, 8 months ago by kolos1 Modified:
3 years, 8 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] Send a request to the password store if there is a password field on a page
At this time, if our algorithms failed to create |PasswordForm|, we don't send a request to the store. It is why Chrome fails to fill credentials even if there are saved password.
If saved creadentials have been requested from the store, it would be possible to fill passwords by click on a field.
BUG=447017
Review-Url: https://codereview.chromium.org/2814093002
Cr-Commit-Position: refs/heads/master@{#464428}
Committed: https://chromium.googlesource.com/chromium/src/+/982e14088763bcda41c1acf201ccd1b66e49dfe0
Patch Set 1 : #
Total comments: 14
Patch Set 2 : Changes addressed to reviewer's comments #
Total comments: 2
Patch Set 3 : Changes addressed to reviewers' comments #Patch Set 4 : Reset |sent_request_to_store_| when the frame is closing #Patch Set 5 : Added more tests #
Messages
Total messages: 39 (20 generated)
kolos@chromium.org changed reviewers: + dvadym@chromium.org
Hi Vadym, Please review this CL. Regards, Maxim
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) 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.
kolos@chromium.org changed reviewers: + vabr@chromium.org
vabr@chromium.org: Please review changes in all files except password_autofill_agent.cc. Thanks. Regards, Maxi,
Hi Maxim, The whole patch LGTM, with comments. Thanks! Vaclav https://codereview.chromium.org/2814093002/diff/40001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2814093002/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:866: TEST_F(PasswordAutofillAgentTest, SendPasswordFormsTest) { I wonder if we should split this test so that each HTML sample has its own test. There is a constant timeout limit on one test, and we are increasing the probability of hitting it with every scenario we add. Also, if the test fails for some of the first scenarios, the others are not run at all. https://codereview.chromium.org/2814093002/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:897: ASSERT_TRUE(static_cast<bool>(fake_driver_.password_forms_parsed())); Just out of curiosity -- why is the explicit cast to bool needed? I though that base::Optional supports an implicit one. https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1140: // any forms visible, as this is also the code path that triggers showing optional nit: Replace "there are any forms visible" with "|password_forms|" are empty. That means the same, but is less confusing in connection to the variable name |only_visible| -- currently the reader might be surprised to see a block of code guarded by "if (only_possible)" to have a comment saying "regardless of ... visible". https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1140: // any forms visible, as this is also the code path that triggers showing nit: (1) let's not say infobar when it's bubble on on desktop, and (2) let's be more concrete about how the empty vector is used. Altogether I suggest this comment instead of the current block: Send the PasswordFormsRendered message regardless of whether |password_forms| is empty. The empty |password_forms| are a possible signal to the browser that a pending login attempt succeeded.
https://codereview.chromium.org/2814093002/diff/40001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2814093002/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:866: TEST_F(PasswordAutofillAgentTest, SendPasswordFormsTest) { On 2017/04/13 06:44:18, vabr (Chromium) wrote: > I wonder if we should split this test so that each HTML sample has its own test. > There is a constant timeout limit on one test, and we are increasing the > probability of hitting it with every scenario we add. Also, if the test fails > for some of the first scenarios, the others are not run at all. Splitted. https://codereview.chromium.org/2814093002/diff/40001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:897: ASSERT_TRUE(static_cast<bool>(fake_driver_.password_forms_parsed())); On 2017/04/13 06:44:18, vabr (Chromium) wrote: > Just out of curiosity -- why is the explicit cast to bool needed? I though that > base::Optional supports an implicit one. Removed casts. https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1140: // any forms visible, as this is also the code path that triggers showing On 2017/04/13 06:44:18, vabr (Chromium) wrote: > optional nit: Replace "there are any forms visible" with "|password_forms|" are > empty. That means the same, but is less confusing in connection to the variable > name |only_visible| -- currently the reader might be surprised to see a block of > code guarded by "if (only_possible)" to have a comment saying "regardless of ... > visible". Done.
On 2017/04/13 08:35:48, kolos1 wrote: > https://codereview.chromium.org/2814093002/diff/40001/chrome/renderer/autofil... > File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): > > https://codereview.chromium.org/2814093002/diff/40001/chrome/renderer/autofil... > chrome/renderer/autofill/password_autofill_agent_browsertest.cc:866: > TEST_F(PasswordAutofillAgentTest, SendPasswordFormsTest) { > On 2017/04/13 06:44:18, vabr (Chromium) wrote: > > I wonder if we should split this test so that each HTML sample has its own > test. > > There is a constant timeout limit on one test, and we are increasing the > > probability of hitting it with every scenario we add. Also, if the test fails > > for some of the first scenarios, the others are not run at all. > > Splitted. > > https://codereview.chromium.org/2814093002/diff/40001/chrome/renderer/autofil... > chrome/renderer/autofill/password_autofill_agent_browsertest.cc:897: > ASSERT_TRUE(static_cast<bool>(fake_driver_.password_forms_parsed())); > On 2017/04/13 06:44:18, vabr (Chromium) wrote: > > Just out of curiosity -- why is the explicit cast to bool needed? I though > that > > base::Optional supports an implicit one. > > Removed casts. > > https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:1140: // any > forms visible, as this is also the code path that triggers showing > On 2017/04/13 06:44:18, vabr (Chromium) wrote: > > optional nit: Replace "there are any forms visible" with "|password_forms|" > are > > empty. That means the same, but is less confusing in connection to the > variable > > name |only_visible| -- currently the reader might be surprised to see a block > of > > code guarded by "if (only_possible)" to have a comment saying "regardless of > ... > > visible". > > Done. Thanks for review Vaclav. Please take a look at changes.
Thanks for fixing this. In general looks good. Some comments below https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:607: for (blink::WebElement element = elements.FirstItem(); !element.IsNull(); Nit: blink::WebElement -> const blink::WebElement& https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1150: if (password_forms.empty() && HasPasswordField(*frame)) { I'd create a boolean flag |request_to_store_sent| in PasswordAutofillAgent, whether the request to store was sent and not to resend it again here. In this implementation we can sent requests to browser many times for the same frame. https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1151: std::unique_ptr<PasswordForm> password_form(new PasswordForm()); It looks that you don't need unique_ptr here, you can just create PasswordForm on stack, or probably even better to push_back an empty PasswordForm and then to set its fields .
Still LGTM. https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:607: for (blink::WebElement element = elements.FirstItem(); !element.IsNull(); On 2017/04/13 09:22:53, dvadym wrote: > Nit: blink::WebElement -> const blink::WebElement& FirstItem returns a (temporary) value, not reference. Keeping a const ref to the temporary might be OK (I believe there was some special rule in C++ about extending the lifetime of objects for const refs specifically), but why rely on magic if we can write the code to be clear? Note that there should be no copy construction involved, the assignment should just result in the returned temporary getting the name |element| and lifespan of the for-loop. (I tested this locally with a dummy .cc file.) https://codereview.chromium.org/2814093002/diff/60001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2814093002/diff/60001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1140: // |password_forms| is empty. The empty |password_forms| are a possible nit: Please re-flow the comment block to remove unnecessary white spaces (between // and the text should be just one space).
https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:607: for (blink::WebElement element = elements.FirstItem(); !element.IsNull(); it is not possible to declare the variable as const, since it is changed on every iteration. https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1150: if (password_forms.empty() && HasPasswordField(*frame)) { On 2017/04/13 09:22:53, dvadym wrote: > I'd create a boolean flag |request_to_store_sent| in PasswordAutofillAgent, > whether the request to store was sent and not to resend it again here. In this > implementation we can sent requests to browser many times for the same frame. Done. https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1151: std::unique_ptr<PasswordForm> password_form(new PasswordForm()); On 2017/04/13 09:22:53, dvadym wrote: > It looks that you don't need unique_ptr here, you can just create PasswordForm > on stack, or probably even better to push_back an empty PasswordForm and then to > set its fields . Done. https://codereview.chromium.org/2814093002/diff/60001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/2814093002/diff/60001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1140: // |password_forms| is empty. The empty |password_forms| are a possible On 2017/04/13 10:12:58, vabr (Chromium) wrote: > nit: Please re-flow the comment block to remove unnecessary white spaces > (between // and the text should be just one space). Done.
Hi Maxim, Please make sure that you also upload the new patch. :) On 2017/04/13 13:26:07, kolos1 wrote: > https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:607: for > (blink::WebElement element = elements.FirstItem(); !element.IsNull(); > it is not possible to declare the variable as const, since it is changed on > every iteration. > > https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:1150: if > (password_forms.empty() && HasPasswordField(*frame)) { > On 2017/04/13 09:22:53, dvadym wrote: > > I'd create a boolean flag |request_to_store_sent| in PasswordAutofillAgent, > > whether the request to store was sent and not to resend it again here. In this > > implementation we can sent requests to browser many times for the same frame. > > Done. > > https://codereview.chromium.org/2814093002/diff/40001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:1151: > std::unique_ptr<PasswordForm> password_form(new PasswordForm()); > On 2017/04/13 09:22:53, dvadym wrote: > > It looks that you don't need unique_ptr here, you can just create PasswordForm > > on stack, or probably even better to push_back an empty PasswordForm and then > to > > set its fields . > > Done. > > https://codereview.chromium.org/2814093002/diff/60001/components/autofill/con... > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/2814093002/diff/60001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:1140: // > |password_forms| is empty. The empty |password_forms| are a possible > On 2017/04/13 10:12:58, vabr (Chromium) wrote: > > nit: Please re-flow the comment block to remove unnecessary white spaces > > (between // and the text should be just one space). > > Done.
I did a couple of seconds ago :) On Thu, Apr 13, 2017 at 3:30 PM, <vabr@chromium.org> wrote: > Hi Maxim, > > Please make sure that you also upload the new patch. :) > > > On 2017/04/13 13:26:07, kolos1 wrote: > > > https://codereview.chromium.org/2814093002/diff/40001/ > components/autofill/content/renderer/password_autofill_agent.cc > > File components/autofill/content/renderer/password_autofill_agent.cc > (right): > > > > > https://codereview.chromium.org/2814093002/diff/40001/ > components/autofill/content/renderer/password_autofill_agent.cc#newcode607 > > components/autofill/content/renderer/password_autofill_agent.cc:607: for > > (blink::WebElement element = elements.FirstItem(); !element.IsNull(); > > it is not possible to declare the variable as const, since it is changed > on > > every iteration. > > > > > https://codereview.chromium.org/2814093002/diff/40001/ > components/autofill/content/renderer/password_autofill_ > agent.cc#newcode1150 > > components/autofill/content/renderer/password_autofill_agent.cc:1150: if > > (password_forms.empty() && HasPasswordField(*frame)) { > > On 2017/04/13 09:22:53, dvadym wrote: > > > I'd create a boolean flag |request_to_store_sent| in > PasswordAutofillAgent, > > > whether the request to store was sent and not to resend it again here. > In > this > > > implementation we can sent requests to browser many times for the same > frame. > > > > Done. > > > > > https://codereview.chromium.org/2814093002/diff/40001/ > components/autofill/content/renderer/password_autofill_ > agent.cc#newcode1151 > > components/autofill/content/renderer/password_autofill_agent.cc:1151: > > std::unique_ptr<PasswordForm> password_form(new PasswordForm()); > > On 2017/04/13 09:22:53, dvadym wrote: > > > It looks that you don't need unique_ptr here, you can just create > PasswordForm > > > on stack, or probably even better to push_back an empty PasswordForm > and > then > > to > > > set its fields . > > > > Done. > > > > > https://codereview.chromium.org/2814093002/diff/60001/ > components/autofill/content/renderer/password_autofill_agent.cc > > File components/autofill/content/renderer/password_autofill_agent.cc > (right): > > > > > https://codereview.chromium.org/2814093002/diff/60001/ > components/autofill/content/renderer/password_autofill_ > agent.cc#newcode1140 > > components/autofill/content/renderer/password_autofill_agent.cc:1140: > // > > |password_forms| is empty. The empty |password_forms| are a possible > > On 2017/04/13 10:12:58, vabr (Chromium) wrote: > > > nit: Please re-flow the comment block to remove unnecessary white > spaces > > > (between // and the text should be just one space). > > > > Done. > > > > https://codereview.chromium.org/2814093002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/04/13 13:30:49, vabr (Chromium) wrote: > Hi Maxim, > > Please make sure that you also upload the new patch. :) I saw it now, sorry for the noise. :) Still LGTM.
LGTM
Found a bug :) |sent_request_to_store_| should be resetted when the frame is closing. Otherwise, there will no new request when the tab is reloaded. Any more cases?
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.
Hi Vaclav, Could you take a quick look at new tests? I would like to land it before the branch point which is today. Sorry for rush. Regards, Maxim
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...
LGTM, thanks for finding & fixing the bug and adding tests for that! Vaclav
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks Vaclav for fast response.
Thanks Vaclav for fast response.
The CQ bit was checked by kolos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dvadym@chromium.org Link to the patchset: https://codereview.chromium.org/2814093002/#ps120001 (title: "Added more tests")
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": 120001, "attempt_start_ts": 1492099065268490,
"parent_rev": "45271937a3aeee0c06e267d33e75d7ea0bee9605", "commit_rev":
"982e14088763bcda41c1acf201ccd1b66e49dfe0"}
Message was sent while issue was closed.
Description was changed from ========== [Password Manager] Send a request to the password store if there is a password field on a page At this time, if our algorithms failed to create |PasswordForm|, we don't send a request to the store. It is why Chrome fails to fill credentials even if there are saved password. If saved creadentials have been requested from the store, it would be possible to fill passwords by click on a field. BUG=447017 ========== to ========== [Password Manager] Send a request to the password store if there is a password field on a page At this time, if our algorithms failed to create |PasswordForm|, we don't send a request to the store. It is why Chrome fails to fill credentials even if there are saved password. If saved creadentials have been requested from the store, it would be possible to fill passwords by click on a field. BUG=447017 Review-Url: https://codereview.chromium.org/2814093002 Cr-Commit-Position: refs/heads/master@{#464428} Committed: https://chromium.googlesource.com/chromium/src/+/982e14088763bcda41c1acf201cc... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001) as https://chromium.googlesource.com/chromium/src/+/982e14088763bcda41c1acf201cc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
