Give request-to-act badges a grey background, and increase spacing to make it fit.

Give request-to-act badges a grey background, and increase spacing to make it fit.

Screen shots: http://imgur.com/a/COWzt
Original mock: https://docs.google.com/a/google.com/folder/d/0B21JpVYxVLsASVVyZlJDb3d3a2M/edit?docId=0B21JpVYxVLsAVE56VWptV3RDTHc

BUG=135460, 133142
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157619

[Jeffrey Yasskin, 2012-08-20 23:39:57]

Views and GTK basically work with this patch. Peter, could you check it for
basic sanity before I implement it for Mac?

Thanks,
Jeffrey

[Aaron Boodman, 2012-08-21 23:12:15]

Since the changes to c/c/e/* are ui related, I'm going to go ahead and say lgtm
if it lgtp.

[Aaron Boodman, 2012-08-24 15:36:53]

http://codereview.chromium.org/10834279/diff/2001/chrome/browser/ui/gtk/locat...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10834279/diff/2001/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:123: const int kInnerPadding = 9;
We only want to change the layout in the case where script badges are enabled.

http://codereview.chromium.org/10834279/diff/2001/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1205: gfx::CanvasSkiaPaint
canvas(event, false);
Document the magic boolean, or else make it a named constant.

[Peter Kasting, 2012-08-24 23:08:28]

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/browser/ui/g...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/browser/ui/g...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1205: gfx::CanvasSkiaPaint
canvas(event, false);
On 2012/08/24 15:36:53, Aaron Boodman wrote:
> Document the magic boolean, or else make it a named constant.

Honestly I'm not a huge fan of documenting at a callsite the value of something
that someone can simply look at the relevant class definition to find out...

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/browser/ui/g...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1206: for (size_t i = 0; i <
page_action_views_.size(); ++i) {
Nit: Use iterator (see location_bar_view.cc)

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/browser/ui/g...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1219: bounds.Inset(-kInnerPadding
/ 2, -2,
How come we have to offset by -2 here and -1 in views?  It seems like the GTK
boxes ought to extend to flush inside the visible edge, so that we could then
just subtract 1 here (and you could then apply the technique suggested in
location_bar_view.cc).

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

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:115: const int
kDesktopItemPadding = 9;
Is increasing this for all items really the right thing to do?  Or should we
have different spacing for items without a background?  Or should we be drawing
faint lines between all the items, as in the mock, so the items with a
background don't look quite as different and the otherwise-big gaps aren't
weird?

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1226: for (size_t i =
0; i < page_action_views_.size(); ++i) {
Nit: Since all you do with i is to get page_action_views_[i], use an iterator.

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1234:
bounds.Inset(-horizontal_padding / 2, -1,
Nit: Don't use -1s here.  Split the kVerticalEdgeThickness constant into two
constants, kVisibleEdgeThickness (= 1) and kVerticalPaddingOutsideEdge (= 2) and
define the former in LocationBar instead of here (see extension_action.cc for
why).  Then subtract off that visible edge thickness constant here.

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/common/exten...
File chrome/common/extensions/extension_action.cc (right):

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/common/exten...
chrome/common/extensions/extension_action.cc:194: const SkColor
ExtensionAction::kGetAttentionBorderColor =
You need to get the correct colors from the theming system, as the omnibox
borders and background colors can be anything.

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/common/exten...
chrome/common/extensions/extension_action.cc:384: SkIntToScalar(bounds.y() +
1)},
Nit: Once you've defined LocationBar::kVisibleEdgeThickness (see
location_bar_view.cc), you can subtract that off here instead of just "-1".

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/common/exten...
chrome/common/extensions/extension_action.cc:394: gfx::Rect bg_rect(bounds.x() -
1, bounds.y(),
Why do we draw outside our bounds?  Shouldn't our bounds include the whole area
we draw on?

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/common/exten...
chrome/common/extensions/extension_action.cc:395: bounds.width() + 1,
bounds.height() - 1);
What's this last -1 mean?

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/common/exten...
File chrome/common/extensions/extension_action.h (right):

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/common/exten...
chrome/common/extensions/extension_action.h:42: static const SkColor
kGetAttentionBorderColor;
Nit: Why are these public class members instead of, say, function- or file-scope
constants in the .cc file?

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/common/exten...
chrome/common/extensions/extension_action.h:253: // two ExtensionActions, so
both ExtensionActions can draw on it.
Nit: You might want to note in this comment what sort of background we're
painting (e.g. that only WantsAttention() cases get a background at all, and
what it looks like).

https://chromiumcodereview.appspot.com/10834279/diff/2001/chrome/common/exten...
chrome/common/extensions/extension_action.h:254: void
PaintBackground(gfx::Canvas* canvas, const gfx::Rect& bounds,
Nit: One line per arg

[Jeffrey Yasskin, 2012-08-29 00:37:50]

http://codereview.chromium.org/10834279/diff/2001/chrome/browser/ui/gtk/locat...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/10834279/diff/2001/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:123: const int kInnerPadding = 9;
On 2012/08/24 15:36:53, Aaron Boodman wrote:
> We only want to change the layout in the case where script badges are enabled.

Good point. Done.

http://codereview.chromium.org/10834279/diff/2001/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:126: const int kStarRightPadding
= 4;
Turns out this was unused.

http://codereview.chromium.org/10834279/diff/2001/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1205: gfx::CanvasSkiaPaint
canvas(event, false);
On 2012/08/24 23:08:28, Peter Kasting wrote:
> On 2012/08/24 15:36:53, Aaron Boodman wrote:
> > Document the magic boolean, or else make it a named constant.
> 
> Honestly I'm not a huge fan of documenting at a callsite the value of
something
> that someone can simply look at the relevant class definition to find out...

I am a fan of doing this, and just forgot here. I find that locating the
relevant class definition is often hard/tedious enough that I just copy/paste
another use that I find, instead of figuring out what the value actually should
be. In this case ... should the argument actually be 'true'?

http://codereview.chromium.org/10834279/diff/2001/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1206: for (size_t i = 0; i <
page_action_views_.size(); ++i) {
On 2012/08/24 23:08:28, Peter Kasting wrote:
> Nit: Use iterator (see location_bar_view.cc)

Done.

http://codereview.chromium.org/10834279/diff/2001/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1219: bounds.Inset(-kInnerPadding
/ 2, -2,
On 2012/08/24 23:08:28, Peter Kasting wrote:
> How come we have to offset by -2 here and -1 in views?  It seems like the GTK
> boxes ought to extend to flush inside the visible edge, so that we could then
> just subtract 1 here (and you could then apply the technique suggested in
> location_bar_view.cc).

The visible edge in gtk (http://i.imgur.com/tmB3A.png) and the  visible edge in
views (http://i.imgur.com/iluNi.png) are different sizes.

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

http://codereview.chromium.org/10834279/diff/2001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:115: const int
kDesktopItemPadding = 9;
On 2012/08/24 23:08:28, Peter Kasting wrote:
> Is increasing this for all items really the right thing to do?  Or should we
> have different spacing for items without a background?

Aaron asked for the extra spacing on the star in http://crbug.com/135460. I
haven't seen anyone explicitly say what should happen for the other, non-star,
icons.

> Or should we be drawing
> faint lines between all the items, as in the mock, so the items with a
> background don't look quite as different and the otherwise-big gaps aren't
> weird?

The mocks only include the faint line between the star and the action box:
https://docs.google.com/a/google.com/folder/d/0B21JpVYxVLsASVVyZlJDb3d3a2M/ed....

http://codereview.chromium.org/10834279/diff/2001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1226: for (size_t i =
0; i < page_action_views_.size(); ++i) {
On 2012/08/24 23:08:28, Peter Kasting wrote:
> Nit: Since all you do with i is to get page_action_views_[i], use an iterator.

Done.

http://codereview.chromium.org/10834279/diff/2001/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1234:
bounds.Inset(-horizontal_padding / 2, -1,
On 2012/08/24 23:08:28, Peter Kasting wrote:
> Nit: Don't use -1s here.  Split the kVerticalEdgeThickness constant into two
> constants, kVisibleEdgeThickness (= 1) and kVerticalPaddingOutsideEdge (= 2)
and
> define the former in LocationBar instead of here (see extension_action.cc for
> why).  Then subtract off that visible edge thickness constant here.

In Views, the visible edge thickness is 1 on the top and 2 on the bottom. Ah,
but it would be 3 on both sides, with the darkest pixel in the center, were it
not for the drawRoundRect call at
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/lo...,
and the fact that the top pixel of omnibox_c.png is transparent.

In the mocks, the top request-to-act border is at the top of the 3-pixel visible
location-bar border, but the bottom border is also at the top of the 2-pixel
visible location-bar border.

http://codereview.chromium.org/10834279/diff/2001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action.cc (right):

http://codereview.chromium.org/10834279/diff/2001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:194: const SkColor
ExtensionAction::kGetAttentionBorderColor =
On 2012/08/24 23:08:28, Peter Kasting wrote:
> You need to get the correct colors from the theming system, as the omnibox
> borders and background colors can be anything.

Oh good. I'll figure out how to do this after sending off this reply.

http://codereview.chromium.org/10834279/diff/2001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:384: SkIntToScalar(bounds.y() +
1)},
On 2012/08/24 23:08:28, Peter Kasting wrote:
> Nit: Once you've defined LocationBar::kVisibleEdgeThickness (see
> location_bar_view.cc), you can subtract that off here instead of just "-1".

This isn't necessarily the thickness of the location bar edge, it's the
thickness of the request-to-act background's edge. Also, the top and bottom
edges of the location bar are different thicknesses, in different directions
depending on the platform.

I could instead make a png for the background, check it into  the themes
directory, and stamp it onto the canvas? I'm not sure if I should use
Canvas::DrawImageInt or SkCanvas::drawBitmapNine in that case.

http://codereview.chromium.org/10834279/diff/2001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:394: gfx::Rect bg_rect(bounds.x() -
1, bounds.y(),
On 2012/08/24 23:08:28, Peter Kasting wrote:
> Why do we draw outside our bounds?  Shouldn't our bounds include the whole
area
> we draw on?

I think this came from before I had the platform-specific code setting the
bounds in the right way. Fixed.

http://codereview.chromium.org/10834279/diff/2001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.cc:395: bounds.width() + 1,
bounds.height() - 1);
On 2012/08/24 23:08:28, Peter Kasting wrote:
> What's this last -1 mean?

This also came from before I got the platform-specific code in its current form.
Somehow I got the impression that the bounds specified in widget locations
extend one past the right and bottom edges, but DrawRect draws exactly on the
specified rectangle, which meant that the rectangle here needed to be one pixel
smaller than the bounds.

Now the platform-specific code sets these bounds to point at the pixels that
should be used for the border, so there's no need to adjust them here anymore.
The gradient keeps its -1 since I shouldn't have included the border in its
endpoints in the first place.

http://codereview.chromium.org/10834279/diff/2001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_action.h (right):

http://codereview.chromium.org/10834279/diff/2001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.h:42: static const SkColor
kGetAttentionBorderColor;
On 2012/08/24 23:08:28, Peter Kasting wrote:
> Nit: Why are these public class members instead of, say, function- or
file-scope
> constants in the .cc file?

At one point I was using them from the platform-specific code, but I'm not
anymore.

http://codereview.chromium.org/10834279/diff/2001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.h:253: // two ExtensionActions, so
both ExtensionActions can draw on it.
On 2012/08/24 23:08:28, Peter Kasting wrote:
> Nit: You might want to note in this comment what sort of background we're
> painting (e.g. that only WantsAttention() cases get a background at all, and
> what it looks like).

Done.

http://codereview.chromium.org/10834279/diff/2001/chrome/common/extensions/ex...
chrome/common/extensions/extension_action.h:254: void
PaintBackground(gfx::Canvas* canvas, const gfx::Rect& bounds,
On 2012/08/24 23:08:28, Peter Kasting wrote:
> Nit: One line per arg

Oops, done.

[Peter Kasting, 2012-08-29 01:00:07]

I don't know whether we're speaking the same language on visible edge thickness.
 I was thinking that there's an obvious-looking single edge pixel on the inside
with some other pixels outside it that might be shadows or highlights or
whatever but aren't so clearly part of the "edge", and the content inside the
location bar runs flush up to the edge.  This is how things work on views.

I can't quite tell but it looks from your GTK screenshot like maybe below the
top edge there is another faint shadow pixel that none of the content in the
location bar overlaps.  This sort of throws wrenches into my ideas.  :(  (I
don't know that that's sticking around though -- I think we're shrinking all the
location bars vertically and we may not have room for that anymore on GTK now?)

In any case, the idea here was that the top and bottom "edges" of the
request-to-act background and the "visible edges" of the omnibox are always
supposed to be the same thickness (currently 1 pixel) and appear at the same
location; if for some reason we e.g. made the omnibox border 2 px on the 200%
UI, we'd want the request-to-act background to follow suit.  The suggestions I
had about how to organize constants sprung from this idea.

[Jeffrey Yasskin, 2012-08-29 01:59:02]

I'm starting to think it makes more sense to have an image as the
background instead of trying to draw this with code.
* If the omnibox border changes in the 200% UI, it's easy to replace
the request-to-act bg image but harder to make the code do the right
thing with two different borders.
 * It's easy to make an image background start drawing a pixel or two
in, but harder to figure out meaningful names for the pixel offsets to
do the same thing.
 * The theme system doesn't seem well suited to adding colors easily,
but it does look easy to add an image to theme_resources.grd.

I'll go in that direction for now.

On Tue, Aug 28, 2012 at 6:00 PM,  <pkasting@chromium.org> wrote:
> I don't know whether we're speaking the same language on visible edge
> thickness.
>  I was thinking that there's an obvious-looking single edge pixel on the
> inside
> with some other pixels outside it that might be shadows or highlights or
> whatever but aren't so clearly part of the "edge", and the content inside
> the
> location bar runs flush up to the edge.  This is how things work on views.
>
> I can't quite tell but it looks from your GTK screenshot like maybe below
> the
> top edge there is another faint shadow pixel that none of the content in the
> location bar overlaps.  This sort of throws wrenches into my ideas.  :(  (I
> don't know that that's sticking around though -- I think we're shrinking all
> the
> location bars vertically and we may not have room for that anymore on GTK
> now?)
>
> In any case, the idea here was that the top and bottom "edges" of the
> request-to-act background and the "visible edges" of the omnibox are always
> supposed to be the same thickness (currently 1 pixel) and appear at the same
> location; if for some reason we e.g. made the omnibox border 2 px on the
> 200%
> UI, we'd want the request-to-act background to follow suit.  The suggestions
> I
> had about how to organize constants sprung from this idea.
>
> http://codereview.chromium.org/10834279/

[Jeffrey Yasskin, 2012-08-29 20:58:47]

Ok, here's another attempt. Aura screenshot at http://i.imgur.com/Ne9mc.png, and
gtk at http://i.imgur.com/YpQZd.png.

[Peter Kasting, 2012-08-29 21:30:23]

If you're going to use images, they need to use alpha so that they work over all
theme backgrounds.

It's generally much easier to do this programmatically.  We already have lots of
blending and auto-color-picking (to preserve contrast) functions that can choose
the right colors for you given some reference points.  (For example, you can
easily ask for a color midway between the location bar background and text
colors).

[Jeffrey Yasskin, 2012-08-29 21:44:19]

So ... what reference points or alpha values should I choose in this
case? All I have are the non-transparent, non-themed mocks that Roma
sketched out. If you think this will be easier in the long run if it's
drawn using code, it might make sense to commit it without theme-based
colors, and update it to include them when Roma has time to actually
specify the design.

Also, where are the functions you're referring to? I don't see them
mentioned in either theme_service.h or theme_provider.h.

Thanks,
Jeffrey

On Wed, Aug 29, 2012 at 2:30 PM,  <pkasting@chromium.org> wrote:
> If you're going to use images, they need to use alpha so that they work over
> all
> theme backgrounds.
>
> It's generally much easier to do this programmatically.  We already have
> lots of
> blending and auto-color-picking (to preserve contrast) functions that can
> choose
> the right colors for you given some reference points.  (For example, you can
> easily ask for a color midway between the location bar background and text
> colors).
>
> http://codereview.chromium.org/10834279/

[Jeffrey Yasskin, 2012-08-29 21:44:19]

So ... what reference points or alpha values should I choose in this
case? All I have are the non-transparent, non-themed mocks that Roma
sketched out. If you think this will be easier in the long run if it's
drawn using code, it might make sense to commit it without theme-based
colors, and update it to include them when Roma has time to actually
specify the design.

Also, where are the functions you're referring to? I don't see them
mentioned in either theme_service.h or theme_provider.h.

Thanks,
Jeffrey

On Wed, Aug 29, 2012 at 2:30 PM,  <pkasting@chromium.org> wrote:
> If you're going to use images, they need to use alpha so that they work over
> all
> theme backgrounds.
>
> It's generally much easier to do this programmatically.  We already have
> lots of
> blending and auto-color-picking (to preserve contrast) functions that can
> choose
> the right colors for you given some reference points.  (For example, you can
> easily ask for a color midway between the location bar background and text
> colors).
>
> http://codereview.chromium.org/10834279/

[Peter Kasting, 2012-08-29 22:19:48]

On Wed, Aug 29, 2012 at 2:43 PM, Jeffrey Yasskin <jyasskin@chromium.org>wrote:

> So ... what reference points or alpha values should I choose in this
> case?


On views, I'd do something like:

  ToolbarModel::SecurityLevel security_level =
ToolbarModel::GetSecurityLevel();
  SkColor background_color = color_utils::AlphaBlend(
      LocationBarView::GetColor(security_level, LocationBarView::TEXT),

  LocationBarView::GetColor(security_level, LocationBarView::BACKGROUND),
20);

This should produce something very close to your Aura screenshot, work on
all color schemes, and work correctly if we do something like change the
location bar background color for EV or something someday (as we used to do
for SSL when we first launched).

We could probably hoist GetColor() from LocationBarView to LocationBar and
implement for other platforms if that helps.

PK

[Peter Kasting, 2012-08-29 22:19:48]

On Wed, Aug 29, 2012 at 2:43 PM, Jeffrey Yasskin <jyasskin@chromium.org>wrote:

> So ... what reference points or alpha values should I choose in this
> case?


On views, I'd do something like:

  ToolbarModel::SecurityLevel security_level =
ToolbarModel::GetSecurityLevel();
  SkColor background_color = color_utils::AlphaBlend(
      LocationBarView::GetColor(security_level, LocationBarView::TEXT),

  LocationBarView::GetColor(security_level, LocationBarView::BACKGROUND),
20);

This should produce something very close to your Aura screenshot, work on
all color schemes, and work correctly if we do something like change the
location bar background color for EV or something someday (as we used to do
for SSL when we first launched).

We could probably hoist GetColor() from LocationBarView to LocationBar and
implement for other platforms if that helps.

PK

[Jeffrey Yasskin, 2012-09-05 21:10:19]

Here's another attempt, only tested on Windows.

https://chromiumcodereview.appspot.com/10834279/diff/26001/chrome/browser/ext...
File chrome/browser/extensions/extension_action_util.cc (right):

https://chromiumcodereview.appspot.com/10834279/diff/26001/chrome/browser/ext...
chrome/browser/extensions/extension_action_util.cc:27: const SkColor text_color
= theme.GetColor(ThemeService::COLOR_NTP_TEXT);
This isn't exactly right, since chrome-themes can affect the NTP_TEXT and
NTP_BACKGROUND colors, while only system themes can affect the location bar's
text and background. It does give the right colors when either the chrome theme
matches the system theme or the chrome theme is left at the default.

I tried moving LocationBarView::GetColor() to LocationBar, but then I noticed
that it's static and called from several places outside of LocationBarView, so
I'd have to put all three platform implementations directly in LocationBar (or
location_bar_util.cc). This wouldn't be a big problem, except the way
gtk_theme_service.cc gets the system colors is by instantiating a GtkEntry and
asking its colors, and this doesn't work so well in a static function.

Another option is to give the ThemeService some colors specifically for the
location bar, which can't be set by the dynamic theme.

And a third option is to pass the text and background colors into
PaintBackground() and use platform-specific code to pick them.

[Peter Kasting, 2012-09-05 22:42:11]

You might ask erg@ about GTK theming issues.  Also the GTK port is going away in
not terribly long as we'll be porting over to Aura.

However, in the meantime, your "third option" might be best.  Let each
individual platform simply provide the appropriate color(s), then you mix them
and draw yourself.

[Jeffrey Yasskin, 2012-09-06 18:34:23]

On 2012/09/05 22:42:11, Peter Kasting wrote:
> You might ask erg@ about GTK theming issues.  Also the GTK port is going away
in
> not terribly long as we'll be porting over to Aura.
> 
> However, in the meantime, your "third option" might be best.  Let each
> individual platform simply provide the appropriate color(s), then you mix them
> and draw yourself.

Third option implemented.

+erg to tell me if my GTK strategy is crazy.

Starting on Mac now.

Any suggestions on how I should write tests for this? Just unit tests, or is
there some stable way to check the actual appearance?

[Elliot Glaysher, 2012-09-06 20:35:15]

On 2012/09/06 18:34:23, Jeffrey Yasskin wrote:
> On 2012/09/05 22:42:11, Peter Kasting wrote:
> > You might ask erg@ about GTK theming issues.  Also the GTK port is going
away
> in
> > not terribly long as we'll be porting over to Aura.
> > 
> > However, in the meantime, your "third option" might be best.  Let each
> > individual platform simply provide the appropriate color(s), then you mix
them
> > and draw yourself.
> 
> Third option implemented.
> 
> +erg to tell me if my GTK strategy is crazy.
> 
> Starting on Mac now.
> 
> Any suggestions on how I should write tests for this? Just unit tests, or is
> there some stable way to check the actual appearance?

The strategy of grabbing GTK colors and doctoring or blending them is what we
usually do. I'd like to see you change your desktop theme to, say, Clearlooks or
High Contrast Inverse to see how things change, since not just colors, but also
padding and spacing is under control of the gtk theme.

[Jeffrey Yasskin, 2012-09-06 23:12:28]

Clearlooks:
[image: Inline image 1]
New Wave:
[image: Inline image 2]
High Contrast Inverse:
[image: Inline image 3]

Do you know how to ask the theme how thick its location bar frame is, or to
cope with thicker frames? I haven't been able to find anything that looks
good on all themes.


On Thu, Sep 6, 2012 at 1:35 PM, <erg@chromium.org> wrote:

> On 2012/09/06 18:34:23, Jeffrey Yasskin wrote:
>
>> On 2012/09/05 22:42:11, Peter Kasting wrote:
>> > You might ask erg@ about GTK theming issues.  Also the GTK port is
>> going
>>
> away
>
>> in
>> > not terribly long as we'll be porting over to Aura.
>> >
>> > However, in the meantime, your "third option" might be best.  Let each
>> > individual platform simply provide the appropriate color(s), then you
>> mix
>>
> them
>
>> > and draw yourself.
>>
>
>  Third option implemented.
>>
>
>  +erg to tell me if my GTK strategy is crazy.
>>
>
>  Starting on Mac now.
>>
>
>  Any suggestions on how I should write tests for this? Just unit tests, or
>> is
>> there some stable way to check the actual appearance?
>>
>
> The strategy of grabbing GTK colors and doctoring or blending them is what
> we
> usually do. I'd like to see you change your desktop theme to, say,
> Clearlooks or
> High Contrast Inverse to see how things change, since not just colors, but
> also
> padding and spacing is under control of the gtk theme.
>
>
https://chromiumcodereview.**appspot.com/10834279/<https://chromiumcodereview...
>

[Elliot Glaysher, 2012-09-07 16:44:49]

On Thu, Sep 6, 2012 at 4:12 PM, Jeffrey Yasskin <jyasskin@chromium.org>wrote:

> Clearlooks:
> [image: Inline image 1]
> New Wave:
> [image: Inline image 2]
> High Contrast Inverse:
> [image: Inline image 3]
>
> Do you know how to ask the theme how thick its location bar frame is, or
> to cope with thicker frames? I haven't been able to find anything that
> looks good on all themes.
>

You don't. Here's the gtk drawing model (way simplified):

You first call gtk_draw_textbox(x, y, w, h);
GTK then dispatches that to an dynamically loaded library that correspond's
with the user's current theme. (Clearlooks is an .so that issues a bunch of
drawing commands, not a set of images.)
That code can draw anything. (And IIRC, in earlier versions of GTK, could
manipulate the stack of clip rects so it could draw anywhere, including
outside of the passed in rectangle.)

If you still want to try, look at the section "style properties" in
http://developer.gnome.org/gtk/2.24/GtkWidget.html . I'm not sure it's
possible to derive what you want from that though.

(This, btw, is why I moved the bookmark star into the omnibox back when it
had its own decoration. Drawing an icon on top of the result works, as I
never found a general way to do full vertical integration here.)

(Also FYI: In the linux_aura gtk2 theming, I plan on not copying the gtk
drawing style of text entires;  I'll just rip the colors and tint the
current views omnibox IDR_* the correct colors.)

-- Elliot


>
> On Thu, Sep 6, 2012 at 1:35 PM, <erg@chromium.org> wrote:
>
>> On 2012/09/06 18:34:23, Jeffrey Yasskin wrote:
>>
>>> On 2012/09/05 22:42:11, Peter Kasting wrote:
>>> > You might ask erg@ about GTK theming issues.  Also the GTK port is
>>> going
>>>
>> away
>>
>>> in
>>> > not terribly long as we'll be porting over to Aura.
>>> >
>>> > However, in the meantime, your "third option" might be best.  Let each
>>> > individual platform simply provide the appropriate color(s), then you
>>> mix
>>>
>> them
>>
>>> > and draw yourself.
>>>
>>
>>  Third option implemented.
>>>
>>
>>  +erg to tell me if my GTK strategy is crazy.
>>>
>>
>>  Starting on Mac now.
>>>
>>
>>  Any suggestions on how I should write tests for this? Just unit tests,
>>> or is
>>> there some stable way to check the actual appearance?
>>>
>>
>> The strategy of grabbing GTK colors and doctoring or blending them is
>> what we
>> usually do. I'd like to see you change your desktop theme to, say,
>> Clearlooks or
>> High Contrast Inverse to see how things change, since not just colors,
>> but also
>> padding and spacing is under control of the gtk theme.
>>
>>
https://chromiumcodereview.**appspot.com/10834279/<https://chromiumcodereview...
>>
>
>

[Peter Kasting, 2012-09-07 22:29:33]

https://chromiumcodereview.appspot.com/10834279/diff/33001/chrome/browser/ext...
File chrome/browser/extensions/extension_action_util.h (right):

https://chromiumcodereview.appspot.com/10834279/diff/33001/chrome/browser/ext...
chrome/browser/extensions/extension_action_util.h:32: void
PaintExtensionActionBackground(const ExtensionAction& action,
This should be in some subdirectory of chrome/browser/ui/ instead.  I suggest
chrome/browser/ui/omnibox/.

https://chromiumcodereview.appspot.com/10834279/diff/33001/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://chromiumcodereview.appspot.com/10834279/diff/33001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/location_bar_view.cc:376:
extensions::switch_utils::AreScriptBadgesEnabled() ?
Nit: Chrome frowns on nested ?:s (especially without parens).  I suggest a
conditional return followed by a "return ?:".  Also, do we want something like
this eventually for touch as well?  (2 places)

[Jeffrey Yasskin, 2012-09-11 01:16:56]

This should be ready for an actual review now. It works on all three platforms,
although there are some interactions with the top and bottom omnibox edges when
gtk themes change its thickness and on MacOS when focus adds a blue border
inside the omnibox. I'd like to get the overall look checked in so the UI folks
can explore it, and then I'll clean up details when they give me a final spec.

http://codereview.chromium.org/10834279/diff/33001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_action_util.h (right):

http://codereview.chromium.org/10834279/diff/33001/chrome/browser/extensions/...
chrome/browser/extensions/extension_action_util.h:32: void
PaintExtensionActionBackground(const ExtensionAction& action,
On 2012/09/07 22:29:33, Peter Kasting wrote:
> This should be in some subdirectory of chrome/browser/ui/ instead.  I suggest
> chrome/browser/ui/omnibox/.

Done.

http://codereview.chromium.org/10834279/diff/33001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10834279/diff/33001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:376:
extensions::switch_utils::AreScriptBadgesEnabled() ?
On 2012/09/07 22:29:33, Peter Kasting wrote:
> Nit: Chrome frowns on nested ?:s (especially without parens).  I suggest a
> conditional return followed by a "return ?:".  Also, do we want something like
> this eventually for touch as well?  (2 places)

Done. I don't know what we want for touch, but I'd rather not guess. Once this
is checked in and the UI folks can click around in it, I'll fix it up however
they want.

[Peter Kasting, 2012-09-11 22:27:47]

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/extension_action_util.cc (right):

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/extension_action_util.cc:23: if
(action.WantsAttention(tab_id)) {
Nit: Or early return so you can unindent rest of function.

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/extension_action_util.cc:41:
canvas->DrawLine(gfx::Point(bounds.x(), bounds.y()),
Nit: First arg can be "bounds.origin()"

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/extension_action_util.cc:45: // bottom edges.
So two adjacent badges that request attention get a double-thickness line
between them?

That seems kinda suboptimal... maybe we could pass in an arg that says whether
to draw this line?

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/extension_action_util.h (right):

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/extension_action_util.h:9: // component.
Nit: Thanks for moving this to this directory.  Should this maybe go in
LocationBar (the mostly-abstract base class) itself, or if not then maybe in
location_bar_util.h?

I just don't tend to like random files with one function in them.  I'm not
convinced the function here really belongs in the extensions namespace either,
it seems more like "part of the location bar" than "part of extensions".

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1270: // mid-point
between two page actions in the bounding rectangle.  For even
Does this mean that the items' bounds might overlap by one pixel?  That's not
good because we won't necessarily behave consistently if a user clicks on that
pixel.

We should ensure the bounds don't overlap.

[Elliot Glaysher, 2012-09-11 22:30:34]

gtk changes are minimal and lgtm.

[Jeffrey Yasskin, 2012-09-11 23:05:28]

I haven't made any changes yet, pending your answers to the comments below.

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/extension_action_util.cc (right):

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/extension_action_util.cc:41:
canvas->DrawLine(gfx::Point(bounds.x(), bounds.y()),
On 2012/09/11 22:27:47, Peter Kasting wrote:
> Nit: First arg can be "bounds.origin()"

I don't mind making this change, but it seemed more consistent to write out the
coordinates given that there's no "bottom_left()" method to match origin().

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/extension_action_util.cc:45: // bottom edges.
On 2012/09/11 22:27:47, Peter Kasting wrote:
> So two adjacent badges that request attention get a double-thickness line
> between them?
> 
> That seems kinda suboptimal... maybe we could pass in an arg that says whether
> to draw this line?

No, they get a single-thickness line between them. The "right()" coordinate is 1
to the right of the right-most pixel contained within the bounds. (This is
vaguely documented in ui/gfx/rect.h, and is what I mean by "half-open".) If we
have 2 badges requesting attention, "|A|B|", then A's bounds.right() points to
the x coordinate of the first pixel to the right of the line separating A and B.
B's bounds.x() will point exactly to the border between A and B.

I considered adjusting the Rect to avoid this oddity, but that seems
inconsistent with the other drawing code (DrawLine omits its final point, for
example) and with the code that retrieves object bounds. The only exception I
found is that Canvas::DrawRect, with style=stroke, draws on the right and bottom
edges of the rectangle, instead of shifting in by 1, which strikes me as an
unfixable bug rather than evidence that Rect actually uses the other model.

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/extension_action_util.h (right):

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/extension_action_util.h:9: // component.
On 2012/09/11 22:27:47, Peter Kasting wrote:
> Nit: Thanks for moving this to this directory.  Should this maybe go in
> LocationBar (the mostly-abstract base class) itself, or if not then maybe in
> location_bar_util.h?
> 
> I just don't tend to like random files with one function in them.  I'm not
> convinced the function here really belongs in the extensions namespace either,
> it seems more like "part of the location bar" than "part of extensions".

I don't care where it lives, but I've already moved it once to the place you
specified. If you wanted it in a different place, you should have said so in
your previous comment. I'm willing to move it one more time, so please tell me
exactly where you want it.

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1270: // mid-point
between two page actions in the bounding rectangle.  For even
On 2012/09/11 22:27:47, Peter Kasting wrote:
> Does this mean that the items' bounds might overlap by one pixel?  That's not
> good because we won't necessarily behave consistently if a user clicks on that
> pixel.
> 
> We should ensure the bounds don't overlap.

I believe this doesn't affect the code that detects which icon has been clicked
on, and that there's a region between the icons that does nothing when you click
on it. If you don't like this aspect of the UI design, I'd appreciate if I could
check the current version of the code in, and let you and the other UI folks
tell me what behavior you want in a subsequent CL.

[Peter Kasting, 2012-09-11 23:25:41]

LGTM, most stuff resolved by IM

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/extension_action_util.cc (right):

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/extension_action_util.cc:41:
canvas->DrawLine(gfx::Point(bounds.x(), bounds.y()),
On 2012/09/11 23:05:28, Jeffrey Yasskin wrote:
> On 2012/09/11 22:27:47, Peter Kasting wrote:
> > Nit: First arg can be "bounds.origin()"
> 
> I don't mind making this change, but it seemed more consistent to write out
the
> coordinates given that there's no "bottom_left()" method to match origin().

That's OK.  I'd prefer to keep the code more simple as opposed to more
parallel-structured.

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/extension_action_util.h (right):

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/extension_action_util.h:9: // component.
On 2012/09/11 23:05:28, Jeffrey Yasskin wrote:
> On 2012/09/11 22:27:47, Peter Kasting wrote:
> > Nit: Thanks for moving this to this directory.  Should this maybe go in
> > LocationBar (the mostly-abstract base class) itself, or if not then maybe in
> > location_bar_util.h?
> > 
> > I just don't tend to like random files with one function in them.  I'm not
> > convinced the function here really belongs in the extensions namespace
either,
> > it seems more like "part of the location bar" than "part of extensions".
> 
> I don't care where it lives, but I've already moved it once to the place you
> specified. If you wanted it in a different place, you should have said so in
> your previous comment. I'm willing to move it one more time, so please tell me
> exactly where you want it.

Sorry, I didn't mean to yank your chain around on this :(

I just wasn't thinking hard before about whether there were existing files in
this directory this should be combined with.  (I didn't even know
location_bar_util.h existed.)  I guess I'd probably put the function into the
location_bar_util file/namespace, since if you put it into LocationBar it would
have to be public and static.

[Jeffrey Yasskin, 2012-09-12 20:29:34]

Thanks for the reviews! Let's see if I need any more.

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/extension_action_util.cc (right):

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/extension_action_util.cc:23: if
(action.WantsAttention(tab_id)) {
On 2012/09/11 22:27:47, Peter Kasting wrote:
> Nit: Or early return so you can unindent rest of function.

Done.

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/extension_action_util.cc:41:
canvas->DrawLine(gfx::Point(bounds.x(), bounds.y()),
On 2012/09/11 23:25:41, Peter Kasting wrote:
> On 2012/09/11 23:05:28, Jeffrey Yasskin wrote:
> > On 2012/09/11 22:27:47, Peter Kasting wrote:
> > > Nit: First arg can be "bounds.origin()"
> > 
> > I don't mind making this change, but it seemed more consistent to write out
> the
> > coordinates given that there's no "bottom_left()" method to match origin().
> 
> That's OK.  I'd prefer to keep the code more simple as opposed to more
> parallel-structured.

Done.

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/extension_action_util.h (right):

http://codereview.chromium.org/10834279/diff/38001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/extension_action_util.h:9: // component.
On 2012/09/11 23:25:41, Peter Kasting wrote:
> On 2012/09/11 23:05:28, Jeffrey Yasskin wrote:
> > On 2012/09/11 22:27:47, Peter Kasting wrote:
> > > Nit: Thanks for moving this to this directory.  Should this maybe go in
> > > LocationBar (the mostly-abstract base class) itself, or if not then maybe
in
> > > location_bar_util.h?
> > > 
> > > I just don't tend to like random files with one function in them.  I'm not
> > > convinced the function here really belongs in the extensions namespace
> either,
> > > it seems more like "part of the location bar" than "part of extensions".
> > 
> > I don't care where it lives, but I've already moved it once to the place you
> > specified. If you wanted it in a different place, you should have said so in
> > your previous comment. I'm willing to move it one more time, so please tell
me
> > exactly where you want it.
> 
> Sorry, I didn't mean to yank your chain around on this :(
> 
> I just wasn't thinking hard before about whether there were existing files in
> this directory this should be combined with.  (I didn't even know
> location_bar_util.h existed.)  I guess I'd probably put the function into the
> location_bar_util file/namespace, since if you put it into LocationBar it
would
> have to be public and static.

Done.

[commit-bot: I haz the power, 2012-09-12 20:29:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/10834279/37003

[commit-bot: I haz the power, 2012-09-12 21:35:39]

Try job failure for 10834279-37003 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[Jeffrey Yasskin, 2012-09-12 21:56:06]

+shess to double-check the cocoa code, since I missed a place that I changed the
behavior in non-script-badge mode.

[Scott Hess - ex-Googler, 2012-09-12 22:54:08]

For some reason, when I patch your change in, when I have multiple page actions,
clicking on any of them always gives the same page action's popup.  I suspect
your changes in the cell have messed up the calculations about which item was
being pointed to.

Hmm.  It keeps showing me the first one I click on.  So if I click on
"lastpass", then "turn off the lights", I see the same popup as for "lastpass". 
If I do it in the other order, I see the "turn off the lights" popup twice. 
Maybe it isn't the tracking.

http://codereview.chromium.org/10834279/diff/37016/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

http://codereview.chromium.org/10834279/diff/37016/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:31: inline
CGFloat RightDecorationXOffset() {
Shouldn't need inline.  It's static to this file, so the compiler can figure it
out.  Or not.  But don't second-guess it.

Also, pull the constants into the function so that nobody accidentally uses them
directly.

http://codereview.chromium.org/10834279/diff/37016/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

http://codereview.chromium.org/10834279/diff/37016/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:100: void
PageActionDecoration::DrawInFrame(NSRect frame, NSView* control_view) {
I'm not entirely comfortable with this.  Having to pull in global info from the
cell feels wrong, but having to re-arrange your frame is definitely indicating a
problem.  The magic all should be baked into what is passed into here by the
cell.

I'm thinking on how to refactor, and might change my mind, but it seems like a
reasonable short-term solution would be to add a method like this to the root
decoration:

void LocationBarDecoration::DrawWithBackgroundInFrame(
    NSRect background_frame,
    NSRect frame,
    NSView* control_view) {
  DrawWithFrame(frame, control_view);
}

and then override that to accomplish your change, with the cell passing in the
right background_frame before calling DrawWithBackgroundInFrame().  Another
option would be DrawBackgroundInFrame(), so the cell could call that, then
DrawInFrame(), but that feels clunky (it works nicely with the implementation of
CanvasSkiaPaint, but doesn't feel very natural).

Well, there's also a problem that this code draws incorrectly when the
background is drawn while the field is in focus.  Is that case not possible for
some reason?  I just hard-wired to see it.

[Jeffrey Yasskin, 2012-09-14 22:13:45]

On 2012/09/12 22:54:08, shess wrote:
> For some reason, when I patch your change in, when I have multiple page
actions,
> clicking on any of them always gives the same page action's popup.  I suspect
> your changes in the cell have messed up the calculations about which item was
> being pointed to.
> 
> Hmm.  It keeps showing me the first one I click on.  So if I click on
> "lastpass", then "turn off the lights", I see the same popup as for
"lastpass". 
> If I do it in the other order, I see the "turn off the lights" popup twice. 
> Maybe it isn't the tracking.

Yep, that bug seems to predate this change. There are a bunch of similar
problems in the behavior of script badges which I haven't fixed yet.

http://codereview.chromium.org/10834279/diff/37016/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

http://codereview.chromium.org/10834279/diff/37016/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:31: inline
CGFloat RightDecorationXOffset() {
On 2012/09/12 22:54:08, shess wrote:
> Shouldn't need inline.  It's static to this file, so the compiler can figure
it
> out.  Or not.  But don't second-guess it.
> 
> Also, pull the constants into the function so that nobody accidentally uses
them
> directly.

Done.

http://codereview.chromium.org/10834279/diff/37016/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

http://codereview.chromium.org/10834279/diff/37016/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:100: void
PageActionDecoration::DrawInFrame(NSRect frame, NSView* control_view) {
On 2012/09/12 22:54:08, shess wrote:
> I'm not entirely comfortable with this.  Having to pull in global info from
the
> cell feels wrong, but having to re-arrange your frame is definitely indicating
a
> problem.  The magic all should be baked into what is passed into here by the
> cell.
> 
> I'm thinking on how to refactor, and might change my mind, but it seems like a
> reasonable short-term solution would be to add a method like this to the root
> decoration:
> 
> void LocationBarDecoration::DrawWithBackgroundInFrame(
>     NSRect background_frame,
>     NSRect frame,
>     NSView* control_view) {
>   DrawWithFrame(frame, control_view);
> }
> 
> and then override that to accomplish your change, with the cell passing in the
> right background_frame before calling DrawWithBackgroundInFrame().

Done.

> Well, there's also a problem that this code draws incorrectly when the
> background is drawn while the field is in focus.  Is that case not possible
for
> some reason?  I just hard-wired to see it.

I'm not sure how to detect that the focus ring is present, and I'm not sure
exactly what the UI folks want to do in that case (change the background size to
remove those pixel rows or make it a different color?), so I'm planning to leave
it not-quite-right for now, and fix it when they make a decision.

[Scott Hess - ex-Googler, 2012-09-18 20:40:32]

LGTM, with the suggested change in the super call to the DrawInFrame() call.

On 2012/09/14 22:13:45, Jeffrey Yasskin wrote:
> On 2012/09/12 22:54:08, shess wrote:
> > Well, there's also a problem that this code draws incorrectly when the
> > background is drawn while the field is in focus.  Is that case not possible
for
> > some reason?  I just hard-wired to see it.
> 
> I'm not sure how to detect that the focus ring is present, and I'm not sure
> exactly what the UI folks want to do in that case (change the background size
to
> remove those pixel rows or make it a different color?), so I'm planning to
leave
> it not-quite-right for now, and fix it when they make a decision.

OK, just be sure to circle back around on it.  This is getting into where I
thought you might have challenges fitting it into the decoration without
substantial involvement from the cell.  I'll be somewhat surprised if you manage
to use a shared drawing model for the background and don't get negative feedback
from other OSX developers on the team.  I think it's more likely that the
backgrounds will have to be drawn in some integrated fashion.  [I don't have any
real suggestions, I just know that it was annoying to get the border right
as-is, and this will need to integrate with the border, thus this will likely be
annoying to get right.]

I think nico mentioned someday going with a three-part-image type solution on
OSX.  If so, that will probably make it easier/cleaner, or at least
broken-by-design.

http://codereview.chromium.org/10834279/diff/27037/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

http://codereview.chromium.org/10834279/diff/27037/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:109:
ImageDecoration::DrawInFrame(frame, control_view);
Either call the DrawInFrame() for this class, or call super's
DrawWithBackgroundInFrame().  I can see arguments for either path, but calling
diagonally like this seems wrong.

[Jeffrey Yasskin, 2012-09-19 18:24:09]

https://codereview.chromium.org/10834279/diff/27037/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

https://codereview.chromium.org/10834279/diff/27037/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:109:
ImageDecoration::DrawInFrame(frame, control_view);
On 2012/09/18 20:40:33, shess wrote:
> Either call the DrawInFrame() for this class, or call super's
> DrawWithBackgroundInFrame().  I can see arguments for either path, but calling
> diagonally like this seems wrong.

Whoops, yes. I went with the super call in case ImageDecoration ever grows
background behavior.

[Scott Hess - ex-Googler, 2012-09-19 19:35:07]

LGTM

https://codereview.chromium.org/10834279/diff/27037/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

https://codereview.chromium.org/10834279/diff/27037/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:109:
ImageDecoration::DrawInFrame(frame, control_view);
On 2012/09/19 18:24:09, Jeffrey Yasskin wrote:
> On 2012/09/18 20:40:33, shess wrote:
> > Either call the DrawInFrame() for this class, or call super's
> > DrawWithBackgroundInFrame().  I can see arguments for either path, but
calling
> > diagonally like this seems wrong.
> 
> Whoops, yes. I went with the super call in case ImageDecoration ever grows
> background behavior.

Ha!  Doom!  Because they you'd probably want to separate drawing-the-background
from drawing-the-foreground so that you can call the super version, composite
over that, then composite the foreground over that.  Next step would be add
:before, :after, and :during qualifiers to determine when the super call
happens, and then ... *foam*

Actually, I hope that we either go forward with action box or revert everything
before this issue ever comes up.  In which case the various drawing entry points
can be combined back together again.

[commit-bot: I haz the power, 2012-09-19 20:19:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jyasskin@chromium.org/10834279/48001

[commit-bot: I haz the power, 2012-09-19 21:59:48]

Change committed as 157619