Change mac notifications UI to match the new mocks. Introduce a new utility ...

Change mac notifications UI to match the new mocks.  Introduce a new utility view which provides an image based button that changes on hover and press.

Refer to the bug for screenshots of the current and desired UI.
 
Screenshot of what results from this patch is visible at http://www.corp.google.com/~johnnyg/49490patch.png

XIB change: change height of title bar, change bindings of buttons.

BUG=49190
TEST=notifications on the mac


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

[Nico, 2010-07-15 22:07:41]

Land the new png file in its own CL, else you can't use the trybots for this
one. (grd change stays in this cl)

[John Gregg, 2010-07-15 22:10:26]

On 2010/07/15 22:07:41, Nico wrote:
> Land the new png file in its own CL, else you can't use the trybots for this
> one. (grd change stays in this cl)

okay done with new patch

[Nico, 2010-07-15 22:12:13]

A few nits below.

In general, we try to keep the mac UI resolution independent. Can you use pdfs
instead of pngs for that icon? Can you maybe even use the pdf image used for the
toolbar's wrench?

The new class needs a unit test.

http://codereview.chromium.org/3014004/diff/1/8
File chrome/browser/cocoa/hover_image_button.mm (right):

http://codereview.chromium.org/3014004/diff/1/8#newcode82
chrome/browser/cocoa/hover_image_button.mm:82: scoped_nsobject<HoverImageButton>
myself([self retain]);
Wrong type

http://codereview.chromium.org/3014004/diff/1/10
File chrome/browser/cocoa/notifications/balloon_controller.mm (right):

http://codereview.chromium.org/3014004/diff/1/10#newcode77
chrome/browser/cocoa/notifications/balloon_controller.mm:77: [closeButton_
setFrame:NSMakeRect(6, 1, frame.size.width, frame.size.height)];
NSWidth(frame), same for height

[John Gregg, 2010-07-27 18:55:27]

New patch posted which uses a PDF and contains a unit test.

[Nico, 2010-07-27 20:13:39]

As said before, land the pdf in a separate cl

http://codereview.chromium.org/3014004/diff/18001/19004
File chrome/browser/cocoa/hover_image_button.mm (right):

http://codereview.chromium.org/3014004/diff/18001/19004#newcode34
chrome/browser/cocoa/hover_image_button.mm:34: [self commonInit];
you probably want to override initWithCoder: instead

http://codereview.chromium.org/3014004/diff/18001/19004#newcode56
chrome/browser/cocoa/hover_image_button.mm:56: [[self cell]
indent 1

http://codereview.chromium.org/3014004/diff/18001/19004#newcode62
chrome/browser/cocoa/hover_image_button.mm:62: [self setTrackingEnabled:NO];
is this necessary?

http://codereview.chromium.org/3014004/diff/18001/19005
File chrome/browser/cocoa/hover_image_button_unittest.mm (right):

http://codereview.chromium.org/3014004/diff/18001/19005#newcode22
chrome/browser/cocoa/hover_image_button_unittest.mm:22:
initWithFrame:content_frame]);
indent

http://codereview.chromium.org/3014004/diff/18001/19005#newcode50
chrome/browser/cocoa/hover_image_button_unittest.mm:50: [button_
setDefaultImage:rb.GetNSImageNamed(IDR_HOME)];
do you need to set the images in this test?

http://codereview.chromium.org/3014004/diff/18001/19007
File chrome/browser/cocoa/notifications/balloon_controller.mm (right):

http://codereview.chromium.org/3014004/diff/18001/19007#newcode149
chrome/browser/cocoa/notifications/balloon_controller.mm:149: int y =
balloon_->GetPosition().y() - 0.5;
roundf()? floor()?

http://codereview.chromium.org/3014004/diff/18001/19008
File chrome/browser/cocoa/notifications/balloon_view.mm (right):

http://codereview.chromium.org/3014004/diff/18001/19008#newcode37
chrome/browser/cocoa/notifications/balloon_view.mm:37: return NO;
that's the default for borderless windows. is this needed?

