Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22975006:
Save password functionality added to the save password bubble (Closed)
|
Created:
7 years, 3 months ago by npentrel Modified:
7 years, 3 months ago CC:
chromium-reviews, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSave password functionality added to the save password bubble behind flag. The buttons now let the user save and blacklist passwords.
BUG=261628
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221424
Patch Set 1 #Patch Set 2 : Minor changes #Patch Set 3 : minor changes #
Total comments: 12
Patch Set 4 : Review 1 #Patch Set 5 : minor change #
Total comments: 2
Patch Set 6 : Review 2 #Patch Set 7 : Review 3 #Patch Set 8 : minor change #Patch Set 9 : minor change #Patch Set 10 : Beautifying the bubble #
Total comments: 16
Patch Set 11 : Review 4 #Patch Set 12 : Review 5 #Patch Set 13 : Fix for regression bug #Messages
Total messages: 27 (0 generated)
Hi all, Please review the following files in this CL: @Garrett: chrome/browser/password_manager/password_form_manager.* @Scott: chrome/browser/ui/views/content_setting_bubble_contents.* Thanks, Naomi
https://codereview.chromium.org/22975006/diff/5001/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/22975006/diff/5001/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.h:99: virtual bool Accept(); Does this and Cancel need to be virtual?
https://codereview.chromium.org/22975006/diff/5001/chrome/browser/content_set... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/22975006/diff/5001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.cc:102: bool TabSpecificContentSettings::Accept() { Seems like these names should be a little more descriptive. Maybe PasswordAccepted() and PasswordFormBlacklisted()? https://codereview.chromium.org/22975006/diff/5001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.cc:110: form_to_save_->DeleteSavedPassword(); There should be no need to call this before blacklisting. Since we are prompting the user that means this form has not yet been saved, so deleting it isn't going to do anything. Even if it was, PermanentlyBlacklist() deletes all saved passwords for the form anyway. https://codereview.chromium.org/22975006/diff/5001/chrome/browser/content_set... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/22975006/diff/5001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:300: virtual bool Accept(); Is there a reason these are virtual? https://codereview.chromium.org/22975006/diff/5001/chrome/browser/password_ma... File chrome/browser/password_manager/password_form_manager.h (right): https://codereview.chromium.org/22975006/diff/5001/chrome/browser/password_ma... chrome/browser/password_manager/password_form_manager.h:74: // Save password according to current password_form. If the user has chosen I'm not sure that I understand the need for these changes. Shouldn't this use the same code the InfoBar is currently using (Save() to save and PermanentlyDelete() to blacklist)? Is there a reason why the functionality should be different?
https://codereview.chromium.org/22975006/diff/5001/chrome/browser/content_set... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/22975006/diff/5001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.cc:102: bool TabSpecificContentSettings::Accept() { On 2013/08/27 00:52:24, Garrett Casto wrote: > Seems like these names should be a little more descriptive. Maybe > PasswordAccepted() and PasswordFormBlacklisted()? Done. https://codereview.chromium.org/22975006/diff/5001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.cc:110: form_to_save_->DeleteSavedPassword(); On 2013/08/27 00:52:24, Garrett Casto wrote: > There should be no need to call this before blacklisting. Since we are prompting > the user that means this form has not yet been saved, so deleting it isn't going > to do anything. Even if it was, PermanentlyBlacklist() deletes all saved > passwords for the form anyway. Done. https://codereview.chromium.org/22975006/diff/5001/chrome/browser/content_set... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/22975006/diff/5001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:300: virtual bool Accept(); On 2013/08/27 00:52:24, Garrett Casto wrote: > Is there a reason these are virtual? Done. https://codereview.chromium.org/22975006/diff/5001/chrome/browser/password_ma... File chrome/browser/password_manager/password_form_manager.h (right): https://codereview.chromium.org/22975006/diff/5001/chrome/browser/password_ma... chrome/browser/password_manager/password_form_manager.h:74: // Save password according to current password_form. If the user has chosen Yes, the functionality needs to be slightly different as we now allow for the user to click on the icon again and change the choice the user previously made. Hence the user can save a password, then click on the icon again and blacklist it. But you are right, there is no need for DeleteSavedPassword. On 2013/08/27 00:52:24, Garrett Casto wrote: > I'm not sure that I understand the need for these changes. Shouldn't this use > the same code the InfoBar is currently using (Save() to save and > PermanentlyDelete() to blacklist)? Is there a reason why the functionality > should be different? https://codereview.chromium.org/22975006/diff/5001/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/22975006/diff/5001/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.h:99: virtual bool Accept(); Actually it doesn't need to be there at all. Thanks! On 2013/08/26 20:48:27, sky wrote: > Does this and Cancel need to be virtual?
LGTM
https://codereview.chromium.org/22975006/diff/5001/chrome/browser/password_ma... File chrome/browser/password_manager/password_form_manager.h (right): https://codereview.chromium.org/22975006/diff/5001/chrome/browser/password_ma... chrome/browser/password_manager/password_form_manager.h:74: // Save password according to current password_form. If the user has chosen On 2013/08/27 16:35:11, npentrel wrote: > Yes, the functionality needs to be slightly different as we now allow for the > user to click on the icon again and change the choice the user previously made. > Hence the user can save a password, then click on the icon again and blacklist > it. > In that case, shouldn't we just wait until navigation has occurred (and the icon has disappeared) before committing their decision to the database? Actually trying to save both if the user clicks back and forth seems more complicated than it has to be. For instance, if the user clicked blacklist and then save you would need to remove the blacklist credential during saving, which we currently don't do. > But you are right, there is no need for DeleteSavedPassword. > > On 2013/08/27 00:52:24, Garrett Casto wrote: > > I'm not sure that I understand the need for these changes. Shouldn't this use > > the same code the InfoBar is currently using (Save() to save and > > PermanentlyDelete() to blacklist)? Is there a reason why the functionality > > should be different? > https://codereview.chromium.org/22975006/diff/30001/chrome/browser/password_m... File chrome/browser/password_manager/password_form_manager.cc (right): https://codereview.chromium.org/22975006/diff/30001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.cc:115: } Note, there should be no reason to do this, the password store is checked in both SaveAsNewLogin() and UpdateLogin() when it is actually used.
https://codereview.chromium.org/22975006/diff/5001/chrome/browser/password_ma... File chrome/browser/password_manager/password_form_manager.h (right): https://codereview.chromium.org/22975006/diff/5001/chrome/browser/password_ma... chrome/browser/password_manager/password_form_manager.h:74: // Save password according to current password_form. If the user has chosen We have the following two cases: 1. User saves password -> user blacklists password In this case we don't have to change anything. 2. User blacklists password -> user saves password In this case all we need to do is (re)set pending_credentials_.blacklisted_by_user to false. This will take the saved password off the blacklisted password list. Hence, I believe that this change is easier and smaller than implementing a way to listen for the icon to disappear on the next navigation. If we did this, I am also not sure whether closing the tab would be understood as a navigation in order for the password to be saved in the case that someone logs in, saves the password, and closes the tab before the icon disappears. We should also consider that the icon is displayed on some pages until the user logs out, in the case that the browser crashed during that time period, the password would also not be saved. We can circumvent this by saving it/blacklisting it on click. On 2013/08/28 00:08:33, Garrett Casto wrote: > On 2013/08/27 16:35:11, npentrel wrote: > > Yes, the functionality needs to be slightly different as we now allow for the > > user to click on the icon again and change the choice the user previously > made. > > Hence the user can save a password, then click on the icon again and blacklist > > it. > > > > In that case, shouldn't we just wait until navigation has occurred (and the icon > has disappeared) before committing their decision to the database? Actually > trying to save both if the user clicks back and forth seems more complicated > than it has to be. For instance, if the user clicked blacklist and then save you > would need to remove the blacklist credential during saving, which we currently > don't do. > > > > But you are right, there is no need for DeleteSavedPassword. > > > > On 2013/08/27 00:52:24, Garrett Casto wrote: > > > I'm not sure that I understand the need for these changes. Shouldn't this > use > > > the same code the InfoBar is currently using (Save() to save and > > > PermanentlyDelete() to blacklist)? Is there a reason why the functionality > > > should be different? > > > https://codereview.chromium.org/22975006/diff/30001/chrome/browser/password_m... File chrome/browser/password_manager/password_form_manager.cc (right): https://codereview.chromium.org/22975006/diff/30001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.cc:115: } On 2013/08/28 00:08:33, Garrett Casto wrote: > Note, there should be no reason to do this, the password store is checked in > both SaveAsNewLogin() and UpdateLogin() when it is actually used. Done.
On 2013/08/28 08:48:43, npentrel wrote: > https://codereview.chromium.org/22975006/diff/5001/chrome/browser/password_ma... > File chrome/browser/password_manager/password_form_manager.h (right): > > https://codereview.chromium.org/22975006/diff/5001/chrome/browser/password_ma... > chrome/browser/password_manager/password_form_manager.h:74: // Save password > according to current password_form. If the user has chosen > We have the following two cases: > > 1. User saves password -> user blacklists password > In this case we don't have to change anything. > > 2. User blacklists password -> user saves password > In this case all we need to do is (re)set > pending_credentials_.blacklisted_by_user to false. This will take the saved > password off the blacklisted password list. > I was under the impression that the reason that PasswordStore had both AddLogin() and UpdateLogin() was that the first wouldn't necessarily touch passwords that are already in the database. However, in practice it seems like they will override anything that has the same values for our "unique" elements, which happens here. I'm not sure if this is intentional or not, but you don't need to remove this extra credential as I thought, as it just gets overridden. I'm still not sure that this is a good idea though. I've noticed two bugs so far 1) If the user blacklists and then saves, we have already deleted all of best_matches_, so we need to add those back. 2) If the user saves then blacklists, we do have the problem of the password persisting but only on Mac. There we save the password in the Keychain, but skip this step if the form has blacklisted_by_user set. So we would need to have something like DeleteSavedPassword or change the logic in PasswordStoreMac. Going forward I'm assuming that we actually have 3 states (do nothing, save, blacklist) that we want to expose to the user and let them toggle between. It seems like testing all these options and making sure we get them right no matter what order the user does them in is reasonably complicated, and we would need a good number of test cases for something like this. > Hence, I believe that this change is easier and smaller than implementing a way > to listen for the icon to disappear on the next navigation. If we did this, I am > also not sure whether closing the tab would be understood as a navigation in > order for the password to be saved in the case that someone logs in, saves the > password, and closes the tab before the icon disappears. > I would assume we would forward this information when the icon object is destroyed, either by navigation or the tab closing. I don't think that there are going to be no edge cases here, I just think that there will be less since you don't have to worry about 5 different implementations doing the right thing. I could be wrong though. Do you know what happen when bookmark a page? Does that get persisted immediately? > We should also consider that the icon is displayed on some pages until the user > logs out, in the case that the browser crashed during that time period, the > password would also not be saved. We can circumvent this by saving > it/blacklisting it on click. > Personally I'm not sure how much this matters. Crashes should be rare and even if we start the saving process when the user clicks, we don't give a guarantee on when this will happen since the work is done on a background thread. > > On 2013/08/28 00:08:33, Garrett Casto wrote: > > On 2013/08/27 16:35:11, npentrel wrote: > > > Yes, the functionality needs to be slightly different as we now allow for > the > > > user to click on the icon again and change the choice the user previously > > made. > > > Hence the user can save a password, then click on the icon again and > blacklist > > > it. > > > > > > > In that case, shouldn't we just wait until navigation has occurred (and the > icon > > has disappeared) before committing their decision to the database? Actually > > trying to save both if the user clicks back and forth seems more complicated > > than it has to be. For instance, if the user clicked blacklist and then save > you > > would need to remove the blacklist credential during saving, which we > currently > > don't do. > > > > > > > But you are right, there is no need for DeleteSavedPassword. > > > > > > On 2013/08/27 00:52:24, Garrett Casto wrote: > > > > I'm not sure that I understand the need for these changes. Shouldn't this > > use > > > > the same code the InfoBar is currently using (Save() to save and > > > > PermanentlyDelete() to blacklist)? Is there a reason why the functionality > > > > should be different? > > > > > > > https://codereview.chromium.org/22975006/diff/30001/chrome/browser/password_m... > File chrome/browser/password_manager/password_form_manager.cc (right): > > https://codereview.chromium.org/22975006/diff/30001/chrome/browser/password_m... > chrome/browser/password_manager/password_form_manager.cc:115: } > On 2013/08/28 00:08:33, Garrett Casto wrote: > > Note, there should be no reason to do this, the password store is checked in > > both SaveAsNewLogin() and UpdateLogin() when it is actually used. > > Done.
Rietveld is having some issues so I can't comment inline, but if you do keep the current code setup you should just update Save() to handle this particular case, not write a different SavePassword() function. There should be only one way to Save(). On Wed, Aug 28, 2013 at 3:43 PM, <gcasto@chromium.org> wrote: > On 2013/08/28 08:48:43, npentrel wrote: > > https://codereview.chromium.**org/22975006/diff/5001/chrome/** > browser/password_manager/**password_form_manager.h<https://codereview.chromium.org/22975006/diff/5001/chrome/browser/password_manager/password_form_manager.h> > >> File chrome/browser/password_**manager/password_form_manager.**h (right): >> > > > https://codereview.chromium.**org/22975006/diff/5001/chrome/** > browser/password_manager/**password_form_manager.h#**newcode74<https://codereview.chromium.org/22975006/diff/5001/chrome/browser/password_manager/password_form_manager.h#newcode74> > >> chrome/browser/password_**manager/password_form_manager.**h:74: // Save >> password >> according to current password_form. If the user has chosen >> We have the following two cases: >> > > 1. User saves password -> user blacklists password >> In this case we don't have to change anything. >> > > 2. User blacklists password -> user saves password >> In this case all we need to do is (re)set >> pending_credentials_.**blacklisted_by_user to false. This will take the >> saved >> password off the blacklisted password list. >> > > > I was under the impression that the reason that PasswordStore had both > AddLogin() and UpdateLogin() was that the first wouldn't necessarily touch > passwords that are already in the database. However, in practice it seems > like > they will override anything that has the same values for our "unique" > elements, > which happens here. I'm not sure if this is intentional or not, but you > don't > need to remove this extra credential as I thought, as it just gets > overridden. > I'm still not sure that this is a good idea though. I've noticed two bugs > so far > > 1) If the user blacklists and then saves, we have already deleted all of > best_matches_, so we need to add those back. > 2) If the user saves then blacklists, we do have the problem of the > password > persisting but only on Mac. There we save the password in the Keychain, > but skip > this step if the form has blacklisted_by_user set. So we would need to have > something like DeleteSavedPassword or change the logic in PasswordStoreMac. > > Going forward I'm assuming that we actually have 3 states (do nothing, > save, > blacklist) that we want to expose to the user and let them toggle between. > It > seems like testing all these options and making sure we get them right no > matter > what order the user does them in is reasonably complicated, and we would > need a > good number of test cases for something like this. > > > Hence, I believe that this change is easier and smaller than implementing >> a >> > way > >> to listen for the icon to disappear on the next navigation. If we did >> this, I >> > am > >> also not sure whether closing the tab would be understood as a navigation >> in >> order for the password to be saved in the case that someone logs in, >> saves the >> password, and closes the tab before the icon disappears. >> > > > I would assume we would forward this information when the icon object is > destroyed, either by navigation or the tab closing. I don't think that > there are > going to be no edge cases here, I just think that there will be less since > you > don't have to worry about 5 different implementations doing the right > thing. I > could be wrong though. Do you know what happen when bookmark a page? Does > that > get persisted immediately? > > > We should also consider that the icon is displayed on some pages until the >> > user > >> logs out, in the case that the browser crashed during that time period, >> the >> password would also not be saved. We can circumvent this by saving >> it/blacklisting it on click. >> > > > Personally I'm not sure how much this matters. Crashes should be rare and > even > if we start the saving process when the user clicks, we don't give a > guarantee > on when this will happen since the work is done on a background thread. > > > > On 2013/08/28 00:08:33, Garrett Casto wrote: >> > On 2013/08/27 16:35:11, npentrel wrote: >> > > Yes, the functionality needs to be slightly different as we now allow >> for >> the >> > > user to click on the icon again and change the choice the user >> previously >> > made. >> > > Hence the user can save a password, then click on the icon again and >> blacklist >> > > it. >> > > >> > >> > In that case, shouldn't we just wait until navigation has occurred (and >> the >> icon >> > has disappeared) before committing their decision to the database? >> Actually >> > trying to save both if the user clicks back and forth seems more >> complicated >> > than it has to be. For instance, if the user clicked blacklist and then >> save >> you >> > would need to remove the blacklist credential during saving, which we >> currently >> > don't do. >> > >> > >> > > But you are right, there is no need for DeleteSavedPassword. >> > > >> > > On 2013/08/27 00:52:24, Garrett Casto wrote: >> > > > I'm not sure that I understand the need for these changes. Shouldn't >> > this > >> > use >> > > > the same code the InfoBar is currently using (Save() to save and >> > > > PermanentlyDelete() to blacklist)? Is there a reason why the >> > functionality > >> > > > should be different? >> > > >> > >> > > > https://codereview.chromium.**org/22975006/diff/30001/** > chrome/browser/password_**manager/password_form_manager.**cc<https://codereview.chromium.org/22975006/diff/30001/chrome/browser/password_manager/password_form_manager.cc> > >> File chrome/browser/password_**manager/password_form_manager.**cc >> (right): >> > > > https://codereview.chromium.**org/22975006/diff/30001/** > chrome/browser/password_**manager/password_form_manager.**cc#newcode115<https://codereview.chromium.org/22975006/diff/30001/chrome/browser/password_manager/password_form_manager.cc#newcode115> > >> chrome/browser/password_**manager/password_form_manager.**cc:115: } >> On 2013/08/28 00:08:33, Garrett Casto wrote: >> > Note, there should be no reason to do this, the password store is >> checked in >> > both SaveAsNewLogin() and UpdateLogin() when it is actually used. >> > > Done. >> > > > > https://codereview.chromium.**org/22975006/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I can change the SavePassword() to simply use Save() that is no problem. > 1) If the user blacklists and then saves, we have already deleted all of > best_matches_, so we need to add those back. > 2) If the user saves then blacklists, we do have the problem of the > password > persisting but only on Mac. There we save the password in the Keychain, > but skip > this step if the form has blacklisted_by_user set. So we would need to have > something like DeleteSavedPassword or change the logic in PasswordStoreMac. To comment on the two bugs: 1. We could store the best_matches_ locally which would enable us to easily add them back. 2. Would the DeleteSavedPassword() that I had initially (which removes the login) solve this? As for the testing: if we do nothing, nothing changes, therefore that is easy. Hence we only have the two cases where we change from blacklist to save and from save to blacklist. I checked what happens with bookmarks. They only apply the removal of a password once the window is closed and the bookmark_bubble_view is destroyed. I cannot apply changes once the password form manager is destroyed because that only happens when the tab is closed and the bubble model is destroyed once the bubble closes. Hence I am not sure how to trigger the saving or the blacklisting could be triggered on navigation. Do you have an idea on this?
On 2013/08/30 15:00:56, npentrel wrote: > I can change the SavePassword() to simply use Save() that is no problem. > > > 1) If the user blacklists and then saves, we have already deleted all of > > best_matches_, so we need to add those back. > > 2) If the user saves then blacklists, we do have the problem of the > > password > > persisting but only on Mac. There we save the password in the Keychain, > > but skip > > this step if the form has blacklisted_by_user set. So we would need to have > > something like DeleteSavedPassword or change the logic in PasswordStoreMac. > > To comment on the two bugs: > 1. We could store the best_matches_ locally which would enable us to easily add > them back. Right, you could loop through best_matches_ and re-add them all. The question is more how complicated is it to get right (do you always want to re-add, or only if they have already been deleted for instance). > 2. Would the DeleteSavedPassword() that I had initially (which removes the > login) solve this? > Yes, I think it should. > As for the testing: if we do nothing, nothing changes, therefore that is easy. > Hence we only have the two cases where we change from blacklist to save and from > save to blacklist. > So my assumption is that if we are going to give users the ability to switch between these two states (saved and blacklisted) before navigation occurs, then really we should allow them to switch between three states (saved, blacklisted, do nothing) before navigation. It seems strange to me that we would allow a user that has accidentally saved their password to blacklist the form, but not just unsave the password. Does this sound reasonable? If that is the case, then you have to worry about someone selecting save, then do nothing (not sure how the UI for that would work), then blacklist, and having it work the same as just blacklisting. We would probably need to add state that says what the user has previously selected and a way to undue that action. As I was saying before, I just think that such a change is reasonably large and would require adding a fair number of tests. > I checked what happens with bookmarks. They only apply the removal of a password > once the window is closed and the bookmark_bubble_view is destroyed. I cannot > apply changes once the password form manager is destroyed because that only > happens when the tab is closed and the bubble model is destroyed once the bubble > closes. Hence I am not sure how to trigger the saving or the blacklisting could > be triggered on navigation. Do you have an idea on this? The equivalent of BookmarkBubbleView is ContentSettingBubbleContent, which still would mean that the change would be committed when the bubble was closed. My thought was that it would be committed when ContentSettingSavePasswordImageModel is destroyed. Markus can probably comment on how reasonable a suggestion this is.
I have implemented a way that the changes are applied as soon as navigation occurs which is when the icon also disappears. > So my assumption is that if we are going to give users the ability to switch > between these two states (saved and blacklisted) before navigation occurs, then > really we should allow them to switch between three states (saved, blacklisted, > do nothing) before navigation. It seems strange to me that we would allow a user > that has accidentally saved their password to blacklist the form, but not just > unsave the password. Does this sound reasonable? Markus and I have discussed this. I believe that it is not likely that a user clicks on save by accident and then changes his/her mind and wants to go back to having done nothing. This functionality is rather for users that clicked on the wrong thing by accident. If the user wants to delete his/her password solely there is still the link to 'manage saved passwords' that allows for this. Once you have taken time to actually click either 'save password' or 'never for this site' it should not be the case that you want to simply go back to not having made a decision.
On 2013/09/01 04:06:34, Garrett Casto wrote: > On 2013/08/30 15:00:56, npentrel wrote: > > I can change the SavePassword() to simply use Save() that is no problem. > > > > > 1) If the user blacklists and then saves, we have already deleted all of > > > best_matches_, so we need to add those back. > > > 2) If the user saves then blacklists, we do have the problem of the > > > password > > > persisting but only on Mac. There we save the password in the Keychain, > > > but skip > > > this step if the form has blacklisted_by_user set. So we would need to have > > > something like DeleteSavedPassword or change the logic in PasswordStoreMac. > > > > To comment on the two bugs: > > 1. We could store the best_matches_ locally which would enable us to easily > add > > them back. > > Right, you could loop through best_matches_ and re-add them all. The question is > more how complicated is it to get right (do you always want to re-add, or only > if they have already been deleted for instance). > > > 2. Would the DeleteSavedPassword() that I had initially (which removes the > > login) solve this? > > > > Yes, I think it should. > > > As for the testing: if we do nothing, nothing changes, therefore that is easy. > > Hence we only have the two cases where we change from blacklist to save and > from > > save to blacklist. > > > > So my assumption is that if we are going to give users the ability to switch > between these two states (saved and blacklisted) before navigation occurs, then > really we should allow them to switch between three states (saved, blacklisted, > do nothing) before navigation. It seems strange to me that we would allow a user > that has accidentally saved their password to blacklist the form, but not just > unsave the password. Does this sound reasonable? > > If that is the case, then you have to worry about someone selecting save, then > do nothing (not sure how the UI for that would work), then blacklist, and having > it work the same as just blacklisting. We would probably need to add state that > says what the user has previously selected and a way to undue that action. As I > was saying before, I just think that such a change is reasonably large and would > require adding a fair number of tests. > > > I checked what happens with bookmarks. They only apply the removal of a > password > > once the window is closed and the bookmark_bubble_view is destroyed. I cannot > > apply changes once the password form manager is destroyed because that only > > happens when the tab is closed and the bubble model is destroyed once the > bubble > > closes. Hence I am not sure how to trigger the saving or the blacklisting > could > > be triggered on navigation. Do you have an idea on this? > > The equivalent of BookmarkBubbleView is ContentSettingBubbleContent, which still > would mean that the change would be committed when the bubble was closed. My > thought was that it would be committed when ContentSettingSavePasswordImageModel > is destroyed. Markus can probably comment on how reasonable a suggestion this > is. The model is recreated every time the bubble is opened (ContentSettingImageView::OnClick ).
On 2013/09/02 10:06:31, npentrel wrote: > I have implemented a way that the changes are applied as soon as navigation > occurs which is when the icon also disappears. > > > So my assumption is that if we are going to give users the ability to switch > > between these two states (saved and blacklisted) before navigation occurs, > then > > really we should allow them to switch between three states (saved, > blacklisted, > > do nothing) before navigation. It seems strange to me that we would allow a > user > > that has accidentally saved their password to blacklist the form, but not just > > unsave the password. Does this sound reasonable? > > Markus and I have discussed this. I believe that it is not likely that a user > clicks on save by accident and then changes his/her mind and wants to go back to > having done nothing. This functionality is rather for users that clicked on the > wrong thing by accident. If the user wants to delete his/her password solely > there is still the link to 'manage saved passwords' that allows for this. Once > you have taken time to actually click either 'save password' or 'never for this > site' it should not be the case that you want to simply go back to not having > made a decision. I agree with Naomi here. I think when a user see the bubble the first time they should have 3 options: ignore (dismiss) , save password or bock password. Once the password was blocked or saved I don't think the undecided state should be supported. Switching from block to safe or the other way arround seem a much stronger use case to me.
On 2013/09/03 22:35:22, markusheintz_ wrote: > On 2013/09/01 04:06:34, Garrett Casto wrote: > > On 2013/08/30 15:00:56, npentrel wrote: > > > I can change the SavePassword() to simply use Save() that is no problem. > > > > > > > 1) If the user blacklists and then saves, we have already deleted all of > > > > best_matches_, so we need to add those back. > > > > 2) If the user saves then blacklists, we do have the problem of the > > > > password > > > > persisting but only on Mac. There we save the password in the Keychain, > > > > but skip > > > > this step if the form has blacklisted_by_user set. So we would need to > have > > > > something like DeleteSavedPassword or change the logic in > PasswordStoreMac. > > > > > > To comment on the two bugs: > > > 1. We could store the best_matches_ locally which would enable us to easily > > add > > > them back. > > > > Right, you could loop through best_matches_ and re-add them all. The question > is > > more how complicated is it to get right (do you always want to re-add, or only > > if they have already been deleted for instance). > > > > > 2. Would the DeleteSavedPassword() that I had initially (which removes the > > > login) solve this? > > > > > > > Yes, I think it should. > > > > > As for the testing: if we do nothing, nothing changes, therefore that is > easy. > > > Hence we only have the two cases where we change from blacklist to save and > > from > > > save to blacklist. > > > > > > > So my assumption is that if we are going to give users the ability to switch > > between these two states (saved and blacklisted) before navigation occurs, > then > > really we should allow them to switch between three states (saved, > blacklisted, > > do nothing) before navigation. It seems strange to me that we would allow a > user > > that has accidentally saved their password to blacklist the form, but not just > > unsave the password. Does this sound reasonable? > > > > If that is the case, then you have to worry about someone selecting save, then > > do nothing (not sure how the UI for that would work), then blacklist, and > having > > it work the same as just blacklisting. We would probably need to add state > that > > says what the user has previously selected and a way to undue that action. As > I > > was saying before, I just think that such a change is reasonably large and > would > > require adding a fair number of tests. > > > > > I checked what happens with bookmarks. They only apply the removal of a > > password > > > once the window is closed and the bookmark_bubble_view is destroyed. I > cannot > > > apply changes once the password form manager is destroyed because that only > > > happens when the tab is closed and the bubble model is destroyed once the > > bubble > > > closes. Hence I am not sure how to trigger the saving or the blacklisting > > could > > > be triggered on navigation. Do you have an idea on this? > > > > The equivalent of BookmarkBubbleView is ContentSettingBubbleContent, which > still > > would mean that the change would be committed when the bubble was closed. My > > thought was that it would be committed when > ContentSettingSavePasswordImageModel > > is destroyed. Markus can probably comment on how reasonable a suggestion this > > is. > > The model is recreated every time the bubble is opened > (ContentSettingImageView::OnClick ). That is the ContentSettingBubbleModel. I was talking about the ContentSettingImageModel subclass, which looks like it's tied to the lifetime of the ContentSettingImageView.
On 2013/09/03 22:39:27, markusheintz_ wrote: > On 2013/09/02 10:06:31, npentrel wrote: > > I have implemented a way that the changes are applied as soon as navigation > > occurs which is when the icon also disappears. > > > > > So my assumption is that if we are going to give users the ability to switch > > > between these two states (saved and blacklisted) before navigation occurs, > > then > > > really we should allow them to switch between three states (saved, > > blacklisted, > > > do nothing) before navigation. It seems strange to me that we would allow a > > user > > > that has accidentally saved their password to blacklist the form, but not > just > > > unsave the password. Does this sound reasonable? > > > > Markus and I have discussed this. I believe that it is not likely that a user > > clicks on save by accident and then changes his/her mind and wants to go back > to > > having done nothing. This functionality is rather for users that clicked on > the > > wrong thing by accident. If the user wants to delete his/her password solely > > there is still the link to 'manage saved passwords' that allows for this. Once > > you have taken time to actually click either 'save password' or 'never for > this > > site' it should not be the case that you want to simply go back to not having > > made a decision. > > I agree with Naomi here. I think when a user see the bubble the first time they > should have 3 options: > ignore (dismiss) , save password or bock password. Once the password was blocked > or saved I don't think the undecided state should be supported. Switching from > block to safe or the other way arround seem a much stronger use case to me. I see. So the idea is that if the user misclicks, it's likely because they meant to click on the other button, not because they were trying to do something else (say click on the page)? I guess that seems reasonable. It seems strange to me that if the user accidentally saves a password (say my corporate password on accounts.google.com) the obvious thing to do is to blacklist to prevent the saving, which has other side effects that you may not want (destroying all the rest of my saved GAIA credentials). This may be such a corner case that it doesn't matter.
I would like to add that you still have the option of clicking on the 'manage saved passwords' link in the bubble (right bottom corner), which allows you to delete single passwords. This should be a corner case as you say but it should also be relatively easy with this link in place.
Few minor comments. Almost there. https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... File chrome/browser/password_manager/password_form_manager.cc (right): https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.cc:115: void PasswordFormManager::ApplyChange() { Could you add a DCHECK(!should_blacklist_password_ || !should_save_password_)? https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.cc:118: else Make this "else if" all on one line. https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... File chrome/browser/password_manager/password_form_manager.h (right): https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:75: // saved upon the next navigation. I try to write comments in the header as a consumer of the function would read them. So something more like "Sets current password to be saved when ApplyEdits() is called. Will override a previous call to BlacklistPassword()". https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:79: // blacklisted upon the next navigation. Similar to above. https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:83: // |should_blacklist_password_| and |should_save_password_|. Something like "Persist changes from the latest call to either SavePassword() or BlacklistPassword()." https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:119: // Blacklist it so that from now on when it is seen we ignore it. Add a TODO here to make this private once we switch to the new UI. https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:134: // observed_form_ (e.g DoesManage(pending_credentials_) == true). Same here.
On 2013/09/03 23:17:28, npentrel wrote: > I would like to add that you still have the option of clicking on the 'manage > saved passwords' link in the bubble (right bottom corner), which allows you to > delete single passwords. This should be a corner case as you say but it should > also be relatively easy with this link in place. Sorry I ignore the multilogin scenario way to often. Which of the following scenarios do we want to support via the bubble: 1) User A and user B stored their password for www.example.com. Now user B wants to delete it's password but not user A. 2) User A want to save it's password on www.example.com but user B want's to block storing passwords on www.example.com. 3) User A has two identities A1 and A2. He decides that the password for A1 should be stored on www.example.com but not the password for A2. I don't think 2 and 3 make sense. But I can see 1 happening when someone logged in at a friends computer and accidentally stored it's password. In this case switching to blacklisting is bad. But recovering from this is easy. The question is what is more common: Multi login or single login per page. In the single login case switching between blacklisting and saving is what we want. Even in some multi login cases. On Alex mocks the "don't save" defaults to "Don't save my password" but not to a "Never save any passwords on this site" (blacklisting). I guess this already serves as a good protection against accidentally deleting other passwords on the same site.
https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... File chrome/browser/password_manager/password_form_manager.cc (right): https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.cc:115: void PasswordFormManager::ApplyChange() { On 2013/09/03 23:46:43, Garrett Casto wrote: > Could you add a DCHECK(!should_blacklist_password_ || !should_save_password_)? No. I call ApplyChange from TabSpecificContentSettings::DidNavigateMainFrame with no regard to whether anything changed or not. https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.cc:118: else On 2013/09/03 23:46:43, Garrett Casto wrote: > Make this "else if" all on one line. Done. https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... File chrome/browser/password_manager/password_form_manager.h (right): https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:75: // saved upon the next navigation. On 2013/09/03 23:46:43, Garrett Casto wrote: > I try to write comments in the header as a consumer of the function would read > them. So something more like "Sets current password to be saved when > ApplyEdits() is called. Will override a previous call to BlacklistPassword()". Done. https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:79: // blacklisted upon the next navigation. On 2013/09/03 23:46:43, Garrett Casto wrote: > Similar to above. Done. https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:83: // |should_blacklist_password_| and |should_save_password_|. On 2013/09/03 23:46:43, Garrett Casto wrote: > Something like "Persist changes from the latest call to either SavePassword() or > BlacklistPassword()." Done. https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:119: // Blacklist it so that from now on when it is seen we ignore it. On 2013/09/03 23:46:43, Garrett Casto wrote: > Add a TODO here to make this private once we switch to the new UI. Done. https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:134: // observed_form_ (e.g DoesManage(pending_credentials_) == true). On 2013/09/03 23:46:43, Garrett Casto wrote: > Same here. Done.
LGTM https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... File chrome/browser/password_manager/password_form_manager.cc (right): https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.cc:115: void PasswordFormManager::ApplyChange() { On 2013/09/04 08:32:23, npentrel wrote: > On 2013/09/03 23:46:43, Garrett Casto wrote: > > Could you add a DCHECK(!should_blacklist_password_ || !should_save_password_)? > > No. I call ApplyChange from TabSpecificContentSettings::DidNavigateMainFrame > with no regard to whether anything changed or not. I meant that you want to verify that should_blacklist_password_ and should_save_password_ are not both set, not that one of them has to be set. I believe that the incantation that I gave above should do that.
https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... File chrome/browser/password_manager/password_form_manager.cc (right): https://codereview.chromium.org/22975006/diff/67001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.cc:115: void PasswordFormManager::ApplyChange() { On 2013/09/04 21:06:28, Garrett Casto wrote: > On 2013/09/04 08:32:23, npentrel wrote: > > On 2013/09/03 23:46:43, Garrett Casto wrote: > > > Could you add a DCHECK(!should_blacklist_password_ || > !should_save_password_)? > > > > No. I call ApplyChange from TabSpecificContentSettings::DidNavigateMainFrame > > with no regard to whether anything changed or not. > > I meant that you want to verify that should_blacklist_password_ and > should_save_password_ are not both set, not that one of them has to be set. I > believe that the incantation that I gave above should do that. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/npentrel@chromium.org/22975006/80001
Message was sent while issue was closed.
Change committed as 221424
Message was sent while issue was closed.
FYI - I think this may have caused a regression with the media permission UI.
Investigating this now but here's the top of the crash stack:
[33928:22892:0906/143422:FATAL:grid_layout.cc(699)] Check failed:
GetColumnSet(id) == NULL.
Backtrace:
base::debug::StackTrace::StackTrace [0x10087B51+33]
(d:\src\src\base\debug\stack_trace_win.cc:207)
logging::LogMessage::~LogMessage [0x101081CE+94]
(d:\src\src\base\logging.cc:566)
views::GridLayout::AddColumnSet [0x19A3F38B+267]
(d:\src\src\ui\views\layout\grid_layout.cc:699)
ContentSettingBubbleContents::Init [0x1605FCE9+9273]
(d:\src\src\chrome\browser\ui\views\content_setting_bubble_contents.cc:431)
views::BubbleDelegateView::CreateBubble [0x198FB0AD+45]
(d:\src\src\ui\views\bubble\bubble_delegate.cc:166)
ToolbarView::CreateViewsBubble [0x147B5CCB+27]
(d:\src\src\chrome\browser\ui\views\toolbar_view.cc:376)
ContentSettingImageView::CreateBubble [0x1677B8F3+323]
(d:\src\src\chrome\browser\ui\views\location_bar\content_setting_image_view.cc:281)
ContentSettingImageView::OnClick [0x1677B756+230]
(d:\src\src\chrome\browser\ui\views\location_bar\content_setting_image_view.cc:272)
ContentSettingImageView::OnMouseReleased [0x1677B25B+171]
(d:\src\src\chrome\browser\ui\views\location_bar\content_setting_image_view.cc:227)
Message was sent while issue was closed.
Thanks, I fixed it. |
