Fix up views web intents location bar decoration.

Fix up views web intents location bar decoration.

R=pkasting@chromium.org
BUG=148885
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157301

[Peter Kasting, 2012-09-15 00:23:46]

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (left):

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:50:
TouchableLocationBarView::Init(this);
Is killing the touch part here safe?

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc
(right):

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc:24: const
int kTextMargin = 5;
Should the icon margin and text margins match?  Should these match how the
keyword hint view draws things?

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc:47: if
(!GetImageSize(&preferred_size))
I don't actually understand what this is doing.  It doesn't look like we ever
call SetImage() so how can GetImageSize() ever succeed?  More fundamentally, if
we aren't calling SetImage(), why does this class inherit from ImageView to
begin with?

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc:177:
LOG(INFO) << "ImageBnds=" << GetImageBounds().right() << " " <<
GetImageBounds().width();
Don't check these in.  (Multiple places)

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc:179:
canvas->DrawStringInt(animated_text_, font_, SK_ColorDKGRAY,
This should not be hardcoding a text color.  We should be using the location
bar's text color, optionally passing it to color_utils::GetReadableColor() with
the relevant background color here (which, if the images are partly transparent,
may itself have to be calculated as an alpha-blend of a particular value with
the location bar's background color).  See how location_bar_view.cc calls
GetColor().

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/location_bar_decoration_view.h
(right):

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.h:65: void
SetFont(const gfx::Font& font);
Nit: Name set_font() and inline

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/web_intents_button_view.cc (right):

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/web_intents_button_view.cc:39:
AlwaysDrawText();
Nit: If the order of these statements matters, write a comment noting that and
explaining why.

[Greg Billock, 2012-09-15 04:58:20]

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (left):

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:50:
TouchableLocationBarView::Init(this);
On 2012/09/15 00:23:46, Peter Kasting wrote:
> Is killing the touch part here safe?

This is in the superclass

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc
(right):

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc:24: const
int kTextMargin = 5;
On 2012/09/15 00:23:46, Peter Kasting wrote:
> Should the icon margin and text margins match?  Should these match how the
> keyword hint view draws things?

The text margin is between the text and other controls. I'm not sure why that'd
have to match icon-left, but I'm happy to do it. I'm mostly reacting to the
specific feedback from UI. Shall I ask Jeff about this?

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc:47: if
(!GetImageSize(&preferred_size))
Me either. This is a failed attempt to get the vertical spacing right for the
background. Here's what I see: This method returns "21" for both the content
settings and the web intents controls. The view superclass is then returning 21
for height(), but OnPaintBackground is getting called with a height of 23 for
the WI control and 21 for the CS control.


On 2012/09/15 00:23:46, Peter Kasting wrote:
> I don't actually understand what this is doing.  It doesn't look like we ever
> call SetImage() so how can GetImageSize() ever succeed?  More fundamentally,
if
> we aren't calling SetImage(), why does this class inherit from ImageView to
> begin with?

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc:177:
LOG(INFO) << "ImageBnds=" << GetImageBounds().right() << " " <<
GetImageBounds().width();
Yeah, this is leftover trying to diagnose this height thing. Any advice would be
appreciated!

On 2012/09/15 00:23:46, Peter Kasting wrote:
> Don't check these in.  (Multiple places)

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc:179:
canvas->DrawStringInt(animated_text_, font_, SK_ColorDKGRAY,
Got it. I'll change this to take a color and call GetColor(ToolbarModel::NONE,
TEXT) in the location bar view.

On 2012/09/15 00:23:46, Peter Kasting wrote:
> This should not be hardcoding a text color.  We should be using the location
> bar's text color, optionally passing it to color_utils::GetReadableColor()
with
> the relevant background color here (which, if the images are partly
transparent,
> may itself have to be calculated as an alpha-blend of a particular value with
> the location bar's background color).  See how location_bar_view.cc calls
> GetColor().

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/location_bar_decoration_view.h
(right):

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.h:65: void
SetFont(const gfx::Font& font);
You sure? This SetFont signature is used all over the place in this directory
and elsewhere.

On 2012/09/15 00:23:46, Peter Kasting wrote:
> Nit: Name set_font() and inline

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/location_bar_view.cc:826: offset,
location_y, width, location_height);
found it...

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
File chrome/browser/ui/views/location_bar/web_intents_button_view.cc (right):

https://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locat...
chrome/browser/ui/views/location_bar/web_intents_button_view.cc:39:
AlwaysDrawText();
It shouldn't, but belongs this way. I'll add a comment.


On 2012/09/15 00:23:46, Peter Kasting wrote:
> Nit: If the order of these statements matters, write a comment noting that and
> explaining why.

[Greg Billock, 2012-09-15 14:29:12]

http://codereview.chromium.org/10917280/diff/5001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10917280/diff/5001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:721: }
Note to self: missing entry_width adjustment here! bug 149756

[Greg Billock, 2012-09-17 19:55:01]

On 2012/09/15 14:29:12, Greg Billock wrote:
>
http://codereview.chromium.org/10917280/diff/5001/chrome/browser/ui/views/loc...
> File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):
> 
>
http://codereview.chromium.org/10917280/diff/5001/chrome/browser/ui/views/loc...
> chrome/browser/ui/views/location_bar/location_bar_view.cc:721: }
> Note to self: missing entry_width adjustment here! bug 149756

