Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1791)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 23980003: 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.

Description

This is relanding of https://codereview.chromium.org/22975006/ which was reverted in https://codereview.chromium.org/24024006. Save password functionality added to the save password bubble behind flag. The buttons now let the user save and blacklist passwords. BUG=261628 TBR=pkasting@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222034

Patch Set 1 #

Total comments: 36

Messages

Total messages: 10 (0 generated)
npentrel
This is a minor fix of a regression that was caused by the previous CL: ...
7 years, 3 months ago (2013-09-09 08:21:38 UTC) #1
vabr (Chromium)
On 2013/09/09 08:21:38, npentrel wrote: > This is a minor fix of a regression that ...
7 years, 3 months ago (2013-09-09 08:25:56 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/npentrel@chromium.org/23980003/1
7 years, 3 months ago (2013-09-09 08:26:18 UTC) #3
commit-bot: I haz the power
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&number=76724
7 years, 3 months ago (2013-09-09 14:08:21 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/npentrel@chromium.org/23980003/1
7 years, 3 months ago (2013-09-09 14:10:57 UTC) #5
commit-bot: I haz the power
Change committed as 222034
7 years, 3 months ago (2013-09-09 15:09:25 UTC) #6
Peter Kasting
Functionally this works, but I think there are a lot of small things that can ...
7 years, 3 months ago (2013-09-09 18:19:02 UTC) #7
npentrel
Addressing comments here: https://codereview.chromium.org/23537029/ https://codereview.chromium.org/23980003/diff/1/chrome/browser/content_settings/tab_specific_content_settings.h File chrome/browser/content_settings/tab_specific_content_settings.h (right): https://codereview.chromium.org/23980003/diff/1/chrome/browser/content_settings/tab_specific_content_settings.h#newcode299 chrome/browser/content_settings/tab_specific_content_settings.h:299: // the next navigation. On ...
7 years, 3 months ago (2013-09-09 22:08:34 UTC) #8
Peter Kasting
(Sorry for replying here and not on the other CL, but since your comments were ...
7 years, 3 months ago (2013-09-10 00:22:11 UTC) #9
npentrel
7 years, 3 months ago (2013-09-10 16:46:37 UTC) #10
Message was sent while issue was closed. 
Changes are on new CL, as above.

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.

Powered by Google App Engine
This is Rietveld 408576698