Mac: improve hover/clicked state appearance of bookmark bar buttons.

Mac: improve hover/clicked state appearance of bookmark bar buttons.

I more or less implemented things as per discussion with Cole and Glen. (This
does not re-create the appearance on Win Chrome, which is not exactly right
either.)

BUG=28477
TEST=Load various themes (in particular, "Karim Rashid") and make sure bookmark bar buttons look good and legible in hover and pressed states.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=32930

[Nico, 2009-11-24 02:24:11]

Screenshot?

http://codereview.chromium.org/440001/diff/3001/3002
File chrome/browser/cocoa/gradient_button_cell.mm (right):

http://codereview.chromium.org/440001/diff/3001/3002#newcode176
chrome/browser/cocoa/gradient_button_cell.mm:176: // unless clicked.
This also affects browser action buttons, wrench & page menu buttons, and
potentially download item "buttons". Did you test that / is that intended?

http://codereview.chromium.org/440001/diff/3001/3002#newcode188
chrome/browser/cocoa/gradient_button_cell.mm:188: }
This function is a mess.

http://codereview.chromium.org/440001/diff/3001/3002#newcode256
chrome/browser/cocoa/gradient_button_cell.mm:256: strokeColor = [theme
strokeColorForStyle:GTMThemeStyleToolBarButton
I like ?: in situations like these.

[viettrungluu, 2009-11-24 06:54:27]

Sent email about screenshots. Will try to clean that function up a bit more
tomorrow (well, today in other time zones).

http://codereview.chromium.org/440001/diff/3001/3002
File chrome/browser/cocoa/gradient_button_cell.mm (right):

http://codereview.chromium.org/440001/diff/3001/3002#newcode176
chrome/browser/cocoa/gradient_button_cell.mm:176: // unless clicked.
On 2009/11/24 02:24:12, Nico wrote:
> This also affects browser action buttons, wrench & page menu buttons, and
> potentially download item "buttons". Did you test that / is that intended?

It is intended and correct, according to Glen (and Cole). I had tested the
wrench and page menus, and now have tested the others as well.

http://codereview.chromium.org/440001/diff/3001/3002#newcode188
chrome/browser/cocoa/gradient_button_cell.mm:188: }
On 2009/11/24 02:24:12, Nico wrote:
> This function is a mess.

Agreed. I tried to leave it in a better state than I found it, but maybe I
failed since I also made it more complicated. I can take another pass at it....

http://codereview.chromium.org/440001/diff/3001/3002#newcode256
chrome/browser/cocoa/gradient_button_cell.mm:256: strokeColor = [theme
strokeColorForStyle:GTMThemeStyleToolBarButton
On 2009/11/24 02:24:12, Nico wrote:
> I like ?: in situations like these.

Yeah, me too, though the alignment becomes kind of suboptimal. I suppose:

  foo = condition ?
      bar.......................... :
      quux........................;

is okay formatting, so I can change that.

[Nico, 2009-11-24 07:27:04]

LG. Thanks for the screenshots and double-checking that this does what it
should.

http://codereview.chromium.org/440001/diff/3001/3002
File chrome/browser/cocoa/gradient_button_cell.mm (right):

http://codereview.chromium.org/440001/diff/3001/3002#newcode188
chrome/browser/cocoa/gradient_button_cell.mm:188: }
On 2009/11/24 06:54:28, viettrungluu wrote:
> On 2009/11/24 02:24:12, Nico wrote:
> > This function is a mess.
> 
> Agreed. I tried to leave it in a better state than I found it, but maybe I
> failed since I also made it more complicated. I can take another pass at
it....

Oh, you don't have to if you don't feel like it. I just rant a bit when doing
reviews at times. Bear with me.

[Avi (use Gerrit), 2009-11-24 14:39:53]

LG.

http://codereview.chromium.org/440001/diff/3001/3002
File chrome/browser/cocoa/gradient_button_cell.mm (right):

http://codereview.chromium.org/440001/diff/3001/3002#newcode118
chrome/browser/cocoa/gradient_button_cell.mm:118: // often interact badly with
the theme.
This comment fits better inside the function, as it's explaining the return
value rather than the function itself.

[viettrungluu, 2009-11-24 16:08:54]

Thanks, Nico and Avi.

http://codereview.chromium.org/440001/diff/3001/3002
File chrome/browser/cocoa/gradient_button_cell.mm (right):

http://codereview.chromium.org/440001/diff/3001/3002#newcode118
chrome/browser/cocoa/gradient_button_cell.mm:118: // often interact badly with
the theme.
On 2009/11/24 14:39:53, Avi wrote:
> This comment fits better inside the function, as it's explaining the return
> value rather than the function itself.

Done.

http://codereview.chromium.org/440001/diff/3001/3002#newcode188
chrome/browser/cocoa/gradient_button_cell.mm:188: }
On 2009/11/24 07:27:04, Nico wrote:
> Oh, you don't have to if you don't feel like it. I just rant a bit when doing
> reviews at times. Bear with me.

No, you're entirely right about it being a mess (that was my reaction to it!).
But since I have a number of pressing things to do, I just inserted a TODO
before this function. :-P