mac: Load the system hotkeys after launch.

chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm

Index: chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm
diff --git a/chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm b/chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm
new file mode 100644
index 0000000000000000000000000000000000000000..5f7515457959d89ab4fb3c561e56d97120cf13de
--- /dev/null
+++ b/chrome/browser/ui/cocoa/system_hotkey_helper_mac.mm
@@ -0,0 +1,59 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/cocoa/system_hotkey_helper_mac.h"
+
+#include "base/bind.h"
+#include "base/message_loop/message_loop.h"
+#include "base/metrics/histogram.h"
+#include "chrome/browser/ui/cocoa/system_hotkey_map.h"
+
+namespace {
+
+NSString* kSystemHotkeyPlistPath =
+    @"~/Library/Preferences/com.apple.symbolichotkeys.plist";
+
+// Amount of time to delay loading the hotkeys.
+const int kLoadHotkeysDelay = 10;
+
+}  // namespace
+
+// static
+SystemHotkeyHelperMac* SystemHotkeyHelperMac::GetInstance() {
+  return Singleton<SystemHotkeyHelperMac,
+                   LeakySingletonTraits<SystemHotkeyHelperMac> >::get();
+}
+
+void SystemHotkeyHelperMac::DeferredLoadSystemHotkeys() {

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

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?

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

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

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

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.

+  base::MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&SystemHotkeyHelperMac::LoadSystemHotkeys,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::TimeDelta::FromSeconds(kLoadHotkeysDelay));
+}
+
+SystemHotkeyHelperMac::SystemHotkeyHelperMac()
+    : map_(new SystemHotkeyMap), weak_ptr_factory_(this) {
+}
+
+SystemHotkeyHelperMac::~SystemHotkeyHelperMac() {
+}
+
+void SystemHotkeyHelperMac::LoadSystemHotkeys() {
+  base::WeakPtr<SystemHotkeyHelperMac> weak_self =
+      weak_ptr_factory_.GetWeakPtr();
+
+  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),

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

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]

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.

[Robert Sesek, 2014/07/08 13:40:00]

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.

[erikchen, 2014/07/10 02:07:31]

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

+                 ^{
+      NSString* expandedFilePath =
+          [kSystemHotkeyPlistPath stringByExpandingTildeInPath];
+      NSData* data = [NSData dataWithContentsOfFile:expandedFilePath];
+      dispatch_async(dispatch_get_main_queue(), ^{
+          if (!weak_self.get())
+            return;
+          bool success = weak_self.get()->map()->ParseData(data);
+          UMA_HISTOGRAM_BOOLEAN("Mac.SystemHotkeyMap.Load", success);
+      });
+  });
+}