mac: Load the system hotkeys after launch.

mac: Load the system hotkeys after launch.

Shortly after launch, the system hotkeys are loaded and parsed. If a hotkey is
reserved by the system, it is not passed to the renderer. This allows system
hotkeys like (cmd + `) to work even if a flash plugin is selected.

Add a histogram to ensure that the system hotkey plist is being correctly
loaded and parsed.

BUG=383558
TEST=Open 2 Chrome windows. Navigate one to www.twitch.tv. The site should
include a flash plugin that automatically starts playing a video. Select the
flash plugin. The hotkey combination (cmd + `) should switch between the open
windows. The hotkey combination (cmd + L) should have no effect (it is a Chrome
hotkey, not a browser hotkey).

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283405

[erikchen, 2014-07-07 21:48:03]

avi: Please review.

[Avi (use Gerrit), 2014-07-07 21:57:49]

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:455: #endif  // defined(OS_MACOSX) &&
!defined(OS_IOS)
But this isn't a per-Browser thing, this is a system-wide thing.

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm (right):

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm:28: void
SystemHotkeyHelperMac::DeferredLoadSystemHotkeys() {
This is called repeatedly from Browser::Browser. Isn't this going to hit up the
file over and over? Do we want to have a boolean saying we've done this, or a
dispatch_once or something?

https://codereview.chromium.org/370293004/diff/100001/content/public/browser/...
File content/public/browser/render_widget_host_view_mac_delegate.h (right):

https://codereview.chromium.org/370293004/diff/100001/content/public/browser/...
content/public/browser/render_widget_host_view_mac_delegate.h:65: -
(BOOL)eventIsReservedBySystem:(NSEvent*)event;
No matter what the embedder is, this feature is needed, yes? If so, this isn't
something that goes here, but this should live entirely in content.

[Robert Sesek, 2014-07-07 22:21:36]

drive-by

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm (right):

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm:28: void
SystemHotkeyHelperMac::DeferredLoadSystemHotkeys() {
On 2014/07/07 21:57:49, Avi (OOO July 11-27) wrote:
> This is called repeatedly from Browser::Browser. Isn't this going to hit up
the
> file over and over? Do we want to have a boolean saying we've done this, or a
> dispatch_once or something?

Couldn't this just be done from the constructor, actually?

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm:47:
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
I think it'd be better to use Chrome threading primitives, specifically
BrowserThread. I'd post the task to BrowserThread::FILE (or maybe even
FILE_USER_BLOCKING) and then post the result back to BrowserThread::UI.

[erikchen, 2014-07-08 01:51:39]

avi, rsesek: PTAL

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:455: #endif  // defined(OS_MACOSX) &&
!defined(OS_IOS)
On 2014/07/07 21:57:49, Avi (OOO July 11-27) wrote:
> But this isn't a per-Browser thing, this is a system-wide thing.

I didn't realize that more than 1 Chrome browser is created. When would a second
browser be created?

I've moved all the code to content/ to mirror your ThemeHelperMac CL.

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm (right):

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm:28: void
SystemHotkeyHelperMac::DeferredLoadSystemHotkeys() {
On 2014/07/07 22:21:36, rsesek wrote:
> On 2014/07/07 21:57:49, Avi (OOO July 11-27) wrote:
> > This is called repeatedly from Browser::Browser. Isn't this going to hit up
> the
> > file over and over? Do we want to have a boolean saying we've done this, or
a
> > dispatch_once or something?
> 
> Couldn't this just be done from the constructor, actually?

I've moved to code into content/ and mirrored avi's ThemeHelperMac logic. I'm
relying on the assumption that BrowserThreadsStarted is only called once. (Is
this correct?)

If it is incorrect, than I'll use dispatch_once. I dislike having the
constructor for a static object perform implicit work. By having an explicit
method perform the relevant initialization, the call site's logic is more easily
understood.

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm:47:
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
On 2014/07/07 22:21:36, rsesek wrote:
> I think it'd be better to use Chrome threading primitives, specifically
> BrowserThread. I'd post the task to BrowserThread::FILE (or maybe even
> FILE_USER_BLOCKING) and then post the result back to BrowserThread::UI.

I've removed the calls to gcd in favor of Chrome threading primitives. I've kept
the file reads on BrowserThread::FILE, since this should not block the UI.

The new logic requires an ugly forward declaration in the header file, as well
as some manual memory counting. (The header file is compiled in C++ only
translation units, so no ObjC syntax can be used. The forward declaration
technically violates the ODR, but its a common Chrome style.

Is this what you were thinking of? This seems more complex and brittle than
using GCD. Is the primary benefit that all file reads are happening on the
BrowserThread::FILE thread? If so, we should probably make some gcd-style block
based helper functions.

https://codereview.chromium.org/370293004/diff/100001/content/public/browser/...
File content/public/browser/render_widget_host_view_mac_delegate.h (right):

https://codereview.chromium.org/370293004/diff/100001/content/public/browser/...
content/public/browser/render_widget_host_view_mac_delegate.h:65: -
(BOOL)eventIsReservedBySystem:(NSEvent*)event;
On 2014/07/07 21:57:49, Avi (OOO July 11-27) wrote:
> No matter what the embedder is, this feature is needed, yes? If so, this isn't
> something that goes here, but this should live entirely in content.

I wasn't sure whether this feature should be embedder-agnostic. I've moved all
the logic to content/ as per your suggestion.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_helper_mac.mm (right):

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:52:
base::Bind(&SystemHotkeyHelperMac::LoadFile, base::Unretained(this)));
I couldn't use a weak ptr here because the weak ptr/factory is bound to a
different thread. Making a second weak_ptr_factory doesn't make sense.
Alternatives?

[Avi (use Gerrit), 2014-07-08 02:04:20]

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:455: #endif  // defined(OS_MACOSX) &&
!defined(OS_IOS)
I don't have time for a full review right now (I'll look at it tomorrow) but
FYI, each Chrome window open has a Browser object. Every time you hit command-N
you get another one.

