Managed preferences warning banner for Mac preferences dialog

Managed preferences warning banner for Mac preferences dialog

Add a new view to the preferences dialog in Preferences.xib that holds the
alert image and the warning message label. Update the visibility of the warning
banner when switching preference views in PreferencesWindowController.

BUG=43423
TEST=none

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

[Mattias Nissler (ping if slow), 2010-06-15 17:02:47]

Mike, would you mind reviewing this? And please bear with me, this is my first
substantial cocoa CL ;)

[Avi (use Gerrit), 2010-06-15 17:05:55]

The bug number is most certainly wrong; please double-check it.

[Mattias Nissler (ping if slow), 2010-06-15 17:17:49]

On 2010/06/15 17:05:55, Avi wrote:
> The bug number is most certainly wrong; please double-check it.

Fixed, sorry about that.

[pink (ping after 24hrs), 2010-06-15 18:53:08]

http://codereview.chromium.org/2831004/diff/1/4
File chrome/browser/cocoa/managed_prefs_banner_gradient_view.mm (right):

http://codereview.chromium.org/2831004/diff/1/4#newcode27
chrome/browser/cocoa/managed_prefs_banner_gradient_view.mm:27: NSGradient*
gradient =
this is leaked. Use scoped_nsobject.

http://codereview.chromium.org/2831004/diff/1/6
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/2831004/diff/1/6#newcode1883
chrome/browser/cocoa/preferences_window_controller.mm:1883: oldPrefsView,
NSViewAnimationTargetKey,
continuations should be indented 4 additional chars

http://codereview.chromium.org/2831004/diff/1/6#newcode1888
chrome/browser/cocoa/preferences_window_controller.mm:1888: prefsView,
NSViewAnimationTargetKey,
same here, several other times in file.

http://codereview.chromium.org/2831004/diff/1/6#newcode1933
chrome/browser/cocoa/preferences_window_controller.mm:1933: NSMutableArray*
subviews = [NSMutableArray arrayWithCapacity:2];
why not just NSArray arrayWithObjects:...?

