Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2604783002:
Do not show the update bubble for sync credentials when a username is unknown. (Closed)
|
Created:
3 years, 12 months ago by dvadym Modified:
3 years, 11 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDo not show the update bubble for sync credentials when a username is unknown.
This CL introduced a checking of |provisionally_saved_credentials| for sync credentials, instead of |pending_credentials|. That's more correct, since |provisionally_saved_credentials| is the submitted form, on other hand |pending_credentials| is a combination of already saved and submitted form. In case of saving both these forms are the same, but for updating they are different.
BUG=663896
Committed: https://crrev.com/68324d6be5b713650770ac5ff8a732eda4146916
Cr-Commit-Position: refs/heads/master@{#440844}
Patch Set 1 #
Total comments: 4
Patch Set 2 : test added #Patch Set 3 : comment updated #
Total comments: 15
Patch Set 4 : Addressed comments #Patch Set 5 : tiny fix #
Messages
Total messages: 26 (15 generated)
Description was changed from ========== Fix BUG=663896 ========== to ========== Do not show update bubble for sync credentials when username is unknown. BUG=663896 ==========
Description was changed from ========== Do not show update bubble for sync credentials when username is unknown. BUG=663896 ========== to ========== Do not show the update bubble for sync credentials when a username is unknown. This CL introduced check of |provisionally_saved_credentials| for sync credentials, instead of |pending_credentials|. That's more correct, since |provisionally_saved_credentials| is the submitted form, on other hand |pending_credentials| is a combination of already saved and submitted form. In case of saving both these forms are the same, but for updating they are different. BUG=663896 ==========
dvadym@chromium.org changed reviewers: + vabr@chromium.org
Description was changed from ========== Do not show the update bubble for sync credentials when a username is unknown. This CL introduced check of |provisionally_saved_credentials| for sync credentials, instead of |pending_credentials|. That's more correct, since |provisionally_saved_credentials| is the submitted form, on other hand |pending_credentials| is a combination of already saved and submitted form. In case of saving both these forms are the same, but for updating they are different. BUG=663896 ========== to ========== Do not show the update bubble for sync credentials when a username is unknown. This CL introduced a checking of |provisionally_saved_credentials| for sync credentials, instead of |pending_credentials|. That's more correct, since |provisionally_saved_credentials| is the submitted form, on other hand |pending_credentials| is a combination of already saved and submitted form. In case of saving both these forms are the same, but for updating they are different. BUG=663896 ==========
Hi Vaclav, Could you please review this CL? Regards, Vadym
Hi Vadym, The code looks good. But could we add a test? Cheers, Vaclav https://codereview.chromium.org/2604783002/diff/1/components/password_manager... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2604783002/diff/1/components/password_manager... components/password_manager/core/browser/password_form_manager.h:181: return provisionally_saved_form_ ? provisionally_saved_form_.get() This could be just: return provisionally_saved_form_.get(); https://codereview.chromium.org/2604783002/diff/1/components/password_manager... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/2604783002/diff/1/components/password_manager... components/password_manager/core/browser/password_manager.cc:683: const auto& credentials_to_check = As discussed offline, it should not happen that pending_credentials is not null while provisionally_saved_form is null (the former is derived from the latter). We should simply expect provisionally_saved_form to be non-null and perhaps document it with a DCHECK.
The CQ bit was checked by dvadym@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...
Thanks for comments Vaclav! I've addressed you comments and added a verification for the submitted form as argument of ShouldSave() in an existing test. https://codereview.chromium.org/2604783002/diff/1/components/password_manager... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2604783002/diff/1/components/password_manager... components/password_manager/core/browser/password_form_manager.h:181: return provisionally_saved_form_ ? provisionally_saved_form_.get() On 2016/12/27 13:17:11, vabr (Chromium) wrote: > This could be just: > return provisionally_saved_form_.get(); Done. https://codereview.chromium.org/2604783002/diff/1/components/password_manager... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/2604783002/diff/1/components/password_manager... components/password_manager/core/browser/password_manager.cc:683: const auto& credentials_to_check = On 2016/12/27 13:17:11, vabr (Chromium) wrote: > As discussed offline, it should not happen that pending_credentials is not null > while provisionally_saved_form is null (the former is derived from the latter). > We should simply expect provisionally_saved_form to be non-null and perhaps > document it with a DCHECK. Done.
Thanks, Vadym. LGTM with some comments. Cheers, Vaclav https://codereview.chromium.org/2604783002/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager.h:179: // Returns the provisionally saved form, if it exists otherwise nullptr. very small nit: Missing comma after "exists". https://codereview.chromium.org/2604783002/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:591: // CredentialsFilter::ReportFormLoginSuccess CredentialsFilter::ShouldSaved nit: Did you man to put "and" between CredentialsFilter::ReportFormLoginSuccess and CredentialsFilter::ShouldSave? Also: ShouldSaved -> ShouldSave https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:592: // should be called. The argument of ShouldSaved shold be submitted form. nit: submitted form -> the submitted form https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:594: PasswordForm form(MakeSimpleForm()); nit: Please rename to |stored_form|, because this form pretends to be returned from GetLogins. https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:598: // Let's make |observed_form| not to be equal to the from saved in the store. Simple phrasing of this comment would be: // Ensure that |observed_form| is different from |stored_form|. But it is not clear what is inside |stored_form|, so perhaps instead of the comment, why not do: EXPECT_NE(observed_form.username_value, stored_form.username_value); https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:598: // Let's make |observed_form| not to be equal to the from saved in the store. Also, it might be beneficial to explain _why_ we want those forms to be different: to ensure that it is the observed_form and not the stored_form what is passed to ShouldSave.
The CQ bit was checked by dvadym@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...
Thanks for review Vaclav! https://codereview.chromium.org/2604783002/diff/40001/components/password_man... File components/password_manager/core/browser/password_form_manager.h (right): https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_form_manager.h:179: // Returns the provisionally saved form, if it exists otherwise nullptr. On 2016/12/27 16:28:22, vabr (Chromium) wrote: > very small nit: Missing comma after "exists". Done. https://codereview.chromium.org/2604783002/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:591: // CredentialsFilter::ReportFormLoginSuccess CredentialsFilter::ShouldSaved On 2016/12/27 16:28:22, vabr (Chromium) wrote: > nit: Did you man to put "and" between CredentialsFilter::ReportFormLoginSuccess > and CredentialsFilter::ShouldSave? > > Also: ShouldSaved -> ShouldSave Done. https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:592: // should be called. The argument of ShouldSaved shold be submitted form. On 2016/12/27 16:28:22, vabr (Chromium) wrote: > nit: submitted form -> the submitted form Done. https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:594: PasswordForm form(MakeSimpleForm()); On 2016/12/27 16:28:22, vabr (Chromium) wrote: > nit: Please rename to |stored_form|, because this form pretends to be returned > from GetLogins. Thanks, it makes sense https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:598: // Let's make |observed_form| not to be equal to the from saved in the store. On 2016/12/27 16:28:22, vabr (Chromium) wrote: > Simple phrasing of this comment would be: > // Ensure that |observed_form| is different from |stored_form|. > > But it is not clear what is inside |stored_form|, so perhaps instead of the > comment, why not do: > EXPECT_NE(observed_form.username_value, stored_form.username_value); Done. https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:598: // Let's make |observed_form| not to be equal to the from saved in the store. On 2016/12/27 16:28:22, vabr (Chromium) wrote: > Also, it might be beneficial to explain _why_ we want those forms to be > different: to ensure that it is the observed_form and not the stored_form what > is passed to ShouldSave. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still LGTM, just one more nit. Thanks! Vaclav https://codereview.chromium.org/2604783002/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:598: // Let's make |observed_form| not to be equal to the from saved in the store. On 2016/12/27 17:17:10, dvadym wrote: > On 2016/12/27 16:28:22, vabr (Chromium) wrote: > > Simple phrasing of this comment would be: > > // Ensure that |observed_form| is different from |stored_form|. > > > > But it is not clear what is inside |stored_form|, so perhaps instead of the > > comment, why not do: > > EXPECT_NE(observed_form.username_value, stored_form.username_value); > > Done. If someone changes MakeSimpleForm to provide "username" as username_element, this test will silently stop checking what it should be checking. That's why I still think an EXPECT_NE(observed_form.username_value, stored_form.username_value); would be valuable. With that, you can reduce the current comment to just say something like: // Different values needed to ensure that it is the observed_form and not the stored_form what is passed to ShouldSave.
https://codereview.chromium.org/2604783002/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:598: // Let's make |observed_form| not to be equal to the from saved in the store. On 2016/12/27 18:23:56, vabr (Chromium) wrote: > On 2016/12/27 17:17:10, dvadym wrote: > > On 2016/12/27 16:28:22, vabr (Chromium) wrote: > > > Simple phrasing of this comment would be: > > > // Ensure that |observed_form| is different from |stored_form|. > > > > > > But it is not clear what is inside |stored_form|, so perhaps instead of the > > > comment, why not do: > > > EXPECT_NE(observed_form.username_value, stored_form.username_value); > > > > Done. > > If someone changes MakeSimpleForm to provide "username" as username_element, > this test will silently stop checking what it should be checking. > > That's why I still think an > > EXPECT_NE(observed_form.username_value, stored_form.username_value); > > would be valuable. With that, you can reduce the current comment to just say > something like: > // Different values needed to ensure that it is the observed_form and not the > stored_form what is passed to ShouldSave. I've update comment. Also I've changed observed_form.username_element = ASCIIToUTF16("username"); to observed_form.username_element += ASCIIToUTF16("1"); That's much more fool proof :) that values will be different.
The CQ bit was checked by dvadym@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/2604783002/#ps80001 (title: "tiny fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, an unconditional LGTM! :) https://codereview.chromium.org/2604783002/diff/40001/components/password_man... File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2604783002/diff/40001/components/password_man... components/password_manager/core/browser/password_manager_unittest.cc:598: // Let's make |observed_form| not to be equal to the from saved in the store. On 2016/12/28 10:26:09, dvadym wrote: > On 2016/12/27 18:23:56, vabr (Chromium) wrote: > > On 2016/12/27 17:17:10, dvadym wrote: > > > On 2016/12/27 16:28:22, vabr (Chromium) wrote: > > > > Simple phrasing of this comment would be: > > > > // Ensure that |observed_form| is different from |stored_form|. > > > > > > > > But it is not clear what is inside |stored_form|, so perhaps instead of > the > > > > comment, why not do: > > > > EXPECT_NE(observed_form.username_value, stored_form.username_value); > > > > > > Done. > > > > If someone changes MakeSimpleForm to provide "username" as username_element, > > this test will silently stop checking what it should be checking. > > > > That's why I still think an > > > > EXPECT_NE(observed_form.username_value, stored_form.username_value); > > > > would be valuable. With that, you can reduce the current comment to just say > > something like: > > // Different values needed to ensure that it is the observed_form and not the > > stored_form what is passed to ShouldSave. > > I've update comment. Also I've changed > observed_form.username_element = ASCIIToUTF16("username"); > to > observed_form.username_element += ASCIIToUTF16("1"); > That's much more fool proof :) that values will be different. Acknowledged, this works for me too.
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482920788942330,
"parent_rev": "d7bbedcbc1f44c1430e2064a6afa0bcb6ecc9f71", "commit_rev":
"e2104de7aa63008f7af04a153d071a1ec5cbf875"}
Message was sent while issue was closed.
Description was changed from ========== Do not show the update bubble for sync credentials when a username is unknown. This CL introduced a checking of |provisionally_saved_credentials| for sync credentials, instead of |pending_credentials|. That's more correct, since |provisionally_saved_credentials| is the submitted form, on other hand |pending_credentials| is a combination of already saved and submitted form. In case of saving both these forms are the same, but for updating they are different. BUG=663896 ========== to ========== Do not show the update bubble for sync credentials when a username is unknown. This CL introduced a checking of |provisionally_saved_credentials| for sync credentials, instead of |pending_credentials|. That's more correct, since |provisionally_saved_credentials| is the submitted form, on other hand |pending_credentials| is a combination of already saved and submitted form. In case of saving both these forms are the same, but for updating they are different. BUG=663896 Review-Url: https://codereview.chromium.org/2604783002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Do not show the update bubble for sync credentials when a username is unknown. This CL introduced a checking of |provisionally_saved_credentials| for sync credentials, instead of |pending_credentials|. That's more correct, since |provisionally_saved_credentials| is the submitted form, on other hand |pending_credentials| is a combination of already saved and submitted form. In case of saving both these forms are the same, but for updating they are different. BUG=663896 Review-Url: https://codereview.chromium.org/2604783002 ========== to ========== Do not show the update bubble for sync credentials when a username is unknown. This CL introduced a checking of |provisionally_saved_credentials| for sync credentials, instead of |pending_credentials|. That's more correct, since |provisionally_saved_credentials| is the submitted form, on other hand |pending_credentials| is a combination of already saved and submitted form. In case of saving both these forms are the same, but for updating they are different. BUG=663896 Committed: https://crrev.com/68324d6be5b713650770ac5ff8a732eda4146916 Cr-Commit-Position: refs/heads/master@{#440844} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/68324d6be5b713650770ac5ff8a732eda4146916 Cr-Commit-Position: refs/heads/master@{#440844} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