[Robert Sesek, 2014-07-08 13:40:01]

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm (right):

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm:47:
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
On 2014/07/08 01:51:39, erikchen1 wrote:
> On 2014/07/07 22:21:36, rsesek wrote:
> > I think it'd be better to use Chrome threading primitives, specifically
> > BrowserThread. I'd post the task to BrowserThread::FILE (or maybe even
> > FILE_USER_BLOCKING) and then post the result back to BrowserThread::UI.
> 
> I've removed the calls to gcd in favor of Chrome threading primitives. I've
kept
> the file reads on BrowserThread::FILE, since this should not block the UI.
> 
> The new logic requires an ugly forward declaration in the header file, as well
> as some manual memory counting. (The header file is compiled in C++ only
> translation units, so no ObjC syntax can be used. The forward declaration
> technically violates the ODR, but its a common Chrome style.
> 
> Is this what you were thinking of? This seems more complex and brittle than
> using GCD. Is the primary benefit that all file reads are happening on the
> BrowserThread::FILE thread? If so, we should probably make some gcd-style
block
> based helper functions.

Yes, your latest patch set is the correct way to do this in Chromium. We don't
use GCD for threading unless there's specific features of libdispatch that we
need to use; BrowserThreads/TaskRunners are the right way to do inter-thread
message passing. I don't know why you think this is more complex or brittle,
though.

https://codereview.chromium.org/370293004/diff/100001/content/public/browser/...
File content/public/browser/render_widget_host_view_mac_delegate.h (right):

https://codereview.chromium.org/370293004/diff/100001/content/public/browser/...
content/public/browser/render_widget_host_view_mac_delegate.h:65: -
(BOOL)eventIsReservedBySystem:(NSEvent*)event;
On 2014/07/08 01:51:39, erikchen1 wrote:
> On 2014/07/07 21:57:49, Avi (OOO July 11-27) wrote:
> > No matter what the embedder is, this feature is needed, yes? If so, this
isn't
> > something that goes here, but this should live entirely in content.
> 
> I wasn't sure whether this feature should be embedder-agnostic. I've moved all
> the logic to content/ as per your suggestion.

I don't know if this should be embedder-agnostic or not. Is there a scenario
where you wouldn't want to obey any system hot keys? Maybe for Chrome Apps? A
native app can choose to take over/remap standard hotkeys. I don't know if we
want to support this, though.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_helper_mac.h (right):

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.h:21: class SystemHotkeyHelperMac
{
Does this need to be its own class? It's small enough to just be part of
SystemHotkeyMap.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.h:52: scoped_ptr<SystemHotkeyMap>
map_;
Any reason this needs to be a pointer instead of just a member?

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_helper_mac.mm (right):

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:18: // Amount of time to delay
loading the hotkeys.
Units? In the name of the symbol, too, please.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:28:
LeakySingletonTraits<SystemHotkeyHelperMac> >::get();
Why does this need to be leaky?

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:32:
base::MessageLoop::current()->PostDelayedTask(
You can post this directly to the FILE thread.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:52:
base::Bind(&SystemHotkeyHelperMac::LoadFile, base::Unretained(this)));
On 2014/07/08 01:51:39, erikchen1 wrote:
> I couldn't use a weak ptr here because the weak ptr/factory is bound to a
> different thread. Making a second weak_ptr_factory doesn't make sense.
> Alternatives?

Right, weak pointer only works on a given thread. Why do you think you even need
a weak pointer? Since this is a singleton, there's no danger in using
base::Unretained.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:59: [kSystemHotkeyPlistPath
stringByExpandingTildeInPath];
base::mac::GetUserLibraryPath()

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:60: NSData* data = [NSData
dataWithContentsOfFile:expandedFilePath];
Use alloc/init so you don't need to manually retain it.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:60: NSData* data = [NSData
dataWithContentsOfFile:expandedFilePath];
Alternatively, use base::ReadFileToString and pass around a std::string(). Then
FileDidLoad doesn't need to do any manual memory management.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:73: bool success =
map()->ParseData(data);
How long does parsing take? Consider doing it on the FILE thread as well?

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:74:
UMA_HISTOGRAM_BOOLEAN("Mac.SystemHotkeyMap.Load", success);
Use OSX. instead of Mac.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_map.h (right):

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_map.h:19: // Maintains a listing of all OSX
user modifiable hotkeys. e.g. (cmd + `)
To what end…?

https://codereview.chromium.org/370293004/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/370293004/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2590: //
TODO(erikchen): SystemHotkeyHelperMac should use FS api to monitor changes
"FS api" ? Don't use abbreviations in comments.

[erikchen, 2014-07-10 02:07:32]

rsesek: PTAL

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm (right):

https://codereview.chromium.org/370293004/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm:47:
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
On 2014/07/08 13:40:00, rsesek (OOO July 9-13) wrote:
> On 2014/07/08 01:51:39, erikchen1 wrote:
> > On 2014/07/07 22:21:36, rsesek wrote:
> > > I think it'd be better to use Chrome threading primitives, specifically
> > > BrowserThread. I'd post the task to BrowserThread::FILE (or maybe even
> > > FILE_USER_BLOCKING) and then post the result back to BrowserThread::UI.
> > 
> > I've removed the calls to gcd in favor of Chrome threading primitives. I've
> kept
> > the file reads on BrowserThread::FILE, since this should not block the UI.
> > 
> > The new logic requires an ugly forward declaration in the header file, as
well
> > as some manual memory counting. (The header file is compiled in C++ only
> > translation units, so no ObjC syntax can be used. The forward declaration
> > technically violates the ODR, but its a common Chrome style.
> > 
> > Is this what you were thinking of? This seems more complex and brittle than
> > using GCD. Is the primary benefit that all file reads are happening on the
> > BrowserThread::FILE thread? If so, we should probably make some gcd-style
> block
> > based helper functions.
> 
> Yes, your latest patch set is the correct way to do this in Chromium. We don't
> use GCD for threading unless there's specific features of libdispatch that we
> need to use; BrowserThreads/TaskRunners are the right way to do inter-thread
> message passing. I don't know why you think this is more complex or brittle,
> though.

I'm happy to discuss this point offline. I will discontinue further discussion
on this CL since the code matches your expectations.

https://codereview.chromium.org/370293004/diff/100001/content/public/browser/...
File content/public/browser/render_widget_host_view_mac_delegate.h (right):

https://codereview.chromium.org/370293004/diff/100001/content/public/browser/...
content/public/browser/render_widget_host_view_mac_delegate.h:65: -
(BOOL)eventIsReservedBySystem:(NSEvent*)event;
On 2014/07/08 13:40:00, rsesek (OOO July 9-13) wrote:
> On 2014/07/08 01:51:39, erikchen1 wrote:
> > On 2014/07/07 21:57:49, Avi (OOO July 11-27) wrote:
> > > No matter what the embedder is, this feature is needed, yes? If so, this
> isn't
> > > something that goes here, but this should live entirely in content.
> > 
> > I wasn't sure whether this feature should be embedder-agnostic. I've moved
all
> > the logic to content/ as per your suggestion.
> 
> I don't know if this should be embedder-agnostic or not. Is there a scenario
> where you wouldn't want to obey any system hot keys? Maybe for Chrome Apps? A
> native app can choose to take over/remap standard hotkeys. I don't know if we
> want to support this, though.

Given that avi has a clear preference, and that myself and rsesek seem on the
fence, I've kept the logic in content/

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_helper_mac.h (right):

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.h:21: class SystemHotkeyHelperMac
{
On 2014/07/08 13:40:00, rsesek (OOO July 9-13) wrote:
> Does this need to be its own class? It's small enough to just be part of
> SystemHotkeyMap.

It would technically be possible to finesse the SystemHotkeyMap class to have no
ObjC in its header file, but it would be ugly. This class also serves the
purpose of having a C++ translation unit safe header file that can invoke ObjC
in its implementation.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.h:52: scoped_ptr<SystemHotkeyMap>
map_;
On 2014/07/08 13:40:00, rsesek (OOO July 9-13) wrote:
> Any reason this needs to be a pointer instead of just a member?

Yes, for it to be a member, I'd have to move the #include into this file, but
the #include is not safe for C++ translation units, which this file is a part
of.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_helper_mac.mm (right):

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:18: // Amount of time to delay
loading the hotkeys.
On 2014/07/08 13:40:01, rsesek (OOO July 9-13) wrote:
> Units? In the name of the symbol, too, please.

Done.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:28:
LeakySingletonTraits<SystemHotkeyHelperMac> >::get();
On 2014/07/08 13:40:01, rsesek (OOO July 9-13) wrote:
> Why does this need to be leaky?

I was copy-pasting some code, and didn't read it too carefully. You're right, it
doesn't need to be leaky.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:32:
base::MessageLoop::current()->PostDelayedTask(
On 2014/07/08 13:40:00, rsesek (OOO July 9-13) wrote:
> You can post this directly to the FILE thread.

Done.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:52:
base::Bind(&SystemHotkeyHelperMac::LoadFile, base::Unretained(this)));
On 2014/07/08 13:40:01, rsesek (OOO July 9-13) wrote:
> On 2014/07/08 01:51:39, erikchen1 wrote:
> > I couldn't use a weak ptr here because the weak ptr/factory is bound to a
> > different thread. Making a second weak_ptr_factory doesn't make sense.
> > Alternatives?
> 
> Right, weak pointer only works on a given thread. Why do you think you even
need
> a weak pointer? Since this is a singleton, there's no danger in using
> base::Unretained.

Removed all the weak pointers.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:59: [kSystemHotkeyPlistPath
stringByExpandingTildeInPath];
On 2014/07/08 13:40:00, rsesek (OOO July 9-13) wrote:
> base::mac::GetUserLibraryPath()

Done.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:60: NSData* data = [NSData
dataWithContentsOfFile:expandedFilePath];
On 2014/07/08 13:40:01, rsesek (OOO July 9-13) wrote:
> Use alloc/init so you don't need to manually retain it.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:60: NSData* data = [NSData
dataWithContentsOfFile:expandedFilePath];
On 2014/07/08 13:40:01, rsesek (OOO July 9-13) wrote:
> Alternatively, use base::ReadFileToString and pass around a std::string().
Then
> FileDidLoad doesn't need to do any manual memory management.

Are you suggesting that we pass around a C++ object instead of a pointer? That
sounds like there will be much more overhead. Let me know if I'm misinterpreting
your suggestion.

https://codereview.chromium.org/370293004/diff/160001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:74:
UMA_HISTOGRAM_BOOLEAN("Mac.SystemHotkeyMap.Load", success);
On 2014/07/08 13:40:01, rsesek (OOO July 9-13) wrote:
> Use OSX. instead of Mac.

Done.

https://codereview.chromium.org/370293004/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/370293004/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:2590: //
TODO(erikchen): SystemHotkeyHelperMac should use FS api to monitor changes
On 2014/07/08 13:40:01, rsesek (OOO July 9-13) wrote:
> "FS api" ? Don't use abbreviations in comments.

Done.

[Avi (use Gerrit), 2014-07-10 02:19:11]

I'm pretty happy with this. If you can make that scoped_nsobject change work,
that would be great.

LGTM in any case, and make rsesek happy too.

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_helper_mac.h (right):

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.h:47: void
FileDidLoad(NSDictionary* dictionary);
If you can express this with a parameter of scoped_nsobject<NSDictionary>
instead and make it work with the task posting, that would be preferable to
expressing the retain count semantics in the comment.

[Robert Sesek, 2014-07-14 15:33:32]

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_helper_mac.h (right):

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.h:21: class SystemHotkeyHelperMac
{
Every class needs a class-level comment.

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.h:37: virtual
~SystemHotkeyHelperMac();
Doesn't need to be virtual.

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.h:47: void
FileDidLoad(NSDictionary* dictionary);
On 2014/07/10 02:19:10, Avi (OOO July 11-27) wrote:
> If you can express this with a parameter of scoped_nsobject<NSDictionary>
> instead and make it work with the task posting, that would be preferable to
> expressing the retain count semantics in the comment.

I'd just rephrase the comment to say "This takes ownership of the |dictionary|."

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_helper_mac.mm (right):

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:51: NSString* expandedFilePath
=
In C++ methods, use under_score naming. E.g. expanded_file_path.

[erikchen, 2014-07-14 18:17:38]

rsesek: PTAL

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_helper_mac.h (right):

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.h:21: class SystemHotkeyHelperMac
{
On 2014/07/14 15:33:31, rsesek wrote:
> Every class needs a class-level comment.

Done.

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.h:37: virtual
~SystemHotkeyHelperMac();
On 2014/07/14 15:33:31, rsesek wrote:
> Doesn't need to be virtual.

Done.

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.h:47: void
FileDidLoad(NSDictionary* dictionary);
On 2014/07/14 15:33:31, rsesek wrote:
> On 2014/07/10 02:19:10, Avi (OOO July 11-27) wrote:
> > If you can express this with a parameter of scoped_nsobject<NSDictionary>
> > instead and make it work with the task posting, that would be preferable to
> > expressing the retain count semantics in the comment.
> 
> I'd just rephrase the comment to say "This takes ownership of the
|dictionary|."

I rephrased the comment as per rsesek's suggestion.

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_helper_mac.mm (right):

https://codereview.chromium.org/370293004/diff/220001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:51: NSString* expandedFilePath
=
On 2014/07/14 15:33:31, rsesek wrote:
> In C++ methods, use under_score naming. E.g. expanded_file_path.

Done.

[Robert Sesek, 2014-07-14 19:37:56]

LGTM

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

https://codereview.chromium.org/370293004/diff/240001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_map.mm:121:
std::vector<SystemHotkey>::const_iterator it;
It occurs to me that doing a linear scan of this vector on every key event may
not be great. It may make sense to turn this into a std::map. Thoughts?

[erikchen, 2014-07-14 19:54:14]

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

https://codereview.chromium.org/370293004/diff/240001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_map.mm:121:
std::vector<SystemHotkey>::const_iterator it;
On 2014/07/14 19:37:56, rsesek wrote:
> It occurs to me that doing a linear scan of this vector on every key event may
> not be great. It may make sense to turn this into a std::map. Thoughts?

When I wrote this, I deemed the minor performance hit (a linear scan of ~50
items) to be better than the minor complexity increase of using another
container. *shrug*

[erikchen, 2014-07-14 19:56:17]

isherman: Looking for an OWNER review of tools/metrics/histograms/histograms.xml

[Robert Sesek, 2014-07-14 19:59:45]

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

https://codereview.chromium.org/370293004/diff/240001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_map.mm:121:
std::vector<SystemHotkey>::const_iterator it;
On 2014/07/14 19:54:14, erikchen1 wrote:
> On 2014/07/14 19:37:56, rsesek wrote:
> > It occurs to me that doing a linear scan of this vector on every key event
may
> > not be great. It may make sense to turn this into a std::map. Thoughts?
> 
> When I wrote this, I deemed the minor performance hit (a linear scan of ~50
> items) to be better than the minor complexity increase of using another
> container. *shrug*

What do you mean another container? You could just replace the vector with a
map, right?

[erikchen, 2014-07-14 20:13:33]

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

https://codereview.chromium.org/370293004/diff/240001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_map.mm:121:
std::vector<SystemHotkey>::const_iterator it;
On 2014/07/14 19:59:45, rsesek wrote:
> On 2014/07/14 19:54:14, erikchen1 wrote:
> > On 2014/07/14 19:37:56, rsesek wrote:
> > > It occurs to me that doing a linear scan of this vector on every key event
> may
> > > not be great. It may make sense to turn this into a std::map. Thoughts?
> > 
> > When I wrote this, I deemed the minor performance hit (a linear scan of ~50
> > items) to be better than the minor complexity increase of using another
> > container. *shrug*
> 
> What do you mean another container? You could just replace the vector with a
> map, right?

Yes, I could replace the vector with a map and a comparator for SystemHotKey. Or
alternatively a multimap. But the performance characteristic for vectors is
better than maps and multimaps for small data sets because of prefetching and
caching, so its unclear if there would be any performance benefit (might even
get worse...).

If you feel strongly about this, let me know and I will write a separate CL and
change the vector to a map.

[Ilya Sherman, 2014-07-15 00:49:56]

metrics LGTM with a suggestion:

https://codereview.chromium.org/370293004/diff/240001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_helper_mac.mm (right):

https://codereview.chromium.org/370293004/diff/240001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:73:
UMA_HISTOGRAM_BOOLEAN("OSX.SystemHotkeyMap.Load", success);
Optional nit: Perhaps "Load" -> "LoadSuccess"?

[erikchen, 2014-07-15 02:45:36]

https://codereview.chromium.org/370293004/diff/240001/content/browser/cocoa/s...
File content/browser/cocoa/system_hotkey_helper_mac.mm (right):

https://codereview.chromium.org/370293004/diff/240001/content/browser/cocoa/s...
content/browser/cocoa/system_hotkey_helper_mac.mm:73:
UMA_HISTOGRAM_BOOLEAN("OSX.SystemHotkeyMap.Load", success);
On 2014/07/15 00:49:55, Ilya Sherman wrote:
> Optional nit: Perhaps "Load" -> "LoadSuccess"?

Done.

[erikchen, 2014-07-15 02:45:40]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-07-15 02:47:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikchen@chromium.org/370293004/260001

[commit-bot: I haz the power, 2014-07-15 23:50:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-16 00:26:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-16 00:26:30]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[erikchen, 2014-07-16 15:27:04]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-07-16 15:29:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/erikchen@chromium.org/370293004/260001

[commit-bot: I haz the power, 2014-07-16 15:53:40]

Change committed as 283405