http://codereview.chromium.org/2831004/diff/1/6#newcode1951
chrome/browser/cocoa/preferences_window_controller.mm:1951: -
(void)initBannerStateForPage:(OptionsPage) page {
comment?

http://codereview.chromium.org/2831004/diff/1/6#newcode1951
chrome/browser/cocoa/preferences_window_controller.mm:1951: -
(void)initBannerStateForPage:(OptionsPage) page {
no space after closing paren

[Mattias Nissler (ping if slow), 2010-06-16 11:16:04]

Thanks for your feedback, Mike. Here's an updated version.

http://codereview.chromium.org/2831004/diff/1/4
File chrome/browser/cocoa/managed_prefs_banner_gradient_view.mm (right):

http://codereview.chromium.org/2831004/diff/1/4#newcode27
chrome/browser/cocoa/managed_prefs_banner_gradient_view.mm:27: NSGradient*
gradient =
On 2010/06/15 18:53:08, pink wrote:
> this is leaked. Use scoped_nsobject.

Done.

http://codereview.chromium.org/2831004/diff/1/6
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/2831004/diff/1/6#newcode1883
chrome/browser/cocoa/preferences_window_controller.mm:1883: oldPrefsView,
NSViewAnimationTargetKey,
On 2010/06/15 18:53:08, pink wrote:
> continuations should be indented 4 additional chars

Ah, I was just imitating the style I found :-)
Done.

http://codereview.chromium.org/2831004/diff/1/6#newcode1888
chrome/browser/cocoa/preferences_window_controller.mm:1888: prefsView,
NSViewAnimationTargetKey,
On 2010/06/15 18:53:08, pink wrote:
> same here, several other times in file.

Done.

http://codereview.chromium.org/2831004/diff/1/6#newcode1933
chrome/browser/cocoa/preferences_window_controller.mm:1933: NSMutableArray*
subviews = [NSMutableArray arrayWithCapacity:2];
On 2010/06/15 18:53:08, pink wrote:
> why not just NSArray arrayWithObjects:...?

Done.

http://codereview.chromium.org/2831004/diff/1/6#newcode1951
chrome/browser/cocoa/preferences_window_controller.mm:1951: -
(void)initBannerStateForPage:(OptionsPage) page {
On 2010/06/15 18:53:08, pink wrote:
> no space after closing paren

Done.

http://codereview.chromium.org/2831004/diff/1/6#newcode1951
chrome/browser/cocoa/preferences_window_controller.mm:1951: -
(void)initBannerStateForPage:(OptionsPage) page {
On 2010/06/15 18:53:08, pink wrote:
> comment?

Done.

[pink (ping after 24hrs), 2010-06-16 14:55:31]

http://codereview.chromium.org/2831004/diff/1007/9003
File chrome/browser/cocoa/managed_prefs_banner_gradient_view.mm (right):

http://codereview.chromium.org/2831004/diff/1007/9003#newcode15
chrome/browser/cocoa/managed_prefs_banner_gradient_view.mm:15: @implementation
ManagedPrefsBannerGradientView
This seems like it could be made into a very generic class with only a small bit
of extra work. I think the codebase could benefit from having a reusable view
that draws a gradient like this. There's really nothing specific to Prefs, or
managed prefs, here besides the colors used, and it seems very un-cocoay to have
such a specific view class just to draw some different colors.

How about just refactoring it to "BannerGradientView"? That said, take a look at
BackgroundGradientView. Is there a way to combine these classes somehow? We
already use BGV in many places in the codebase, though it may not be applicable
here because it's themeable. The implementations are very similar.

http://codereview.chromium.org/2831004/diff/1007/9004
File chrome/browser/cocoa/preferences_window_controller.h (right):

http://codereview.chromium.org/2831004/diff/1007/9004#newcode47
chrome/browser/cocoa/preferences_window_controller.h:47: NSView*
currentPrefsView_;
this appears to be a weak reference. It needs to be commented as such per the
objc coding guidelines since it's not an IBOutlet.

http://codereview.chromium.org/2831004/diff/1007/9005
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/2831004/diff/1007/9005#newcode1922
chrome/browser/cocoa/preferences_window_controller.mm:1922: static const
wchar_t* kManagedPrefsGeneral[] = {
does this need to be at the file level? can it be inside initBannerStateForPage,
or if nothing else, in a anonymous namespace?

[Mattias Nissler (ping if slow), 2010-06-18 11:01:20]

Thanks Mike for your comments, here's the next iteration.

http://codereview.chromium.org/2831004/diff/1007/9003
File chrome/browser/cocoa/managed_prefs_banner_gradient_view.mm (right):

http://codereview.chromium.org/2831004/diff/1007/9003#newcode15
chrome/browser/cocoa/managed_prefs_banner_gradient_view.mm:15: @implementation
ManagedPrefsBannerGradientView
On 2010/06/16 14:55:31, pink wrote:
> This seems like it could be made into a very generic class with only a small
bit
> of extra work. I think the codebase could benefit from having a reusable view
> that draws a gradient like this. There's really nothing specific to Prefs, or
> managed prefs, here besides the colors used, and it seems very un-cocoay to
have
> such a specific view class just to draw some different colors.

Agreed.

> 
> How about just refactoring it to "BannerGradientView"? That said, take a look
at
> BackgroundGradientView. Is there a way to combine these classes somehow? We
> already use BGV in many places in the codebase, though it may not be
applicable
> here because it's themeable. The implementations are very similar.

The theming bits in BGV actually were my reason to roll my own. Note that BGV
also uses offsets for the starting and ending points of the gradient, so I think
merging the two would actually overcomplicate things. However, there is also
InfoBarGradientView, which is really similar to ManagedPrefsBannerGradientView.
I've now converted ManagedPrefsBannerGradientView into a generic
VerticalGradientView, and changed InfoBarGradientView to extend the former so we
can share the drawing code.

http://codereview.chromium.org/2831004/diff/1007/9004
File chrome/browser/cocoa/preferences_window_controller.h (right):

http://codereview.chromium.org/2831004/diff/1007/9004#newcode47
chrome/browser/cocoa/preferences_window_controller.h:47: NSView*
currentPrefsView_;
On 2010/06/16 14:55:31, pink wrote:
> this appears to be a weak reference. It needs to be commented as such per the
> objc coding guidelines since it's not an IBOutlet.

Done.

http://codereview.chromium.org/2831004/diff/1007/9005
File chrome/browser/cocoa/preferences_window_controller.mm (right):

http://codereview.chromium.org/2831004/diff/1007/9005#newcode1922
chrome/browser/cocoa/preferences_window_controller.mm:1922: static const
wchar_t* kManagedPrefsGeneral[] = {
On 2010/06/16 14:55:31, pink wrote:
> does this need to be at the file level? can it be inside
initBannerStateForPage,
> or if nothing else, in a anonymous namespace?

I didn't like this code either, to me it feels just to complicated to achieve a
simple goal of instantiating the state tracker object. What I've done know is
created a factory method within ManagedPrefsBannerState to take care of the
initialization. I think I like that approach a little better. WDYT?

[pink (ping after 24hrs), 2010-06-18 19:46:56]

http://codereview.chromium.org/2831004/diff/15001/16004
File chrome/browser/cocoa/preferences_window_controller.h (right):

http://codereview.chromium.org/2831004/diff/15001/16004#newcode9
chrome/browser/cocoa/preferences_window_controller.h:9: #import
"chrome/browser/cocoa/vertical_gradient_view.h"
i don't think you need this include here, you can probably just do an @class
forward decl.

http://codereview.chromium.org/2831004/diff/15001/16006
File chrome/browser/cocoa/vertical_gradient_view.h (right):

http://codereview.chromium.org/2831004/diff/15001/16006#newcode18
chrome/browser/cocoa/vertical_gradient_view.h:18: NSGradient* gradient_;
if this is a strong ref, why not use scoped_nsobject?

http://codereview.chromium.org/2831004/diff/15001/16006#newcode20
chrome/browser/cocoa/vertical_gradient_view.h:20: NSColor* strokeColor_;
ditto

http://codereview.chromium.org/2831004/diff/15001/16007
File chrome/browser/cocoa/vertical_gradient_view.mm (right):

http://codereview.chromium.org/2831004/diff/15001/16007#newcode9
chrome/browser/cocoa/vertical_gradient_view.mm:9: - (void)dealloc {
if you used scoped_nsobjects, the dealloc wouldn't be necessary at all.

http://codereview.chromium.org/2831004/diff/15001/16007#newcode10
chrome/browser/cocoa/vertical_gradient_view.mm:10: if (gradient_)
checking for nil not necessary before calling object methods in obj-c. see the
style guide.

http://codereview.chromium.org/2831004/diff/15001/16007#newcode21
chrome/browser/cocoa/vertical_gradient_view.mm:21: -
(void)setGradient:(NSGradient*)gradient {
check the style guide for the general pattern of setters.

http://codereview.chromium.org/2831004/diff/15001/16007#newcode23
chrome/browser/cocoa/vertical_gradient_view.mm:23: if (gradient_)
no need to check for nil

http://codereview.chromium.org/2831004/diff/15001/16007#newcode42
chrome/browser/cocoa/vertical_gradient_view.mm:42: if (gradient)
nil check not needed. Why not just do:

[[self gradient] drawInRect....];

http://codereview.chromium.org/2831004/diff/15001/16007#newcode46
chrome/browser/cocoa/vertical_gradient_view.mm:46: NSColor* strokeColor = [self
strokeColor];
fix indenting.

http://codereview.chromium.org/2831004/diff/15001/16008
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/2831004/diff/15001/16008#newcode905
chrome/chrome_browser.gypi:905: 'browser/cocoa/vertical_gradient_view.mm',
can you add a simple unit test for vertical_gradient_view that just instantiates
the view into a window and removes it? We do this with a bunch of our other
views to make sure drawing basic doesn't crash or leak under valgrind.

[Mattias Nissler (ping if slow), 2010-06-21 18:42:28]

Mike, sorry for all the poor style in the last incarnation. Here's an updated
version.

http://codereview.chromium.org/2831004/diff/15001/16004
File chrome/browser/cocoa/preferences_window_controller.h (right):

http://codereview.chromium.org/2831004/diff/15001/16004#newcode9
chrome/browser/cocoa/preferences_window_controller.h:9: #import
"chrome/browser/cocoa/vertical_gradient_view.h"
On 2010/06/18 19:46:56, pink wrote:
> i don't think you need this include here, you can probably just do an @class
> forward decl.

Done.

http://codereview.chromium.org/2831004/diff/15001/16006
File chrome/browser/cocoa/vertical_gradient_view.h (right):

http://codereview.chromium.org/2831004/diff/15001/16006#newcode18
chrome/browser/cocoa/vertical_gradient_view.h:18: NSGradient* gradient_;
On 2010/06/18 19:46:56, pink wrote:
> if this is a strong ref, why not use scoped_nsobject?

Done.

http://codereview.chromium.org/2831004/diff/15001/16006#newcode20
chrome/browser/cocoa/vertical_gradient_view.h:20: NSColor* strokeColor_;
On 2010/06/18 19:46:56, pink wrote:
> ditto

Done.

http://codereview.chromium.org/2831004/diff/15001/16007
File chrome/browser/cocoa/vertical_gradient_view.mm (right):

http://codereview.chromium.org/2831004/diff/15001/16007#newcode9
chrome/browser/cocoa/vertical_gradient_view.mm:9: - (void)dealloc {
On 2010/06/18 19:46:56, pink wrote:
> if you used scoped_nsobjects, the dealloc wouldn't be necessary at all.

Done.

http://codereview.chromium.org/2831004/diff/15001/16007#newcode10
chrome/browser/cocoa/vertical_gradient_view.mm:10: if (gradient_)
On 2010/06/18 19:46:56, pink wrote:
> checking for nil not necessary before calling object methods in obj-c. see the
> style guide.

Done.

http://codereview.chromium.org/2831004/diff/15001/16007#newcode21
chrome/browser/cocoa/vertical_gradient_view.mm:21: -
(void)setGradient:(NSGradient*)gradient {
On 2010/06/18 19:46:56, pink wrote:
> check the style guide for the general pattern of setters.

Done.

http://codereview.chromium.org/2831004/diff/15001/16007#newcode23
chrome/browser/cocoa/vertical_gradient_view.mm:23: if (gradient_)
On 2010/06/18 19:46:56, pink wrote:
> no need to check for nil

Done.

http://codereview.chromium.org/2831004/diff/15001/16007#newcode42
chrome/browser/cocoa/vertical_gradient_view.mm:42: if (gradient)
On 2010/06/18 19:46:56, pink wrote:
> nil check not needed. Why not just do:
> 
> [[self gradient] drawInRect....];

Done.

http://codereview.chromium.org/2831004/diff/15001/16007#newcode46
chrome/browser/cocoa/vertical_gradient_view.mm:46: NSColor* strokeColor = [self
strokeColor];
On 2010/06/18 19:46:56, pink wrote:
> fix indenting.

Done.

http://codereview.chromium.org/2831004/diff/15001/16008
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/2831004/diff/15001/16008#newcode905
chrome/chrome_browser.gypi:905: 'browser/cocoa/vertical_gradient_view.mm',
On 2010/06/18 19:46:56, pink wrote:
> can you add a simple unit test for vertical_gradient_view that just
instantiates
> the view into a window and removes it? We do this with a bunch of our other
> views to make sure drawing basic doesn't crash or leak under valgrind.

Done.

[pink (ping after 24hrs), 2010-06-21 18:51:59]

lgtm. thanks!