Add default hotkey dictionary for com.apple.symbolichotkeys.plist

Add default hotkey dictionary for com.apple.symbolichotkeys.plist

On a new user account
"~/Library/Preferences/com.apple.symbolichotkeys.plist" only contains
18 hotkeys (where window switch entry "27" is missing), but OS will use
some private APIs to get the default values.
(See https://bugs.chromium.org/p/chromium/issues/detail?id=383558#c20)

This CL simulates OS's behavior and uses a default hotkey dictionary if
the action is missing.

BUG=145062
Committed: https://crrev.com/2d59d8b7e771d9823b8e4d1a3384ebd9bb1d4adc
Cr-Commit-Position: refs/heads/master@{#384701}

[chongz, 2016-03-31 23:08:05]

chongz@chromium.org changed reviewers:
+ erikchen@chromium.org

[chongz, 2016-03-31 23:08:05]

Hi erikchen@, can you take a look at this CL please? Thanks!

[erikchen, 2016-04-01 03:55:12]

https://codereview.chromium.org/1853533002/diff/1/content/browser/cocoa/syste...
File content/browser/cocoa/system_hotkey_map.mm (right):

https://codereview.chromium.org/1853533002/diff/1/content/browser/cocoa/syste...
content/browser/cocoa/system_hotkey_map.mm:42: @"parameters" : @[ @96, @50,
@1048576 ],
Where did these numbers come from?

https://codereview.chromium.org/1853533002/diff/1/content/browser/cocoa/syste...
content/browser/cocoa/system_hotkey_map.mm:94: // Safe for user disabled
hotkeys. If a hotkey was disabled it will have
I don't understand your comment. I think you are trying to say:

"""
Start with a dictionary of default OS X hotkeys that are not necessarily listed
in com.apple.symbolichotkeys.plist. If the user has overridden any of these
hotkeys, -NSMutableDictionary addEntriesFromDictionary:] will ensure that the
new values are used.
"""

https://codereview.chromium.org/1853533002/diff/1/content/browser/cocoa/syste...
content/browser/cocoa/system_hotkey_map.mm:95: // |enabled| set to "NO" instead
of removed from |user_hotkey_dictionaries|.
Add a link to https://bugs.chromium.org/p/chromium/issues/detail?id=145062#c8

https://codereview.chromium.org/1853533002/diff/1/content/browser/cocoa/syste...
content/browser/cocoa/system_hotkey_map.mm:97: [kDefaultSymbolicHotKeys
mutableCopy];
You are leaking a reference. Use scoped_nsobject.

[chongz, 2016-04-01 19:32:40]

Patchset #2 (id:20001) has been deleted

[chongz, 2016-04-01 19:34:03]

Patchset #2 (id:40001) has been deleted

[chongz, 2016-04-01 19:47:18]

Hi erikchen, I've updated CL as per your comments, PTAL, thanks!

https://codereview.chromium.org/1853533002/diff/1/content/browser/cocoa/syste...
File content/browser/cocoa/system_hotkey_map.mm (right):

https://codereview.chromium.org/1853533002/diff/1/content/browser/cocoa/syste...
content/browser/cocoa/system_hotkey_map.mm:42: @"parameters" : @[ @96, @50,
@1048576 ],
On 2016/04/01 03:55:12, erikchen wrote:
> Where did these numbers come from?

They come from my local file, replaced with meaningful names (have no idea what
@96 means, but it's not used in our code)

https://codereview.chromium.org/1853533002/diff/1/content/browser/cocoa/syste...
content/browser/cocoa/system_hotkey_map.mm:94: // Safe for user disabled
hotkeys. If a hotkey was disabled it will have
On 2016/04/01 03:55:11, erikchen wrote:
> I don't understand your comment. I think you are trying to say:
> 
> """
> Start with a dictionary of default OS X hotkeys that are not necessarily
listed
> in com.apple.symbolichotkeys.plist. If the user has overridden any of these
> hotkeys, -NSMutableDictionary addEntriesFromDictionary:] will ensure that the
> new values are used.
> """

Done. I was trying to emphasize that not only modified hotkeys but also disabled
hotkeys are handled.

https://codereview.chromium.org/1853533002/diff/1/content/browser/cocoa/syste...
content/browser/cocoa/system_hotkey_map.mm:95: // |enabled| set to "NO" instead
of removed from |user_hotkey_dictionaries|.
On 2016/04/01 03:55:11, erikchen wrote:
> Add a link to https://bugs.chromium.org/p/chromium/issues/detail?id=145062#c8

Done.

https://codereview.chromium.org/1853533002/diff/1/content/browser/cocoa/syste...
content/browser/cocoa/system_hotkey_map.mm:97: [kDefaultSymbolicHotKeys
mutableCopy];
On 2016/04/01 03:55:12, erikchen wrote:
> You are leaking a reference. Use scoped_nsobject.

Done.

[erikchen, 2016-04-01 19:48:49]

lgtm

https://codereview.chromium.org/1853533002/diff/60001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_map.mm (right):

https://codereview.chromium.org/1853533002/diff/60001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_map.mm:107: // See
http://crbug.com/145062#c8
s/http/https

[chongz, 2016-04-01 19:55:51]

chongz@chromium.org changed reviewers:
+ avi@chromium.org

[chongz, 2016-04-01 19:55:51]

Hi avi@, PTAL, thanks!

[Avi (use Gerrit), 2016-04-01 20:03:56]

https://codereview.chromium.org/1853533002/diff/80001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_map.mm (right):

https://codereview.chromium.org/1853533002/diff/80001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_map.mm:40: NSDictionary*
kDefaultSymbolicHotKeys = @{
Does using @{} at the top level cause static initialization code?

http://clang.llvm.org/docs/ObjectiveCLiterals.html implies that this turns into
a call to +[NSDictionary dictionaryWithObjects:forKeys:count:], which is
runtime.

You might need to put this into a function.

https://codereview.chromium.org/1853533002/diff/80001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_map.mm:110: [hotkey_dictionaries
addEntriesFromDictionary:user_hotkey_dictionaries];
... or rather than having a "constant" global kDefaultSymbolicHotKeys and making
a mutable copy, just put the @{} here and always parse it.

[chongz, 2016-04-01 20:46:46]

Hi avi@, thanks for the review! PTAL again, thanks!

https://codereview.chromium.org/1853533002/diff/80001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_map.mm (right):

https://codereview.chromium.org/1853533002/diff/80001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_map.mm:40: NSDictionary*
kDefaultSymbolicHotKeys = @{
On 2016/04/01 20:03:56, Avi wrote:
> Does using @{} at the top level cause static initialization code?
> 
> http://clang.llvm.org/docs/ObjectiveCLiterals.html implies that this turns
into
> a call to +[NSDictionary dictionaryWithObjects:forKeys:count:], which is
> runtime.
> 
> You might need to put this into a function.

Done.

https://codereview.chromium.org/1853533002/diff/80001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_map.mm:110: [hotkey_dictionaries
addEntriesFromDictionary:user_hotkey_dictionaries];
On 2016/04/01 20:03:56, Avi wrote:
> ... or rather than having a "constant" global kDefaultSymbolicHotKeys and
making
> a mutable copy, just put the @{} here and always parse it.

Didn't find a way to initialize NSMutableDictionary from @{}, I guess I have to
use mutableCopy?

[Avi (use Gerrit), 2016-04-01 20:56:40]

lgtm

Nah, that's fine.

[chongz, 2016-04-01 21:03:17]

The CQ bit was checked by chongz@chromium.org

[chongz, 2016-04-01 21:03:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1853533002/#ps100001
(title: "avi's review")

[commit-bot: I haz the power, 2016-04-01 21:03:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1853533002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1853533002/100001

[commit-bot: I haz the power, 2016-04-01 21:58:31]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-04-01 22:01:16]

Description was changed from

==========
Add default hotkey dictionary for com.apple.symbolichotkeys.plist

On a new user account
"~/Library/Preferences/com.apple.symbolichotkeys.plist" only contains
18 hotkeys (where window switch entry "27" is missing), but OS will use
some private APIs to get the default values.
(See https://bugs.chromium.org/p/chromium/issues/detail?id=383558#c20)

This CL simulates OS's behavior and uses a default hotkey dictionary if
the action is missing.

BUG=145062
==========

to

==========
Add default hotkey dictionary for com.apple.symbolichotkeys.plist

On a new user account
"~/Library/Preferences/com.apple.symbolichotkeys.plist" only contains
18 hotkeys (where window switch entry "27" is missing), but OS will use
some private APIs to get the default values.
(See https://bugs.chromium.org/p/chromium/issues/detail?id=383558#c20)

This CL simulates OS's behavior and uses a default hotkey dictionary if
the action is missing.

BUG=145062

Committed: https://crrev.com/2d59d8b7e771d9823b8e4d1a3384ebd9bb1d4adc
Cr-Commit-Position: refs/heads/master@{#384701}
==========

[commit-bot: I haz the power, 2016-04-01 22:01:18]

Patchset 4 (id:??) landed as
https://crrev.com/2d59d8b7e771d9823b8e4d1a3384ebd9bb1d4adc
Cr-Commit-Position: refs/heads/master@{#384701}