Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(101)

Issue 155788: Infobar UI cleanup on Mac. Adds the yellow background gradient and... (Closed)

Created:
11 years, 5 months ago by rohitrao (ping after 24h)
Modified:
9 years, 7 months ago
Reviewers:
John Grabowski
CC:
chromium-reviews_googlegroups.com, John Grabowski, Ben Goodger (Google)
Visibility:
Public.

Description

Infobar UI cleanup on Mac. Adds the yellow background gradient and centers all of the buttons. Also adds the ok/cancel buttons to the xib file. Infobars that do not need the buttons can remove them from the view before displaying. BUG=http://crbug.com/14462 BUG=http://crbug.com/17195 TEST=Infobars should have yellow background, look less ugly. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21128

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 10

Patch Set 3 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+276 lines, -161 lines) Patch
M chrome/app/nibs/InfoBar.xib View 9 chunks +148 lines, -19 lines 0 comments Download
M chrome/browser/cocoa/infobar_controller.h View 1 2 1 chunk +9 lines, -1 line 0 comments Download
M chrome/browser/cocoa/infobar_controller.mm View 1 2 5 chunks +65 lines, -52 lines 0 comments Download
A + chrome/browser/cocoa/infobar_gradient_view.h View 1 2 1 chunk +5 lines, -15 lines 0 comments Download
A + chrome/browser/cocoa/infobar_gradient_view.mm View 1 1 chunk +33 lines, -65 lines 0 comments Download
A + chrome/browser/cocoa/infobar_gradient_view_unittest.mm View 1 1 chunk +13 lines, -9 lines 0 comments Download
M chrome/chrome.gyp View 1 2 2 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 3 (0 generated)
rohitrao (ping after 24h)
11 years, 5 months ago (2009-07-20 20:51:22 UTC) #1
John Grabowski
LGTM assuming BackgroundGradientView is unmodified http://codereview.chromium.org/155788/diff/10/12 File chrome/browser/cocoa/infobar_controller.h (right): http://codereview.chromium.org/155788/diff/10/12#newcode36 Line 36: - (IBAction)dismiss:(id)sender; Perhaps ...
11 years, 5 months ago (2009-07-20 22:25:15 UTC) #2
rohitrao (ping after 24h)
11 years, 5 months ago (2009-07-20 22:32:46 UTC) #3
http://codereview.chromium.org/155788/diff/10/12
File chrome/browser/cocoa/infobar_controller.h (right):

http://codereview.chromium.org/155788/diff/10/12#newcode36
Line 36: - (IBAction)dismiss:(id)sender;
On 2009/07/20 22:25:15, John Grabowski wrote:
> Perhaps add a comment: 

Added "Called when someone clicks on the close button," to be like the comment
above.

http://codereview.chromium.org/155788/diff/10/13
File chrome/browser/cocoa/infobar_controller.mm (right):

http://codereview.chromium.org/155788/diff/10/13#newcode57
Line 57: // The default implementation does nothing.
On 2009/07/20 22:25:15, John Grabowski wrote:
> Perhaps a reasonable default is to act like dismiss:?

Eh, neither choice is really correct.  These functions exist in the base class
only so I have something to connect to in the xib file.  I could throw in a
NOTREACHED(), if you'd like that better.

http://codereview.chromium.org/155788/diff/10/15
File chrome/browser/cocoa/infobar_gradient_view.h (right):

http://codereview.chromium.org/155788/diff/10/15#newcode10
Line 10: // A custom view that draws a 'standard' background gradient.
On 2009/07/20 22:25:15, John Grabowski wrote:
> 'standard' background gradient for info bars.
> 
> 

Done.

http://codereview.chromium.org/155788/diff/10/15#newcode12
Line 12: @interface InfoBarGradientView : NSView {
On 2009/07/20 22:25:15, John Grabowski wrote:
> Once your CL lands, we'll still have
> background_gradient_view.h, yes?

Yeah, I did an "svn cp."  This file was similar enough to BGV that I felt a copy
was warranted, but different enough that it didn't seem worth merging the two
classes.

http://codereview.chromium.org/155788/diff/10/16
File chrome/browser/cocoa/infobar_gradient_view_unittest.mm (right):

http://codereview.chromium.org/155788/diff/10/16#newcode35
Line 35: TEST_F(InfoBarGradientViewTest, Display) {
On 2009/07/20 22:25:15, John Grabowski wrote:
> Excellent

I aim to please.

Powered by Google App Engine
This is Rietveld 408576698