Add support for dark theme in Mac Notification Center

Add support for dark theme in Mac Notification Center

This updates the SDK forward declarations and adds dark resources.

R=rsesek@chromium.org
BUG=402787
Committed: https://crrev.com/81d046cf0412b39ee6ac88e4bae10ab7768df5ac
Cr-Commit-Position: refs/heads/master@{#296766}

[dewittj, 2014-09-11 20:31:18]

Robert: Here is a first pass.  Still doesn't build because I don't know how to
declare that NSView supports the NSAppearanceCustomization protocol.  Any
pointers?

[Robert Sesek, 2014-09-11 20:50:00]

https://codereview.chromium.org/564953002/diff/1/base/mac/sdk_forward_declara...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/564953002/diff/1/base/mac/sdk_forward_declara...
base/mac/sdk_forward_declarations.h:260: @interface NSAppearance : NSObject
You won't be able to do this, unfortunately. What I'd recommend doing is this:

// NSAppearance is a new class in the 10.9 SDK. New classes cannot be
forward-declared because
// they also require an @implementation, which would produce conflicting
linkage. Instead, just
// declare the necessary pieces of the interface as a protocol, and treat
objects of this type as id.
@protocol CrNSAppearance
- (NSString*)name;
@end

... but do that above line 247 so that you can do:

@interface NSView (MavericksSDK)
- (id<CrNSAppearance>)effectiveAppearance;
@end

https://codereview.chromium.org/564953002/diff/1/ui/message_center/cocoa/stat...
File ui/message_center/cocoa/status_item_view.mm (right):

https://codereview.chromium.org/564953002/diff/1/ui/message_center/cocoa/stat...
ui/message_center/cocoa/status_item_view.mm:157: dark = (appearance != nil) &&
(appearance == darkAppearance);
The appearance from -effectiveAppearance will actually not be a pointer match.
You should do [[appearance name] isEqualToString:NSAppearanceNameVibrantDark]
instead.

[Robert Sesek, 2014-09-11 20:50:26]

On 2014/09/11 20:31:18, dewittj wrote:
> Robert: Here is a first pass.  Still doesn't build because I don't know how to
> declare that NSView supports the NSAppearanceCustomization protocol.  Any
> pointers?

See the in-file comment, but if you follow the approach I outlined, I think you
won't need to mark NSView as conforming to that protocol.

[dewittj, 2014-09-12 17:48:47]

Thanks for the pointers.  One more question:  I don't have the appropriate
Appearance.h; did I get the string value right in sdk_forward_declarations.mm?

[dewittj, 2014-09-12 18:02:11]

also, in
https://developer.apple.com/library/mac/documentation/AppKit/Reference/NSAppe...

I don't see the property "name" on the NSAppearance class.  Where is that
generated?

[dewittj, 2014-09-12 18:15:36]

Addressed your comments, please take a look.  I did not replace the "pressed"
graphic since we don't have a resource yet.  There is a chance it looks right
anyway since it's just an alpha mask; can you check?

https://codereview.chromium.org/564953002/diff/20001/ui/message_center/cocoa/...
File ui/message_center/cocoa/status_item_view.mm (right):

https://codereview.chromium.org/564953002/diff/20001/ui/message_center/cocoa/...
ui/message_center/cocoa/status_item_view.mm:159: int kResourceIds[2][2][2][2] =
{
Wondering now if I should use a bitmask to index into an array of resource
names.  WDYT?

[dewittj, 2014-09-12 18:16:23]

https://codereview.chromium.org/564953002/diff/1/base/mac/sdk_forward_declara...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/564953002/diff/1/base/mac/sdk_forward_declara...
base/mac/sdk_forward_declarations.h:260: @interface NSAppearance : NSObject
On 2014/09/11 20:50:00, rsesek wrote:
> You won't be able to do this, unfortunately. What I'd recommend doing is this:
> 
> // NSAppearance is a new class in the 10.9 SDK. New classes cannot be
> forward-declared because
> // they also require an @implementation, which would produce conflicting
> linkage. Instead, just
> // declare the necessary pieces of the interface as a protocol, and treat
> objects of this type as id.
> @protocol CrNSAppearance
> - (NSString*)name;
> @end
> 
> ... but do that above line 247 so that you can do:
> 
> @interface NSView (MavericksSDK)
> - (id<CrNSAppearance>)effectiveAppearance;
> @end

Done.

https://codereview.chromium.org/564953002/diff/1/ui/message_center/cocoa/stat...
File ui/message_center/cocoa/status_item_view.mm (right):

https://codereview.chromium.org/564953002/diff/1/ui/message_center/cocoa/stat...
ui/message_center/cocoa/status_item_view.mm:157: dark = (appearance != nil) &&
(appearance == darkAppearance);
On 2014/09/11 20:50:00, rsesek wrote:
> The appearance from -effectiveAppearance will actually not be a pointer match.
> You should do [[appearance name] isEqualToString:NSAppearanceNameVibrantDark]
> instead.

Done.

[Robert Sesek, 2014-09-12 19:16:24]

On 2014/09/12 18:15:36, dewittj wrote:
> Addressed your comments, please take a look.  I did not replace the "pressed"
> graphic since we don't have a resource yet.  There is a chance it looks right
> anyway since it's just an alpha mask; can you check?

Replied to you on the bug.

https://codereview.chromium.org/564953002/diff/20001/base/mac/sdk_forward_dec...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/564953002/diff/20001/base/mac/sdk_forward_dec...
base/mac/sdk_forward_declarations.h:252: - (NSString*)name;
-name is a new property in 10.10, which is why it doesn't show up in that link.
However I tested a different way to do this, which I think is the best way to go
(sorry for the change in recommendation!). Remove -name and replace it with:

+ (id<NSObject>)appearanceNamed:(NSString*)name;

Then you'd use this by doing:

Class<CrNSAppearance> nsAppearanceClass = NSClassFromString(@"NSAppearance");
if ([nsAppearanceClass respondsToSelector:@selector(appearanceNamed:)]) {
  id<NSObject> darkAppearance = [nsAppearanceClass
appearanceNamed:NSAppearanceNameVibrantDark];
  if ([[self effectiveAppearance] isEqual:darkAppearance]) {
    // in dark mode
  }
}

https://codereview.chromium.org/564953002/diff/20001/ui/message_center/cocoa/...
File ui/message_center/cocoa/status_item_view.mm (right):

https://codereview.chromium.org/564953002/diff/20001/ui/message_center/cocoa/...
ui/message_center/cocoa/status_item_view.mm:155: dark = (appearanceName != nil)
&&
You can drop the nil check. Messaging nil will always return a 0 value.

https://codereview.chromium.org/564953002/diff/20001/ui/message_center/cocoa/...
ui/message_center/cocoa/status_item_view.mm:159: int kResourceIds[2][2][2][2] =
{
On 2014/09/12 18:15:36, dewittj wrote:
> Wondering now if I should use a bitmask to index into an array of resource
> names.  WDYT?

I'd be fine with either. Your call.

[dewittj, 2014-09-12 23:24:21]

oshima@chromium.org: Please review changes in ui/resources

thakis@chromium.org: Please review changes in base/mac

rsesek: I adopted your changes but also added an additional respondsToSelector
to be super safe.  LMK what you think.  Also I don't have access to my Mac since
I'm WFH this afternoon, so I was unable to compile my latest changes :-)

https://codereview.chromium.org/564953002/diff/20001/base/mac/sdk_forward_dec...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/564953002/diff/20001/base/mac/sdk_forward_dec...
base/mac/sdk_forward_declarations.h:252: - (NSString*)name;
On 2014/09/12 19:16:23, rsesek wrote:
> -name is a new property in 10.10, which is why it doesn't show up in that
link.
> However I tested a different way to do this, which I think is the best way to
go
> (sorry for the change in recommendation!). Remove -name and replace it with:
> 
> + (id<NSObject>)appearanceNamed:(NSString*)name;
> 
> Then you'd use this by doing:
> 
> Class<CrNSAppearance> nsAppearanceClass = NSClassFromString(@"NSAppearance");
> if ([nsAppearanceClass respondsToSelector:@selector(appearanceNamed:)]) {
>   id<NSObject> darkAppearance = [nsAppearanceClass
> appearanceNamed:NSAppearanceNameVibrantDark];
>   if ([[self effectiveAppearance] isEqual:darkAppearance]) {
>     // in dark mode
>   }
> }

Done.

https://codereview.chromium.org/564953002/diff/20001/ui/message_center/cocoa/...
File ui/message_center/cocoa/status_item_view.mm (right):

https://codereview.chromium.org/564953002/diff/20001/ui/message_center/cocoa/...
ui/message_center/cocoa/status_item_view.mm:155: dark = (appearanceName != nil)
&&
On 2014/09/12 19:16:23, rsesek wrote:
> You can drop the nil check. Messaging nil will always return a 0 value.

Done.

https://codereview.chromium.org/564953002/diff/20001/ui/message_center/cocoa/...
ui/message_center/cocoa/status_item_view.mm:159: int kResourceIds[2][2][2][2] =
{
On 2014/09/12 19:16:23, rsesek wrote:
> On 2014/09/12 18:15:36, dewittj wrote:
> > Wondering now if I should use a bitmask to index into an array of resource
> > names.  WDYT?
> 
> I'd be fine with either. Your call.

Acknowledged.

[dewittj, 2014-09-12 23:24:41]

dewittj@chromium.org changed reviewers:
+ oshima@chromium.org, thakis@chomium.org

[dewittj, 2014-09-12 23:24:46]

oshima@chromium.org: Please review changes in ui/resources

thakis@chromium.org: Please review changes in base/mac

[oshima, 2014-09-13 00:42:48]

https://codereview.chromium.org/564953002/diff/60001/ui/resources/ui_resource...
File ui/resources/ui_resources.grd (left):

https://codereview.chromium.org/564953002/diff/60001/ui/resources/ui_resource...
ui/resources/ui_resources.grd:29: <structure type="chrome_scaled_image"
name="IDR_APP_LIST_SEARCH_ICON" file="common/app_list_search_icon.png" />
is this intentional?

[Robert Sesek, 2014-09-15 16:36:01]

rsesek@chromium.org changed reviewers:
- thakis@chomium.org

[Robert Sesek, 2014-09-15 16:36:02]

-thakis since I should be able to approve base/mac/ changes

https://codereview.chromium.org/564953002/diff/60001/base/mac/sdk_forward_dec...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/564953002/diff/60001/base/mac/sdk_forward_dec...
base/mac/sdk_forward_declarations.h:270: @protocol CrNSAppearance
To resolve the compile errors, add " <NSObject>" at the end of this line.

https://codereview.chromium.org/564953002/diff/60001/ui/message_center/cocoa/...
File ui/message_center/cocoa/status_item_view.mm (right):

https://codereview.chromium.org/564953002/diff/60001/ui/message_center/cocoa/...
ui/message_center/cocoa/status_item_view.mm:156: [nsAppearanceClass
respondsToSelector:@selector(appearanceNamed:)]) {
And this should be +instancesRespondToSelector:, which is a class method.

https://codereview.chromium.org/564953002/diff/60001/ui/message_center/cocoa/...
ui/message_center/cocoa/status_item_view.mm:158:
appearanceNamed:NSAppearanceNameVibrantDark];
nit: indent +4

[dewittj, 2014-09-15 17:32:37]

https://codereview.chromium.org/564953002/diff/60001/base/mac/sdk_forward_dec...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/564953002/diff/60001/base/mac/sdk_forward_dec...
base/mac/sdk_forward_declarations.h:270: @protocol CrNSAppearance
On 2014/09/15 16:36:02, rsesek wrote:
> To resolve the compile errors, add " <NSObject>" at the end of this line.

Done.

https://codereview.chromium.org/564953002/diff/60001/ui/message_center/cocoa/...
File ui/message_center/cocoa/status_item_view.mm (right):

https://codereview.chromium.org/564953002/diff/60001/ui/message_center/cocoa/...
ui/message_center/cocoa/status_item_view.mm:156: [nsAppearanceClass
respondsToSelector:@selector(appearanceNamed:)]) {
On 2014/09/15 16:36:02, rsesek wrote:
> And this should be +instancesRespondToSelector:, which is a class method.

Done.

https://codereview.chromium.org/564953002/diff/60001/ui/message_center/cocoa/...
ui/message_center/cocoa/status_item_view.mm:158:
appearanceNamed:NSAppearanceNameVibrantDark];
On 2014/09/15 16:36:02, rsesek wrote:
> nit: indent +4

Done.

https://codereview.chromium.org/564953002/diff/60001/ui/resources/ui_resource...
File ui/resources/ui_resources.grd (left):

https://codereview.chromium.org/564953002/diff/60001/ui/resources/ui_resource...
ui/resources/ui_resources.grd:29: <structure type="chrome_scaled_image"
name="IDR_APP_LIST_SEARCH_ICON" file="common/app_list_search_icon.png" />
On 2014/09/13 00:42:48, oshima wrote:
> is this intentional?

Nope, good catch.

https://codereview.chromium.org/564953002/diff/80001/ui/message_center/cocoa/...
File ui/message_center/cocoa/status_item_view.mm (right):

https://codereview.chromium.org/564953002/diff/80001/ui/message_center/cocoa/...
ui/message_center/cocoa/status_item_view.mm:154: Class nsAppearanceClass =
NSClassFromString(@"NSAppearance");
I had to remove <CrNSObject> from this in order to make this work.

[Robert Sesek, 2014-09-15 18:01:36]

LGTM

[Robert Sesek, 2014-09-15 18:02:37]

https://codereview.chromium.org/564953002/diff/80001/base/mac/sdk_forward_dec...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/564953002/diff/80001/base/mac/sdk_forward_dec...
base/mac/sdk_forward_declarations.h:257: -
(id<CrNSAppearance>)effectiveAppearance;
Actually, this needs to go into the 10.10 section below.

[dewittj, 2014-09-15 19:15:09]

https://codereview.chromium.org/564953002/diff/80001/base/mac/sdk_forward_dec...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/564953002/diff/80001/base/mac/sdk_forward_dec...
base/mac/sdk_forward_declarations.h:257: -
(id<CrNSAppearance>)effectiveAppearance;
On 2014/09/15 18:02:37, rsesek wrote:
> Actually, this needs to go into the 10.10 section below.

Really?
https://developer.apple.com/library/mac/documentation/AppKit/Reference/NSAppe...
shows effectiveappearance "Available in OS X v10.9 and later".

[Robert Sesek, 2014-09-15 19:40:56]

https://codereview.chromium.org/564953002/diff/80001/base/mac/sdk_forward_dec...
File base/mac/sdk_forward_declarations.h (right):

https://codereview.chromium.org/564953002/diff/80001/base/mac/sdk_forward_dec...
base/mac/sdk_forward_declarations.h:257: -
(id<CrNSAppearance>)effectiveAppearance;
On 2014/09/15 19:15:08, dewittj wrote:
> On 2014/09/15 18:02:37, rsesek wrote:
> > Actually, this needs to go into the 10.10 section below.
> 
> Really?
>
https://developer.apple.com/library/mac/documentation/AppKit/Reference/NSAppe...
> shows effectiveappearance "Available in OS X v10.9 and later".

Oh, my mistake. I skimmed the 10.10 API diffs too quickly.

[dewittj, 2014-09-16 22:51:00]

Added 4 pressed resources.

rsesek: minor changes - will you please verify on your yosemite box?
oshima: PTAL

[Robert Sesek, 2014-09-17 23:01:02]

On 2014/09/16 22:51:00, dewittj wrote:
> Added 4 pressed resources.
> 
> rsesek: minor changes - will you please verify on your yosemite box?
> oshima: PTAL

I can't patch this in because of the binary files, unfortunately. Could you
upload a Chromium build to gdrive and send me the link so I can test?

[dewittj, 2014-09-23 00:54:36]

oshima: ping

[dewittj, 2014-09-23 22:54:44]

rsesek:  try "git apply $FILENAME" after saving the binary patch file here:

https://drive.google.com/a/chromium.org/file/d/0B7CI7Ya8SYWjd2pOZXE3T1p2VmM/e...

[oshima, 2014-09-23 23:01:26]

ui/resources lgtm

[dewittj, 2014-09-24 16:22:41]

I'm going to land this; rsesek plz let me know if you find an issue with the
resources in this patch.

[dewittj, 2014-09-24 16:22:46]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2014-09-24 16:23:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/564953002/120001

[Robert Sesek, 2014-09-24 17:14:18]

The CQ bit was unchecked by rsesek@chromium.org

[Robert Sesek, 2014-09-24 17:15:01]

https://codereview.chromium.org/564953002/diff/120001/ui/message_center/cocoa...
File ui/message_center/cocoa/status_item_view.mm (right):

https://codereview.chromium.org/564953002/diff/120001/ui/message_center/cocoa...
ui/message_center/cocoa/status_item_view.mm:157:
instancesRespondToSelector:@selector(appearanceNamed:)]) {
Tested. This does need to change back to -respondsToSelector: and then
everything works great.

[dewittj, 2014-09-25 18:21:39]

https://codereview.chromium.org/564953002/diff/120001/ui/message_center/cocoa...
File ui/message_center/cocoa/status_item_view.mm (right):

https://codereview.chromium.org/564953002/diff/120001/ui/message_center/cocoa...
ui/message_center/cocoa/status_item_view.mm:157:
instancesRespondToSelector:@selector(appearanceNamed:)]) {
On 2014/09/24 17:15:00, Robert Sesek wrote:
> Tested. This does need to change back to -respondsToSelector: and then
> everything works great.

Is this what you mean?  It compiles on my 10.9 box.

[dewittj, 2014-09-25 18:21:46]

The CQ bit was checked by dewittj@chromium.org

[Robert Sesek, 2014-09-25 18:23:27]

https://codereview.chromium.org/564953002/diff/120001/ui/message_center/cocoa...
File ui/message_center/cocoa/status_item_view.mm (right):

https://codereview.chromium.org/564953002/diff/120001/ui/message_center/cocoa...
ui/message_center/cocoa/status_item_view.mm:157:
instancesRespondToSelector:@selector(appearanceNamed:)]) {
On 2014/09/25 18:21:39, dewittj wrote:
> On 2014/09/24 17:15:00, Robert Sesek wrote:
> > Tested. This does need to change back to -respondsToSelector: and then
> > everything works great.
> 
> Is this what you mean?  It compiles on my 10.9 box.

Yup. I'm not sure why it produced a compiler error before.

[commit-bot: I haz the power, 2014-09-25 18:23:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/564953002/140001

[commit-bot: I haz the power, 2014-09-25 19:38:18]

Committed patchset #8 (id:140001) as a54743428561df548a13abdd1b180b628a356f78

[commit-bot: I haz the power, 2014-09-25 19:39:43]

Patchset 8 (id:??) landed as
https://crrev.com/81d046cf0412b39ee6ac88e4bae10ab7768df5ac
Cr-Commit-Position: refs/heads/master@{#296766}