Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23463013:
[gtk] Save password bubble (Closed)
|
Created:
7 years, 3 months ago by npentrel Modified:
7 years, 3 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionThis is the gtk implementation of the save password bubble. This code depends on another CL (https://codereview.chromium.org/22975006/).
BUG=261628
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222761
Patch Set 1 #
Total comments: 3
Patch Set 2 : Change alignment of manage link #
Total comments: 3
Patch Set 3 : changing the grab focus #Patch Set 4 : Review 3 #Patch Set 5 : Review 3 #Patch Set 6 : test #Patch Set 7 : removing unused file #Patch Set 8 : removing unchanged file #Patch Set 9 : minor change #Patch Set 10 : Review 4 #
Total comments: 1
Patch Set 11 : Review 5 #Patch Set 12 : indent change #
Messages
Total messages: 15 (0 generated)
Please have a look at this CL. Thanks, Naomi
https://codereview.chromium.org/23463013/diff/1/chrome/browser/ui/gtk/content... File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (right): https://codereview.chromium.org/23463013/diff/1/chrome/browser/ui/gtk/content... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:368: gtk_box_pack_end(GTK_BOX(bottom_box), manage_link, FALSE, FALSE, 0); why is this not shared with the nearly identical line of code in the else case? Is there actually a function difference between end and start in this case?
https://codereview.chromium.org/23463013/diff/1/chrome/browser/ui/gtk/content... File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (right): https://codereview.chromium.org/23463013/diff/1/chrome/browser/ui/gtk/content... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:368: gtk_box_pack_end(GTK_BOX(bottom_box), manage_link, FALSE, FALSE, 0); On 2013/09/05 00:52:55, Evan Stade wrote: > why is this not shared with the nearly identical line of code in the else case? > Is there actually a function difference between end and start in this case? Yes there is a difference: end will put it at the end of the box, meaning the manage_link will appear at the right end of the box, whereas for start it will appear at the beginning of the box (meaning on the left). This is necessary if there is a 'done' button on the left but for the save-password bubble there is no 'done' button and the manage_link should appear on the right.
https://codereview.chromium.org/23463013/diff/1/chrome/browser/ui/gtk/content... File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (right): https://codereview.chromium.org/23463013/diff/1/chrome/browser/ui/gtk/content... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:368: gtk_box_pack_end(GTK_BOX(bottom_box), manage_link, FALSE, FALSE, 0); On 2013/09/05 08:00:16, npentrel wrote: > On 2013/09/05 00:52:55, Evan Stade wrote: > > why is this not shared with the nearly identical line of code in the else > case? > > Is there actually a function difference between end and start in this case? > > Yes there is a difference: end will put it at the end of the box, meaning the > manage_link will appear at the right end of the box, whereas for start it will > appear at the beginning of the box (meaning on the left). This is necessary if > there is a 'done' button on the left but for the save-password bubble there is > no 'done' button and the manage_link should appear on the right. not sure why we'd move the link to the right when there's no Done button. Did some UI designer come up with a mock that highlighted this as an important difference of the save password bubble? If you look at other links (for example the one at the bottom of the 'connection' tab of the cookies + permission bubble) they are left-flush.
On 2013/09/05 16:43:00, Evan Stade wrote: > https://codereview.chromium.org/23463013/diff/1/chrome/browser/ui/gtk/content... > File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (right): > > https://codereview.chromium.org/23463013/diff/1/chrome/browser/ui/gtk/content... > chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:368: > gtk_box_pack_end(GTK_BOX(bottom_box), manage_link, FALSE, FALSE, 0); > On 2013/09/05 08:00:16, npentrel wrote: > > On 2013/09/05 00:52:55, Evan Stade wrote: > > > why is this not shared with the nearly identical line of code in the else > > case? > > > Is there actually a function difference between end and start in this case? > > > > Yes there is a difference: end will put it at the end of the box, meaning the > > manage_link will appear at the right end of the box, whereas for start it will > > appear at the beginning of the box (meaning on the left). This is necessary if > > there is a 'done' button on the left but for the save-password bubble there is > > no 'done' button and the manage_link should appear on the right. > > not sure why we'd move the link to the right when there's no Done button. Did > some UI designer come up with a mock that highlighted this as an important > difference of the save password bubble? If you look at other links (for example > the one at the bottom of the 'connection' tab of the cookies + permission > bubble) they are left-flush. true, I changed the alignment accordingly.
https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (left): https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:354: gtk_widget_grab_focus(button); I don't understand why there were previously two grab_focus calls in a row, but it seems that inverting the order of them might not do what you want? does the grab_focus call on the |bottom_box| even do anything? Gtk docs say that it needs to be focusable to have any effect.
On 2013/09/06 21:15:45, Evan Stade wrote: > https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... > File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (left): > > https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... > chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:354: > gtk_widget_grab_focus(button); > I don't understand why there were previously two grab_focus calls in a row, but > it seems that inverting the order of them might not do what you want? > > does the grab_focus call on the |bottom_box| even do anything? Gtk docs say that > it needs to be focusable to have any effect. I changed it so that it doesn't alter the order anymore. For me the additional focus calls didn't do much because the focus is on 'save' already but I am not sure whether the two grab_focus calls in a row are important for other cases.
https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (left): https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:354: gtk_widget_grab_focus(button); On 2013/09/06 21:15:45, Evan Stade wrote: > I don't understand why there were previously two grab_focus calls in a row, but > it seems that inverting the order of them might not do what you want? > > does the grab_focus call on the |bottom_box| even do anything? Gtk docs say that > it needs to be focusable to have any effect. they were added here: https://codereview.chromium.org/1360007 let's see if there's any response. I'd rather not just leave this mysterious code here (we can at least add a comment to it).
On 2013/09/09 20:44:07, Evan Stade wrote: > https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... > File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (left): > > https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... > chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:354: > gtk_widget_grab_focus(button); > On 2013/09/06 21:15:45, Evan Stade wrote: > > I don't understand why there were previously two grab_focus calls in a row, > but > > it seems that inverting the order of them might not do what you want? > > > > does the grab_focus call on the |bottom_box| even do anything? Gtk docs say > that > > it needs to be focusable to have any effect. > > they were added here: https://codereview.chromium.org/1360007 > > let's see if there's any response. I'd rather not just leave this mysterious > code here (we can at least add a comment to it). I think you can remove the first grab.
I removed the second grab_focus call. https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (left): https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:354: gtk_widget_grab_focus(button); On 2013/09/09 20:44:07, Evan Stade wrote: > On 2013/09/06 21:15:45, Evan Stade wrote: > > I don't understand why there were previously two grab_focus calls in a row, > but > > it seems that inverting the order of them might not do what you want? > > > > does the grab_focus call on the |bottom_box| even do anything? Gtk docs say > that > > it needs to be focusable to have any effect. > > they were added here: https://codereview.chromium.org/1360007 > > let's see if there's any response. I'd rather not just leave this mysterious > code here (we can at least add a comment to it). Done.
On 2013/09/10 16:25:34, npentrel wrote: > I removed the second grab_focus call. > > https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... > File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (left): > > https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... > chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:354: > gtk_widget_grab_focus(button); > On 2013/09/09 20:44:07, Evan Stade wrote: > > On 2013/09/06 21:15:45, Evan Stade wrote: > > > I don't understand why there were previously two grab_focus calls in a row, > > but > > > it seems that inverting the order of them might not do what you want? > > > > > > does the grab_focus call on the |bottom_box| even do anything? Gtk docs say > > that > > > it needs to be focusable to have any effect. > > > > they were added here: https://codereview.chromium.org/1360007 > > > > let's see if there's any response. I'd rather not just leave this mysterious > > code here (we can at least add a comment to it). > > Done. why did you remove the second instead of the first?
On 2013/09/10 21:42:06, Evan Stade wrote: > On 2013/09/10 16:25:34, npentrel wrote: > > I removed the second grab_focus call. > > > > > https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... > > File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (left): > > > > > https://codereview.chromium.org/23463013/diff/8001/chrome/browser/ui/gtk/cont... > > chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:354: > > gtk_widget_grab_focus(button); > > On 2013/09/09 20:44:07, Evan Stade wrote: > > > On 2013/09/06 21:15:45, Evan Stade wrote: > > > > I don't understand why there were previously two grab_focus calls in a > row, > > > but > > > > it seems that inverting the order of them might not do what you want? > > > > > > > > does the grab_focus call on the |bottom_box| even do anything? Gtk docs > say > > > that > > > > it needs to be focusable to have any effect. > > > > > > they were added here: https://codereview.chromium.org/1360007 > > > > > > let's see if there's any response. I'd rather not just leave this mysterious > > > code here (we can at least add a comment to it). > > > > Done. > > why did you remove the second instead of the first? I thought you were referring to that one, as the order had changed in my changes. In any case, I changed it again.
lgtm https://codereview.chromium.org/23463013/diff/39001/chrome/browser/ui/gtk/con... File chrome/browser/ui/gtk/content_setting_bubble_gtk.cc (right): https://codereview.chromium.org/23463013/diff/39001/chrome/browser/ui/gtk/con... chrome/browser/ui/gtk/content_setting_bubble_gtk.cc:372: this); indent
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/npentrel@chromium.org/23463013/46001
Message was sent while issue was closed.
Change committed as 222761 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