Done now. Look good, Peter?

[Peter Kasting, 2012-09-17 20:04:27]

LGTM

http://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc
(right):

http://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.cc:24: const
int kTextMargin = 5;
On 2012/09/15 04:58:20, Greg Billock wrote:
> On 2012/09/15 00:23:46, Peter Kasting wrote:
> > Should the icon margin and text margins match?  Should these match how the
> > keyword hint view draws things?
> 
> The text margin is between the text and other controls. I'm not sure why
that'd
> have to match icon-left, but I'm happy to do it. I'm mostly reacting to the
> specific feedback from UI. Shall I ask Jeff about this?

Nah, don't worry about it.

http://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/location_bar_decoration_view.h
(right):

http://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.h:65: void
SetFont(const gfx::Font& font);
On 2012/09/15 04:58:20, Greg Billock wrote:
> You sure? This SetFont signature is used all over the place in this directory
> and elsewhere.

If the function is non-virtual and a cheap member accessor, it should be inlined
and named accordingly.

In a lot of cases we have SetFont() functions that violate one of these two
conditions (either they're virtual, or they change the font and then do other
stuff like call some "needs layout" function or something).  In a few cases, we
may simply have things named wrong because originally views violated the Chrome
style guide in a bunch of ways we've been slowly fixing over the years.

This applies to SetFontColor() too.

http://codereview.chromium.org/10917280/diff/5001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10917280/diff/5001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:294:
content_blocked_view->SetFontColor(GetColor(ToolbarModel::NONE, TEXT));
Can we just pass the font and color to the constructor?  Then we omit the need
for accessors entirely.  (2 places)

http://codereview.chromium.org/10917280/diff/5001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:721: }
On 2012/09/15 14:29:12, Greg Billock wrote:
> Note to self: missing entry_width adjustment here! bug 149756

Indeed.

http://codereview.chromium.org/10917280/diff/5001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/web_intents_button_view.cc (right):

http://codereview.chromium.org/10917280/diff/5001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/web_intents_button_view.cc:38: // the label
open.
This comment says what the code does; I was more interested in knowing _why_ the
code does it.

[Greg Billock, 2012-09-17 21:23:18]

http://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locati...
File chrome/browser/ui/views/location_bar/location_bar_decoration_view.h
(right):

http://codereview.chromium.org/10917280/diff/1/chrome/browser/ui/views/locati...
chrome/browser/ui/views/location_bar/location_bar_decoration_view.h:65: void
SetFont(const gfx::Font& font);
OK, cool. I'll rename it.

On 2012/09/17 20:04:27, Peter Kasting wrote:
> On 2012/09/15 04:58:20, Greg Billock wrote:
> > You sure? This SetFont signature is used all over the place in this
directory
> > and elsewhere.
> 
> If the function is non-virtual and a cheap member accessor, it should be
inlined
> and named accordingly.
> 
> In a lot of cases we have SetFont() functions that violate one of these two
> conditions (either they're virtual, or they change the font and then do other
> stuff like call some "needs layout" function or something).  In a few cases,
we
> may simply have things named wrong because originally views violated the
Chrome
> style guide in a bunch of ways we've been slowly fixing over the years.
> 
> This applies to SetFontColor() too.

http://codereview.chromium.org/10917280/diff/5001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10917280/diff/5001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:294:
content_blocked_view->SetFontColor(GetColor(ToolbarModel::NONE, TEXT));
On 2012/09/17 20:04:27, Peter Kasting wrote:
> Can we just pass the font and color to the constructor?  Then we omit the need
> for accessors entirely.  (2 places)

Done.

http://codereview.chromium.org/10917280/diff/5001/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/web_intents_button_view.cc (right):

http://codereview.chromium.org/10917280/diff/5001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/web_intents_button_view.cc:38: // the label
open.
On 2012/09/17 20:04:27, Peter Kasting wrote:
> This comment says what the code does; I was more interested in knowing _why_
the
> code does it.

Done.

[commit-bot: I haz the power, 2012-09-17 21:23:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10917280/4003

[Peter Kasting, 2012-09-17 22:13:35]

Small comment -- it would be nice if all the various classes that passed around
background images, parent view, font, color, etc. took their arguments in the
same order and the argument order matched the member initializer order.

https://chromiumcodereview.appspot.com/10917280/diff/4003/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10917280/diff/4003/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:828: // Now the web
intents button
Nit: While here, can you add a trailing period?

[commit-bot: I haz the power, 2012-09-18 01:22:17]

Retried try job too often for step(s) browser_tests

[Greg Billock, 2012-09-18 02:03:51]

https://chromiumcodereview.appspot.com/10917280/diff/4003/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10917280/diff/4003/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:828: // Now the web
intents button
On 2012/09/17 22:13:35, Peter Kasting wrote:
> Nit: While here, can you add a trailing period?

CQ kicked this back, so done.

[commit-bot: I haz the power, 2012-09-18 02:03:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10917280/14002

[commit-bot: I haz the power, 2012-09-18 04:43:41]

Change committed as 157301