can't notifications contain html? seems like input elements in notificatiosn
won't work?

[John Gregg, 2010-07-27 21:01:46]

http://codereview.chromium.org/3014004/diff/18001/19004
File chrome/browser/cocoa/hover_image_button.mm (right):

http://codereview.chromium.org/3014004/diff/18001/19004#newcode34
chrome/browser/cocoa/hover_image_button.mm:34: [self commonInit];
On 2010/07/27 20:13:39, Nico wrote:
> you probably want to override initWithCoder: instead

Why?  I'm following pattern of hover_close_button.mm here.

http://codereview.chromium.org/3014004/diff/18001/19004#newcode56
chrome/browser/cocoa/hover_image_button.mm:56: [[self cell]
On 2010/07/27 20:13:39, Nico wrote:
> indent 1

Done.

http://codereview.chromium.org/3014004/diff/18001/19004#newcode62
chrome/browser/cocoa/hover_image_button.mm:62: [self setTrackingEnabled:NO];
On 2010/07/27 20:13:39, Nico wrote:
> is this necessary?

I think it is.  Again following the pattern of hover_close_button.

http://codereview.chromium.org/3014004/diff/18001/19005
File chrome/browser/cocoa/hover_image_button_unittest.mm (right):

http://codereview.chromium.org/3014004/diff/18001/19005#newcode22
chrome/browser/cocoa/hover_image_button_unittest.mm:22:
initWithFrame:content_frame]);
On 2010/07/27 20:13:39, Nico wrote:
> indent

Done.

http://codereview.chromium.org/3014004/diff/18001/19005#newcode50
chrome/browser/cocoa/hover_image_button_unittest.mm:50: [button_
setDefaultImage:rb.GetNSImageNamed(IDR_HOME)];
On 2010/07/27 20:13:39, Nico wrote:
> do you need to set the images in this test?

No, it's fine without.  Removed.

http://codereview.chromium.org/3014004/diff/18001/19007
File chrome/browser/cocoa/notifications/balloon_controller.mm (right):

http://codereview.chromium.org/3014004/diff/18001/19007#newcode149
chrome/browser/cocoa/notifications/balloon_controller.mm:149: int y =
balloon_->GetPosition().y() - 0.5;
On 2010/07/27 20:13:39, Nico wrote:
> roundf()? floor()?

The 0.5 is no longer necessary, sorry.

http://codereview.chromium.org/3014004/diff/18001/19008
File chrome/browser/cocoa/notifications/balloon_view.mm (right):

http://codereview.chromium.org/3014004/diff/18001/19008#newcode37
chrome/browser/cocoa/notifications/balloon_view.mm:37: return NO;
On 2010/07/27 20:13:39, Nico wrote:
> that's the default for borderless windows. is this needed?
> 
> can't notifications contain html? seems like input elements in notificatiosn
> won't work?

