New action box button images and supporting code

Updated images, changed code to support new image size. New images include transparent area that goes on top of location bar border.

BUG=125307
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=161926

[yefimt, 2012-10-11 22:21:03]

pkasting: chrome/browser/ui/views/location_bar/*
sail: chrome/app/theme/*

[sail, 2012-10-11 22:23:07]

Hi, could you make sure to run pngcrush on all images. Instructions are in
app/theme/README

[sail, 2012-10-11 22:23:18]

lgtm

[Peter Kasting, 2012-10-12 00:32:15]

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

https://chromiumcodereview.appspot.com/11095008/diff/4003/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:66:
kTopBorderOverlapThickness + 1);
Why do we need the +1?  Shouldn't height() already be pointing at
one-past-the-last pixel (much like width() does in the horizontal dimension)?

Also, if the top and bottom pixels of the image are fully transparent, can we
simply eliminate them from the image and avoid much or all of this hit test code
entirely?

https://chromiumcodereview.appspot.com/11095008/diff/4003/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/11095008/diff/4003/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.h:29: static const
int kBottomBorderOverlapThickness;
Nit: At least this second const doesn't need to be a member (though it may go
away entirely, see .cc file)

[yefimt, 2012-10-12 17:10:44]

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

https://chromiumcodereview.appspot.com/11095008/diff/4003/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:66:
kTopBorderOverlapThickness + 1);
The reason transparent pixels are included into image is to make image for 200%
double of sizes of 100% image.
Internal height of location bar for 100% is 23px and for 200% - 48px.

On 2012/10/12 00:32:15, Peter Kasting wrote:
> Why do we need the +1?  Shouldn't height() already be pointing at
> one-past-the-last pixel (much like width() does in the horizontal dimension)?
> 
> Also, if the top and bottom pixels of the image are fully transparent, can we
> simply eliminate them from the image and avoid much or all of this hit test
code
> entirely?

[Peter Kasting, 2012-10-12 17:36:20]

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

https://chromiumcodereview.appspot.com/11095008/diff/4003/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:66:
kTopBorderOverlapThickness + 1);
On 2012/10/12 17:10:44, yefimt wrote:
> The reason transparent pixels are included into image is to make image for
200%
> double of sizes of 100% image.
> Internal height of location bar for 100% is 23px and for 200% - 48px.

Why isn't the internal height exactly 2x?  Is that something that will be
changing?  If the heights aren't proportional how can we have correct
positioning without conditional code?

[yefimt, 2012-10-12 17:43:17]

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

https://chromiumcodereview.appspot.com/11095008/diff/4003/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:66:
kTopBorderOverlapThickness + 1);
In 200% mode, size of the control doubles but border stays the same.

Another limitation that same action box button images are used on MAC and they
draw borders using lines. Borders looks the same visually but sizes are
different (windows has 3px border image). As a result location bar on windows
29px and on mac 26px, both have same internal size.
On 2012/10/12 17:36:20, Peter Kasting wrote:
> On 2012/10/12 17:10:44, yefimt wrote:
> > The reason transparent pixels are included into image is to make image for
> 200%
> > double of sizes of 100% image.
> > Internal height of location bar for 100% is 23px and for 200% - 48px.
> 
> Why isn't the internal height exactly 2x?  Is that something that will be
> changing?  If the heights aren't proportional how can we have correct
> positioning without conditional code?

[Peter Kasting, 2012-10-12 18:07:23]

OK, I understand why we'd have 23/48 px then.

But it still seems like we could make the code simpler by removing the 1 px of
transparency at the top and bottom of both the images.  Yes, this would make the
200% image not be 2x the size of the 100% image, but I don't see why that's a
problem.

Plus I still don't understand the whole "- 2 + 1" thing going on with the height
calculation.

[yefimt, 2012-10-12 18:16:57]

See inline
On 2012/10/12 18:07:23, Peter Kasting wrote:
> OK, I understand why we'd have 23/48 px then.
> 
> But it still seems like we could make the code simpler by removing the 1 px of
> transparency at the top and bottom of both the images.  Yes, this would make
the
> 200% image not be 2x the size of the 100% image, but I don't see why that's a
> problem.
It is how it is implemented now (23px and 48px), but MAC developers strongly
insisted images to be exact 2x of each other. I don't think it is worse to argue
as it could be easily done. 

> 
> Plus I still don't understand the whole "- 2 + 1" thing going on with the
height
> calculation.
Yes, I don't understand either yet, will look into it

[yefimt, 2012-10-12 20:09:38]

https://chromiumcodereview.appspot.com/11095008/diff/4003/chrome/browser/ui/v...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/11095008/diff/4003/chrome/browser/ui/v...
chrome/browser/ui/views/location_bar/action_box_button_view.h:29: static const
int kBottomBorderOverlapThickness;
Removed second static area and made clickable height settable from location bar.

On 2012/10/12 00:32:15, Peter Kasting wrote:
> Nit: At least this second const doesn't need to be a member (though it may go
> away entirely, see .cc file)

[Peter Kasting, 2012-10-12 20:54:49]

On 2012/10/12 18:16:57, yefimt wrote:
> See inline

BTW, in the future, please try to always reply-at-bottom on codereview comments
and emails, otherwise it's very hard to follow replies.

> On 2012/10/12 18:07:23, Peter Kasting wrote:
> > But it still seems like we could make the code simpler by removing the 1 px
of
> > transparency at the top and bottom of both the images.  Yes, this would make
> the
> > 200% image not be 2x the size of the 100% image, but I don't see why that's
a
> > problem.
> It is how it is implemented now (23px and 48px), but MAC developers strongly
> insisted images to be exact 2x of each other. I don't think it is worse to
argue
> as it could be easily done. 

Which developers?  I want to understand what's going on and why.

Oh, another small note -- it's Mac rather than MAC, MAC stands for Media Access
Control (as in MAC address for a network adapter) so if you say that it's very
confusing :)

[Peter Kasting, 2012-10-12 20:57:30]

https://chromiumcodereview.appspot.com/11095008/diff/25001/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/11095008/diff/25001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:28: void
SetClickableHeight(int clickable_height) {
This is much worse than the code before.  The location bar should definitely not
know about concepts like this.

The old code was weird not because it had a bottom area at all but because for
some reason the bottom area was declared as 2 px instead of 1 but then you
manually added 1 back.  That math made no sense, but the abstraction was in the
right place.

But honestly I don't understand why we care so much about not making the top and
bottom pixel clickable.  The location bar (on Windows at least) already treats
clicks on the bottom border as clicks in the location bar, why shouldn't we just
skip all the hittesting stuff entirely?

[yefimt, 2012-10-12 20:59:00]

macourteau@ was implementing Mac side of action box button and after talking to
his reviewer got back to me saying that 2x images has to be literary 2x

On 2012/10/12 20:54:49, Peter Kasting wrote:
> On 2012/10/12 18:16:57, yefimt wrote:
> > See inline
> 
> BTW, in the future, please try to always reply-at-bottom on codereview
comments
> and emails, otherwise it's very hard to follow replies.
> 
> > On 2012/10/12 18:07:23, Peter Kasting wrote:
> > > But it still seems like we could make the code simpler by removing the 1
px
> of
> > > transparency at the top and bottom of both the images.  Yes, this would
make
> > the
> > > 200% image not be 2x the size of the 100% image, but I don't see why
that's
> a
> > > problem.
> > It is how it is implemented now (23px and 48px), but MAC developers strongly
> > insisted images to be exact 2x of each other. I don't think it is worse to
> argue
> > as it could be easily done. 
> 
> Which developers?  I want to understand what's going on and why.
> 
> Oh, another small note -- it's Mac rather than MAC, MAC stands for Media
Access
> Control (as in MAC address for a network adapter) so if you say that it's very
> confusing :)

[Peter Kasting, 2012-10-12 21:06:21]

On 2012/10/12 20:59:00, yefimt wrote:
> macourteau@ was implementing Mac side of action box button and after talking
to
> his reviewer got back to me saying that 2x images has to be literary 2x

+macourteau.  Marc-Antoine, could you explain more about the Mac constraints
here?  On views at least it makes things easier to eliminate the transparent
pixel at the top and bottom of the images and just display each 1 px below the
location bar top border row, even though this means the overall image size for
200% is not the same as 100%.  What problems does this cause on Mac?

[stromme, 2012-10-12 21:19:04]

> +macourteau.  Marc-Antoine, could you explain more about the Mac constraints
> here?  On views at least it makes things easier to eliminate the transparent
> pixel at the top and bottom of the images and just display each 1 px below the
> location bar top border row, even though this means the overall image size for
> 200% is not the same as 100%.  What problems does this cause on Mac?

On Mac the @2x images must be exactly double the size of the normal images. They
are loaded by framework level code and this requirement can't be changed by the
developer. See the following in the Apple Developer Guide for more information:

http://developer.apple.com/library/mac/#documentation/GraphicsAnimation/Conce...

The options here are to have two sets of images (one for views, one for osx) or
to adopt the views code as this CL does to handle 2x images that are actually
2x.

See the equivalent CL in mac land: http://codereview.chromium.org/11135002/

[Peter Kasting, 2012-10-12 21:20:59]

On 2012/10/12 21:19:04, stromme wrote:
> On Mac the @2x images must be exactly double the size of the normal images.
They
> are loaded by framework level code and this requirement can't be changed by
the
> developer.

OK, fair enough.  Thanks for the detail!

[yefimt, 2012-10-12 22:53:37]

On 2012/10/12 21:20:59, Peter Kasting wrote:
> On 2012/10/12 21:19:04, stromme wrote:
> > On Mac the @2x images must be exactly double the size of the normal images.
> They
> > are loaded by framework level code and this requirement can't be changed by
> the
> > developer.
> 
> OK, fair enough.  Thanks for the detail!

Hi Peter,
I think all your comments were addressed. Let me know if I'm wrong.

[Peter Kasting, 2012-10-12 22:56:14]

No, they're not.

SetClickableHeight() shouldn't exist.  clickable_height_ shouldn't exist.  The
top border overlap has mysteriously changed from 1 to 2, meaning either the
previous patches or this patch position the image wrongly (I'm inclined to
suspect this one).

[yefimt, 2012-10-12 23:09:18]

Why clickable_height_ should not exist? I think it is reasonable for location
bar to set what area of the button should be clickable.

Top border overlap changed because I got new images (as result of all
discussions we had), they have 2px overlap.

On 2012/10/12 22:56:14, Peter Kasting wrote:
> No, they're not.
> 
> SetClickableHeight() shouldn't exist.  clickable_height_ shouldn't exist.  The
> top border overlap has mysteriously changed from 1 to 2, meaning either the
> previous patches or this patch position the image wrongly (I'm inclined to
> suspect this one).

[Peter Kasting, 2012-10-12 23:13:33]

On 2012/10/12 23:09:18, yefimt wrote:
> Why clickable_height_ should not exist? I think it is reasonable for location
> bar to set what area of the button should be clickable.

I spelled this out pretty clearly already, did you not see it?

In short the action box should do this itself if it needs to, we gain nothing
but more complexity and less encapsulation by exposing this outside the class.

I also thought if it was a 1 px overlap we just shouldn't bother customizing the
hit box but 2 px makes it sound like a better idea.

[yefimt, 2012-10-12 23:27:34]

Yes, I did miss it.
Done

https://chromiumcodereview.appspot.com/11095008/diff/25001/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/11095008/diff/25001/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:28: void
SetClickableHeight(int clickable_height) {
On 2012/10/12 20:57:30, Peter Kasting wrote:
> This is much worse than the code before.  The location bar should definitely
not
> know about concepts like this.
> 
> The old code was weird not because it had a bottom area at all but because for
> some reason the bottom area was declared as 2 px instead of 1 but then you
> manually added 1 back.  That math made no sense, but the abstraction was in
the
> right place.
> 
> But honestly I don't understand why we care so much about not making the top
and
> bottom pixel clickable.  The location bar (on Windows at least) already treats
> clicks on the bottom border as clicks in the location bar, why shouldn't we
just
> skip all the hittesting stuff entirely?

Done.

[Peter Kasting, 2012-10-13 00:25:29]

LGTM

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right):

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:22: // location
bar border.
Nit: Don't repeat this comment

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:28: const int
kBottomBorderOverlapThickness = 2;
Nit: Eliminate this constant and rename the other variable "kBorderOverlap"
since the top and bottom border overlaps are equal

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:29: // location
bar border.
Nit: Slightly better:

  // The action box image is transparent at the top and bottom and overlaps the
  // location bar border.
  static const int kBorderOverlap;

(See also the .cc file regarding naming)

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:30: static const
int kTopBorderOverlapThickness;
Nit: Per the Google style guide, constants go above constructor/destructor

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:40: // Overridden
from veiws::View:
Nit: veiws -> views

[yefimt, 2012-10-13 00:51:30]

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/action_box_button_view.cc (right):

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:22: // location
bar border.
On 2012/10/13 00:25:29, Peter Kasting wrote:
> Nit: Don't repeat this comment

Done.

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.cc:28: const int
kBottomBorderOverlapThickness = 2;
On 2012/10/13 00:25:29, Peter Kasting wrote:
> Nit: Eliminate this constant and rename the other variable "kBorderOverlap"
> since the top and bottom border overlaps are equal

Done.

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
File chrome/browser/ui/views/location_bar/action_box_button_view.h (right):

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:29: // location
bar border.
On 2012/10/13 00:25:29, Peter Kasting wrote:
> Nit: Slightly better:
> 
>   // The action box image is transparent at the top and bottom and overlaps
the
>   // location bar border.
>   static const int kBorderOverlap;
> 
> (See also the .cc file regarding naming)

Done.

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:30: static const
int kTopBorderOverlapThickness;
On 2012/10/13 00:25:29, Peter Kasting wrote:
> Nit: Per the Google style guide, constants go above constructor/destructor

Done.

https://chromiumcodereview.appspot.com/11095008/diff/43002/chrome/browser/ui/...
chrome/browser/ui/views/location_bar/action_box_button_view.h:40: // Overridden
from veiws::View:
On 2012/10/13 00:25:29, Peter Kasting wrote:
> Nit: veiws -> views

Done.