Issue 10396003: Add Icon Support for New Autofill Gtk UI

Issue 10396003: Add Icon Support for New Autofill Gtk UI (Closed)

Created:
8 years, 7 months ago by csharp

Modified:
8 years, 7 months ago

Reviewers:
Elliot Glaysher, Ilya Sherman

CC:
chromium-reviews, dhollowa+watch_chromium.org, James Su, Ilya Sherman, dyu1

Base URL:
http://git.chromium.org/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

Add Icon Support for New Autofill Gtk UI Show credit card icons for the new Autofill Gtk popup. BUG=51644 TEST= Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137126

Patch Set 3 : #

Created: 8 years, 7 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+129 lines, -42 lines)			Patch
M	chrome/browser/autofill/autofill_external_delegate.h	View		2 chunks	+4 lines, -1 line	0 comments	Download
M	chrome/browser/autofill/autofill_external_delegate.cc	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/autofill/autofill_external_delegate_gtk.cc	View	1	2 chunks	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/autofill/autofill_popup_view.h	View	1 2	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/autofill/autofill_popup_view.cc	View	1 2	3 chunks	+27 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.h	View	1	3 chunks	+5 lines, -3 lines	0 comments	Download
M	chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc	View	1	11 chunks	+55 lines, -20 lines	0 comments	Download
M	chrome/browser/ui/gtk/gtk_util.h	View		2 chunks	+11 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/gtk/gtk_util.cc	View		1 chunk	+12 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/gtk/omnibox/omnibox_popup_view_gtk.cc	View		3 chunks	+9 lines, -17 lines	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

csharp

8 years, 7 months ago (2012-05-11 19:26:37 UTC) #1

Ilya Sherman

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/autofill/autofill_popup_view.cc File chrome/browser/autofill/autofill_popup_view.cc (right): https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/autofill/autofill_popup_view.cc#newcode80 chrome/browser/autofill/autofill_popup_view.cc:80: if (autofill_icons_[i] != string16()) nit: if (!autofill_icons()[i].empty()) https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/autofill/autofill_popup_view.cc#newcode81 chrome/browser/autofill/autofill_popup_view.cc:81: ...

8 years, 7 months ago (2012-05-11 22:12:17 UTC) #2

csharp

8 years, 7 months ago (2012-05-14 14:19:02 UTC) #5

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/autofil...
File chrome/browser/autofill/autofill_popup_view.cc (right):

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_view.cc:80: if (autofill_icons_[i] !=
string16())
On 2012/05/11 22:12:17, Ilya Sherman wrote:
> nit: if (!autofill_icons()[i].empty())

Done.

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_view.cc:81: has_icons_ = true;
On 2012/05/11 22:12:17, Ilya Sherman wrote:
> nit: You can add a break; stmt here as well

Done.

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_view.cc:180: std::string name =
UTF16ToUTF8(resource_name);
On 2012/05/11 22:12:17, Ilya Sherman wrote:
> On 2012/05/11 19:26:37, csharp wrote:
> > I'm not sure how to best do the comparison between the string16 here and the
> > const char* I define in the file. If I try to define the const chars as
> > string16s, I get a compile error, "declaration requires an exit-time
> > destructor".
> > 
> > Any thoughts on how to better do this?
> 
> You can use a char16 pointer for the string constants, or you can use
> ASCIIToString16() to up-convert the constants to UTF16 (theoretically, it's
> faster to convert ASCII up than to convert across UTF's... though for this
code
> the performance difference probably doesn't actually matter).

K, I'll just use ASCIIToUTF16 to up the constants

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_popup_view.cc:183: if (!strcmp(name.c_str(),
kDataResources[i].name))
On 2012/05/11 22:12:17, Ilya Sherman wrote:
> nit: Please use std::string's or string16's operator== rather than strcmp
here.

Done.

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/...
File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:27: // The vertical
height of each row.
On 2012/05/11 22:12:17, Ilya Sherman wrote:
> nit: "in pixels"

Done.

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:39: // Width of the
icons.
On 2012/05/11 22:12:17, Ilya Sherman wrote:
> nit: "in pixels"

Done.

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:42: // Height of the
icons
On 2012/05/11 22:12:17, Ilya Sherman wrote:
> nit: "in pixels"

Done.

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:65:
theme_service_(GtkThemeService::GetFrom(profile)),
On 2012/05/11 22:12:17, Ilya Sherman wrote:
> nit: How about passing the GtkThemeService directly, rather than the profile?

Done.

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:206: if (has_icons())
On 2012/05/11 22:12:17, Ilya Sherman wrote:
> Hmm, interesting.  Did you try the other option, of having rows that lack
icons
> have their labels extend further to the right?  I'm not sure offhand which
would
> be preferable -- I can see advantages and disadvantages to both.  If you tried
> both and prefer how this looks, then that works for me :)

