Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23537029:
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, markusheintz_ Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionThis addresses comments on https://codereview.chromium.org/23980003/
Save 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=223608
Patch Set 1 #Patch Set 2 : Review 1 #
Total comments: 22
Patch Set 3 : Review 2 #
Total comments: 14
Patch Set 4 : Review 3 #
Total comments: 7
Patch Set 5 : Review 4 #Patch Set 6 : Review 5 #Messages
Total messages: 16 (0 generated)
Addressing comments from: https://codereview.chromium.org/23980003/
(Replied to comments on the old CL)
https://codereview.chromium.org/23980003/diff/1/chrome/browser/ui/browser_con... File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc (right): https://codereview.chromium.org/23980003/diff/1/chrome/browser/ui/browser_con... chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:34: if (type == CONTENT_SETTINGS_TYPE_MIXEDSCRIPT) { On 2013/09/10 00:22:11, Peter Kasting wrote: > On 2013/09/09 22:08:34, npentrel wrote: > > On 2013/09/09 18:19:02, Peter Kasting wrote: > > > Nit: Consider converting these conditionals to a switch statement. > > > > That would mean creating a lot of cases where nothing happens. I don't think > > that would be better as there are only three if branches right now. > > ? How so? > > switch (type) { > case CONTENT_SETTINGS_TYPE_MIXEDSCRIPT: > ...; > > case CONTENT_SETTINGS_TYPE_PROTOCOL_HANDLERS: > ...; > > case CONTENT_SETTINGS_TYPE_SAVE_PASSWORD: > ...; > > default: > chrome::ShowContentSettings(browser_, type); > return; > } > > (Each of the ... blocks, of course, contains the full content of the current > conditional body blocks.) Done. https://codereview.chromium.org/23980003/diff/1/chrome/browser/ui/views/conte... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/23980003/diff/1/chrome/browser/ui/views/conte... chrome/browser/ui/views/content_setting_bubble_contents.cc:396: const int kDoubleColumnSetId = 2; On 2013/09/10 00:22:11, Peter Kasting wrote: > On 2013/09/09 22:08:34, npentrel wrote: > > On 2013/09/09 18:19:02, Peter Kasting wrote: > > > Tiny nit: Why order these as (2, 1) instead of (1, 2)? Plus if you did the > > > latter you could pull kDoubleColumnSet and the first AddColumn() call out > > above > > > the conditional. > > > > Changed the order. However it is not possible to pull out those two calls > > because the ids used in initializing double_column_set differ. > > You misunderstood the intent of my comment; sorry if I was unclear. > > I did not mean "reverse the order of the conditional arms". I meant "swap the > column set IDs used inside this arm, since there's no obvious reason we add > column set number 2 before we add set 1". Once that is done, both arms will > begin with: > > const int kDoubleColumnSetId = 1; > views::ColumnSet* double_column_set = layout->AddColumnSet(kDoubleColumnSetId); > > ...and then this code can be hoisted. Done. Gotcha. https://codereview.chromium.org/23980003/diff/1/chrome/browser/ui/views/conte... chrome/browser/ui/views/content_setting_bubble_contents.cc:454: if (sender == save_button_) { On 2013/09/10 00:22:11, Peter Kasting wrote: > On 2013/09/09 22:08:34, npentrel wrote: > > On 2013/09/09 18:19:02, Peter Kasting wrote: > > > Nit: You can simplify this function by handling the radio buttons first and > > then > > > collapsing the duplicate StartFade() calls: > > > > > > /* Code to handle radio buttons here */ > > > > > > if (sender == save_button_) > > > content_setting_bubble_model_->OnSaveClicked(); > > > else if (sender == cancel_button_) > > > content_setting_bubble_model_->OnCancelClicked(); > > > else if (sender == close_button_) > > > content_setting_bubble_model_->OnDoneClicked(); > > > StartFade(false); > > > // No "return;" necessary > > > > That does not quite work because of the NOTREACHED(). I cannot put that call > > anywhere else and therefore I need to call return in each if branch. > > My intent was for you to discard the NOTREACHED. I don't think it's critical. > > However, you could preserve it if you wanted by e.g.: > > ... > else if (sender == close_button_) > content_setting_bubble_model_->OnDoneClicked(); > else > NOTREACHED(); > StartFade(false); > > ...or, identically, by adding {} to the conditional arms and converting the last > to: > > ... > } else { > DCHECK_EQ(close_button_, sender); > content_setting_bubble_model_->OnDoneClicked(); > } Done. https://codereview.chromium.org/23980003/diff/1/chrome/browser/ui/views/conte... File chrome/browser/ui/views/content_setting_bubble_contents.h (right): https://codereview.chromium.org/23980003/diff/1/chrome/browser/ui/views/conte... chrome/browser/ui/views/content_setting_bubble_contents.h:113: views::Link* custom_link_; On 2013/09/10 00:22:11, Peter Kasting wrote: > On 2013/09/09 22:08:34, npentrel wrote: > > On 2013/09/09 18:19:02, Peter Kasting wrote: > > > Tiny nit: It might be nice to order all these in visual order (L->R, T->B) > > > > They are not always all used. I think it would be more confusing. > > OK, let me rephrase: > > I would like these to be in some clear order. Right now they're basically > random. Most views use visual order, but if there is something that's clearly > better, feel free to use that instead :) Ok, I changed it to visual order. I thought it was confusing to have label buttons and then links and then another label button.
https://codereview.chromium.org/23537029/diff/5001/chrome/browser/content_set... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:301: bool PasswordAccepted(); These two functions should also be replaced by a single SetPasswordAction() routine. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... File chrome/browser/password_manager/password_form_manager.cc (left): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... chrome/browser/password_manager/password_form_manager.cc:563: // The most important element that should match is the origin, followed by Why did you change this comment? https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... File chrome/browser/password_manager/password_form_manager.cc (right): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... chrome/browser/password_manager/password_form_manager.cc:56: if (!DO_NOTHING) This isn't what you intended to write. Plus, this check isn't necessary. ApplyChange() checks internally, so you can call it unconditionally. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... File chrome/browser/password_manager/password_form_manager.h (right): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... chrome/browser/password_manager/password_form_manager.h:86: // either scenario ApplyChange() is called. This comment is very confusing (poor wording). Consider instead writing a comment on the |password_action_| member and/or on ApplyChange(). Comments in these places are likely to be less circuitous. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... chrome/browser/password_manager/password_form_manager.h:88: password_action_ = password_action; Simple inlined setters should be named set_password_action(). https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/browser_... File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc (right): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/browser_... chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:39: GURL(kInsecureScriptHelpUrl))); Nit: I suggest inlining this into the next statement in order to avoid the need to put braces around this case. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/browser_... chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:42: break; Nit: Use return instead of break where possible. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/content_... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.h:202: private: Nit: Blank line above this. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:397: if (content_setting_bubble_model_->content_type() != Tiny nit: I think the original ordering of "== SAVE_PASSWORD" was fractionally better. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:463: NOTREACHED() << "unknown radio"; Nit: Remove this comment, which is not necessarily accurate and not helpful anyway. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:464: Tiny nit: I'd probably remove this blank line
https://codereview.chromium.org/23537029/diff/5001/chrome/browser/content_set... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:301: bool PasswordAccepted(); On 2013/09/10 23:04:41, Peter Kasting wrote: > These two functions should also be replaced by a single SetPasswordAction() > routine. Done. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... File chrome/browser/password_manager/password_form_manager.cc (left): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... chrome/browser/password_manager/password_form_manager.cc:563: // The most important element that should match is the origin, followed by On 2013/09/10 23:04:41, Peter Kasting wrote: > Why did you change this comment? Done. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... File chrome/browser/password_manager/password_form_manager.cc (right): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... chrome/browser/password_manager/password_form_manager.cc:56: if (!DO_NOTHING) On 2013/09/10 23:04:41, Peter Kasting wrote: > This isn't what you intended to write. > > Plus, this check isn't necessary. ApplyChange() checks internally, so you can > call it unconditionally. Done. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... File chrome/browser/password_manager/password_form_manager.h (right): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... chrome/browser/password_manager/password_form_manager.h:86: // either scenario ApplyChange() is called. On 2013/09/10 23:04:41, Peter Kasting wrote: > This comment is very confusing (poor wording). > > Consider instead writing a comment on the |password_action_| member and/or on > ApplyChange(). Comments in these places are likely to be less circuitous. Done. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/password_ma... chrome/browser/password_manager/password_form_manager.h:88: password_action_ = password_action; On 2013/09/10 23:04:41, Peter Kasting wrote: > Simple inlined setters should be named set_password_action(). Done. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/browser_... File chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc (right): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/browser_... chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:39: GURL(kInsecureScriptHelpUrl))); On 2013/09/10 23:04:41, Peter Kasting wrote: > Nit: I suggest inlining this into the next statement in order to avoid the need > to put braces around this case. Done. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/browser_... chrome/browser/ui/browser_content_setting_bubble_model_delegate.cc:42: break; On 2013/09/10 23:04:41, Peter Kasting wrote: > Nit: Use return instead of break where possible. Done. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/content_... File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/content_... chrome/browser/ui/content_settings/content_setting_bubble_model.h:202: private: On 2013/09/10 23:04:41, Peter Kasting wrote: > Nit: Blank line above this. Done. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/views/co... File chrome/browser/ui/views/content_setting_bubble_contents.cc (right): https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:397: if (content_setting_bubble_model_->content_type() != On 2013/09/10 23:04:41, Peter Kasting wrote: > Tiny nit: I think the original ordering of "== SAVE_PASSWORD" was fractionally > better. Done. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:463: NOTREACHED() << "unknown radio"; On 2013/09/10 23:04:41, Peter Kasting wrote: > Nit: Remove this comment, which is not necessarily accurate and not helpful > anyway. Done. https://codereview.chromium.org/23537029/diff/5001/chrome/browser/ui/views/co... chrome/browser/ui/views/content_setting_bubble_contents.cc:464: On 2013/09/10 23:04:41, Peter Kasting wrote: > Tiny nit: I'd probably remove this blank line Done.
I'm sorry to be so picky about comments and the like, but a lot of this text just is not as clear as it seems like it could be. https://codereview.chromium.org/23537029/diff/19001/chrome/browser/content_se... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/23537029/diff/19001/chrome/browser/content_se... chrome/browser/content_settings/tab_specific_content_settings.cc:104: form_to_save_->set_password_action(password_action); Nit: Seems like you can just make this function an inlined unix_hacker_style() setter in the header, if this is the whole body. https://codereview.chromium.org/23537029/diff/19001/chrome/browser/content_se... chrome/browser/content_settings/tab_specific_content_settings.cc:105: return true; Why does this return a bool at all? Shouldn't this return void? https://codereview.chromium.org/23537029/diff/19001/chrome/browser/content_se... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/23537029/diff/19001/chrome/browser/content_se... chrome/browser/content_settings/tab_specific_content_settings.h:299: // |form_to_save| to perfom the chosen action when the next navigation occurs Nit: form_to_save_ ? Perhaps this could be improved even more by naming this member (and relevant setters, etc.) |form_manager_|, since that way the name will imply that (1) This is a management object, not a form itself. (2) The management object may do something other than "save" on navigating. https://codereview.chromium.org/23537029/diff/19001/chrome/browser/content_se... chrome/browser/content_settings/tab_specific_content_settings.h:344: // PasswordAccepted() and PasswordFormBlacklisted respectively. This comment is out of date; also see comments above. https://codereview.chromium.org/23537029/diff/19001/chrome/browser/password_m... File chrome/browser/password_manager/password_form_manager.h (right): https://codereview.chromium.org/23537029/diff/19001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:81: // navigation occurs or when the tab is closed. Nit: How about: Called when the page is navigated or the tab is closed. Takes the necessary action with the form's login/password based on the desired |password_action_|. https://codereview.chromium.org/23537029/diff/19001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:84: // Record state of |password_action_|. This comment is misleading. Just remove it entirely. https://codereview.chromium.org/23537029/diff/19001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:313: // credentials. The action gets applied with ApplyChange(). Nit: How about: Whether we should save, blacklist, or do nothing with this form's login/password on the next navigation or when the tab is closed.
No worries. I'm trying to learn. https://codereview.chromium.org/23537029/diff/19001/chrome/browser/content_se... File chrome/browser/content_settings/tab_specific_content_settings.cc (right): https://codereview.chromium.org/23537029/diff/19001/chrome/browser/content_se... chrome/browser/content_settings/tab_specific_content_settings.cc:104: form_to_save_->set_password_action(password_action); On 2013/09/11 21:49:39, Peter Kasting wrote: > Nit: Seems like you can just make this function an inlined unix_hacker_style() > setter in the header, if this is the whole body. Done. https://codereview.chromium.org/23537029/diff/19001/chrome/browser/content_se... chrome/browser/content_settings/tab_specific_content_settings.cc:105: return true; On 2013/09/11 21:49:39, Peter Kasting wrote: > Why does this return a bool at all? Shouldn't this return void? Done. https://codereview.chromium.org/23537029/diff/19001/chrome/browser/content_se... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/23537029/diff/19001/chrome/browser/content_se... chrome/browser/content_settings/tab_specific_content_settings.h:299: // |form_to_save| to perfom the chosen action when the next navigation occurs On 2013/09/11 21:49:39, Peter Kasting wrote: > Nit: form_to_save_ ? Perhaps this could be improved even more by naming this > member (and relevant setters, etc.) |form_manager_|, since that way the name > will imply that > (1) This is a management object, not a form itself. > (2) The management object may do something other than "save" on navigating. Done. https://codereview.chromium.org/23537029/diff/19001/chrome/browser/content_se... chrome/browser/content_settings/tab_specific_content_settings.h:344: // PasswordAccepted() and PasswordFormBlacklisted respectively. On 2013/09/11 21:49:39, Peter Kasting wrote: > This comment is out of date; also see comments above. Done. https://codereview.chromium.org/23537029/diff/19001/chrome/browser/password_m... File chrome/browser/password_manager/password_form_manager.h (right): https://codereview.chromium.org/23537029/diff/19001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:81: // navigation occurs or when the tab is closed. On 2013/09/11 21:49:39, Peter Kasting wrote: > Nit: How about: > > Called when the page is navigated or the tab is closed. Takes the necessary > action with the form's login/password based on the desired |password_action_|. Done. https://codereview.chromium.org/23537029/diff/19001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:84: // Record state of |password_action_|. On 2013/09/11 21:49:39, Peter Kasting wrote: > This comment is misleading. Just remove it entirely. Done. https://codereview.chromium.org/23537029/diff/19001/chrome/browser/password_m... chrome/browser/password_manager/password_form_manager.h:313: // credentials. The action gets applied with ApplyChange(). On 2013/09/11 21:49:39, Peter Kasting wrote: > Nit: How about: > > Whether we should save, blacklist, or do nothing with this form's login/password > on the next navigation or when the tab is closed. Done.
https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:300: // or when the tab is closed. So why is it that we do set_password_action() and then wait for a navigation/tab closure to apply the change, instead of combining those two steps into one and making user action in the infobar immediately "apply change" with the desired change? After all, the UI disappears once the user responds to it, right? https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:349: // PasswordFormManager::ApplyChange(). Spelling and clarity issues. How about: Called when the user submits a form containing login information, so we can handle later requests to save or blacklist that login information. This stores the provided object in form_manager_ and triggers the UI to prompt the user about whether they'd like to save the password. https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:434: // and should update as per the decision. This sentence is not grammatically correct. How about: Set by OnPasswordSubmitted() when the user submits a form containing login information. If the user responds to a subsequent "Do you want to save this password?" prompt, we ask this object to save or blacklist the associated login information in Chrome's password store.
https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:300: // or when the tab is closed. So why is it that we do set_password_action() and then wait for a navigation/tab closure to apply the change, instead of combining those two steps into one and making user action in the infobar immediately "apply change" with the desired change? After all, the UI disappears once the user responds to it, right? https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:349: // PasswordFormManager::ApplyChange(). Spelling and clarity issues. How about: Called when the user submits a form containing login information, so we can handle later requests to save or blacklist that login information. This stores the provided object in form_manager_ and triggers the UI to prompt the user about whether they'd like to save the password. https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:434: // and should update as per the decision. This sentence is not grammatically correct. How about: Set by OnPasswordSubmitted() when the user submits a form containing login information. If the user responds to a subsequent "Do you want to save this password?" prompt, we ask this object to save or blacklist the associated login information in Chrome's password store.
https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:300: // or when the tab is closed. On 2013/09/12 20:21:00, Peter Kasting wrote: > So why is it that we do set_password_action() and then wait for a navigation/tab > closure to apply the change, instead of combining those two steps into one and > making user action in the infobar immediately "apply change" with the desired > change? After all, the UI disappears once the user responds to it, right? First of all, you are right the UI disappears once the user responds to it but we now have the option of bringing the UI up again by clicking on the icon (which remains visible until the next navigation occurs). Hence the user can still change the choice. (There will be changes to the bubble that appears if the user decides to change the choice but for now it pops up the same bubble.) My first inclination was to apply the choice the user makes as soon as it is made but Garrett convinced me to change this because I would have needed to undo the previously chosen action. (i.e. if the user chooses save and then never save for this site, the saved password has to be deleted before the site is blacklisted - which is not a big problem but the other case where the user first chooses to blacklist and then to save is a bit intricate as the blacklisting action deletes all saved passwords for that site and those would need to also be re-added when the user decides to now save the password instead). We might still change this but that depends on some design decisions that need to be made. https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:349: // PasswordFormManager::ApplyChange(). On 2013/09/12 20:21:00, Peter Kasting wrote: > Spelling and clarity issues. How about: > > Called when the user submits a form containing login information, so we can > handle later requests to save or blacklist that login information. This stores > the provided object in form_manager_ and triggers the UI to prompt the user > about whether they'd like to save the password. Done. https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:434: // and should update as per the decision. On 2013/09/12 20:21:00, Peter Kasting wrote: > This sentence is not grammatically correct. How about: > > Set by OnPasswordSubmitted() when the user submits a form containing login > information. If the user responds to a subsequent "Do you want to save this > password?" prompt, we ask this object to save or blacklist the associated login > information in Chrome's password store. Done.
LGTM https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/23537029/diff/4001/chrome/browser/content_set... chrome/browser/content_settings/tab_specific_content_settings.h:300: // or when the tab is closed. On 2013/09/13 08:31:26, npentrel wrote: > On 2013/09/12 20:21:00, Peter Kasting wrote: > > So why is it that we do set_password_action() and then wait for a > navigation/tab > > closure to apply the change, instead of combining those two steps into one and > > making user action in the infobar immediately "apply change" with the desired > > change? After all, the UI disappears once the user responds to it, right? > > First of all, you are right the UI disappears once the user responds to it but > we now have the option of bringing the UI up again by clicking on the icon > (which remains visible until the next navigation occurs). Hence the user can > still change the choice. (There will be changes to the bubble that appears if > the user decides to change the choice but for now it pops up the same bubble.) > My first inclination was to apply the choice the user makes as soon as it is > made but Garrett convinced me to change this because I would have needed to undo > the previously chosen action. (i.e. if the user chooses save and then never save > for this site, the saved password has to be deleted before the site is > blacklisted - which is not a big problem but the other case where the user first > chooses to blacklist and then to save is a bit intricate as the blacklisting > action deletes all saved passwords for that site and those would need to also be > re-added when the user decides to now save the password instead). We might still > change this but that depends on some design decisions that need to be made. Thanks, this is a great explanation. Perhaps this sentence should be added to the end of the comment: "The change isn't applied immediately because until the next navigation the user can still recall the UI and change the desired action, and undoing a blacklist operation is nontrivial."
@Markus: Please review tab_specific_content_settings.* @Garrett: Please review password_form_manager.* These have a few changes now. Thanks, Naomi
lgtm
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/npentrel@chromium.org/23537029/38001
Message was sent while issue was closed.
Change committed as 223608 |