I removed it.  It's true that input elements don't work, but that's been the
case for a while and is tracked as a separate issue (http://crbug.com/34521)

[John Gregg, 2010-07-28 21:56:36]

ping on the latest CL?  Thanks

[Nico, 2010-07-28 22:04:07]

Sorry, missed this

On Tue, Jul 27, 2010 at 2:01 PM,  <[email protected]> wrote:
>
> http://codereview.chromium.org/3014004/diff/18001/19004
> File chrome/browser/cocoa/hover_image_button.mm (right):
>
> http://codereview.chromium.org/3014004/diff/18001/19004#newcode34
> chrome/browser/cocoa/hover_image_button.mm:34: [self commonInit];
> On 2010/07/27 20:13:39, Nico wrote:
>>
>> you probably want to override initWithCoder: instead
>
> Why?  I'm following pattern of hover_close_button.mm here.
>
> http://codereview.chromium.org/3014004/diff/18001/19004#newcode56
> chrome/browser/cocoa/hover_image_button.mm:56: [[self cell]
> On 2010/07/27 20:13:39, Nico wrote:
>>
>> indent 1
>
> Done.
>
> http://codereview.chromium.org/3014004/diff/18001/19004#newcode62
> chrome/browser/cocoa/hover_image_button.mm:62: [self
> setTrackingEnabled:NO];
> On 2010/07/27 20:13:39, Nico wrote:
>>
>> is this necessary?
>
> I think it is.  Again following the pattern of hover_close_button.

Oh, I see. Can you pull out the common code between the two classes
(the hover stuff) into a shared superclass?

>
> http://codereview.chromium.org/3014004/diff/18001/19005
> File chrome/browser/cocoa/hover_image_button_unittest.mm (right):
>
> http://codereview.chromium.org/3014004/diff/18001/19005#newcode22
> chrome/browser/cocoa/hover_image_button_unittest.mm:22:
> initWithFrame:content_frame]);
> On 2010/07/27 20:13:39, Nico wrote:
>>
>> indent
>
> Done.
>
> http://codereview.chromium.org/3014004/diff/18001/19005#newcode50
> chrome/browser/cocoa/hover_image_button_unittest.mm:50: [button_
> setDefaultImage:rb.GetNSImageNamed(IDR_HOME)];
> On 2010/07/27 20:13:39, Nico wrote:
>>
>> do you need to set the images in this test?
>
> No, it's fine without.  Removed.
>
> http://codereview.chromium.org/3014004/diff/18001/19007
> File chrome/browser/cocoa/notifications/balloon_controller.mm (right):
>
> http://codereview.chromium.org/3014004/diff/18001/19007#newcode149
> chrome/browser/cocoa/notifications/balloon_controller.mm:149: int y =
> balloon_->GetPosition().y() - 0.5;
> On 2010/07/27 20:13:39, Nico wrote:
>>
>> roundf()? floor()?
>
> The 0.5 is no longer necessary, sorry.
>
> http://codereview.chromium.org/3014004/diff/18001/19008
> File chrome/browser/cocoa/notifications/balloon_view.mm (right):
>
> http://codereview.chromium.org/3014004/diff/18001/19008#newcode37
> chrome/browser/cocoa/notifications/balloon_view.mm:37: return NO;
> On 2010/07/27 20:13:39, Nico wrote:
>>
>> that's the default for borderless windows. is this needed?
>
>> can't notifications contain html? seems like input elements in
>
> notificatiosn
>>
>> won't work?
>
> I removed it.  It's true that input elements don't work, but that's been
> the case for a while and is tracked as a separate issue
> (http://crbug.com/34521)
>
> http://codereview.chromium.org/3014004/show
>

[John Gregg, 2010-07-29 00:03:50]

> Oh, I see. Can you pull out the common code between the two classes
> (the hover stuff) into a shared superclass?
> 
Okay, I did that and reposted.

[Nico, 2010-07-29 16:12:59]

lg

http://codereview.chromium.org/3014004/diff/35001/36007
File chrome/browser/cocoa/hover_image_button.mm (right):

http://codereview.chromium.org/3014004/diff/35001/36007#newcode13
chrome/browser/cocoa/hover_image_button.mm:13: -
(void)setTrackingEnabled:(BOOL)enabled;
remvoe

http://codereview.chromium.org/3014004/diff/35001/36007#newcode17
chrome/browser/cocoa/hover_image_button.mm:17: - (void)commonInit;
reove

http://codereview.chromium.org/3014004/diff/35001/36007#newcode21
chrome/browser/cocoa/hover_image_button.mm:21: - (void)checkImageState;
remvoe

[TVL, 2010-08-09 20:08:54]

http://codereview.chromium.org/3014004/diff/35001/36013
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/3014004/diff/35001/36013#newcode3118
chrome/chrome_browser.gypi:3118: 'app/nibs/Notification.xib',
This doesn't appear to have any localized resources in it and is triggering a
warning in the build.  Is it supposed to have localized resources?