In my quick testing I don't think there is output where lines with just labels
and lines with labels and icons mixed. Only credit cards have the icons and I
don't think credit card profiles and other profiles can get mixed (although I
could be wrong). Do you know of some way to get that mix?

So I don't think it matters which one we pick (atm), but after thinking about it
a bit it might be better to right align the labels with no icons if we do end up
having them intermixed.

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:217: if
(autofill_icons()[i] != string16()) {
On 2012/05/11 22:12:17, Ilya Sherman wrote:
> nit: if (!autofill_icons()[i].empty())

Done.

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:218: int icon =
GetIconResourceID(autofill_icons()[i].c_str());
On 2012/05/11 22:12:17, Ilya Sherman wrote:
> nit: Why are you passing the c_str()?

Good catch, that was from when I was trying different things earlier. Fixed.

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:219: if (icon != -1) {
On 2012/05/11 22:12:17, Ilya Sherman wrote:
> nit: Can this be a DCHECK rather than an if-stmt?

Yup, I had thought GetImageNamed might crash with bad input, but it looks like
it will just return an empty image.

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:336: if
(autofill_icons()[i] != string16())
On 2012/05/11 22:12:17, Ilya Sherman wrote:
> nit: if (!autofill_icons()[i].empty())

Done.

Ilya Sherman

LGTM https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right): https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc#newcode206 chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:206: if (has_icons()) On 2012/05/14 14:19:03, csharp wrote: > ...

8 years, 7 months ago (2012-05-14 20:38:21 UTC) #6

LGTM

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/...
File chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10396003/diff/1/chrome/browser/ui/gtk/...
chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc:206: if (has_icons())
On 2012/05/14 14:19:03, csharp wrote:
> On 2012/05/11 22:12:17, Ilya Sherman wrote:
> > Hmm, interesting.  Did you try the other option, of having rows that lack
> icons
> > have their labels extend further to the right?  I'm not sure offhand which
> would
> > be preferable -- I can see advantages and disadvantages to both.  If you
tried
> > both and prefer how this looks, then that works for me :)
> 
> In my quick testing I don't think there is output where lines with just labels
> and lines with labels and icons mixed. Only credit cards have the icons and I
> don't think credit card profiles and other profiles can get mixed (although I
> could be wrong). Do you know of some way to get that mix?
> 
> So I don't think it matters which one we pick (atm), but after thinking about
it
> a bit it might be better to right align the labels with no icons if we do end
up
> having them intermixed.

The only way that I can think of to have mixed icons and labels is if a credit
card form field has a <datalist> element associated with it.  Since <datalist>
isn't supported by the new UI yet, I agree that it doesn't matter which we pick
for now.  Looking forward for when we do add support for <datalist>, I'm happy
with the current choice to right-align the labels without icons.

https://chromiumcodereview.appspot.com/10396003/diff/7001/chrome/browser/auto...
File chrome/browser/autofill/autofill_popup_view.h (right):

https://chromiumcodereview.appspot.com/10396003/diff/7001/chrome/browser/auto...
chrome/browser/autofill/autofill_popup_view.h:99: bool has_icons() { return
has_icons_; }
nit: No longer needed?

https://chromiumcodereview.appspot.com/10396003/diff/7001/chrome/browser/auto...
chrome/browser/autofill/autofill_popup_view.h:133: bool has_icons_;
nit: No longer needed?

csharp

https://chromiumcodereview.appspot.com/10396003/diff/7001/chrome/browser/autofill/autofill_popup_view.h File chrome/browser/autofill/autofill_popup_view.h (right): https://chromiumcodereview.appspot.com/10396003/diff/7001/chrome/browser/autofill/autofill_popup_view.h#newcode99 chrome/browser/autofill/autofill_popup_view.h:99: bool has_icons() { return has_icons_; } On 2012/05/14 20:38:22, ...

8 years, 7 months ago (2012-05-15 13:19:08 UTC) #7

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/10396003/4002

8 years, 7 months ago (2012-05-15 13:19:56 UTC) #8

Change committed as 137126

Expand Messages | Collapse Messages