Mac[Views]: Make native menus more responsive by pumping private runloop modes.

base/message_loop/message_pump_mac.mm

 // Copyright (c) 2012 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.
 
 #import "base/message_loop/message_pump_mac.h"
 
 #import <Foundation/Foundation.h>
 
 #include <limits>
 
 #include "base/logging.h"
 #include "base/mac/call_with_eh_frame.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "base/macros.h"
 #include "base/message_loop/timer_slack.h"
 #include "base/run_loop.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 
 #if !defined(OS_IOS)
 #import <AppKit/AppKit.h>
 #endif  // !defined(OS_IOS)
 
 namespace base {
 
 namespace {
 
+// AppKit RunLoop modes observed to potentially run tasks posted to Chrome's
+// main thread task runner. Some are internal to AppKit but must be observed to
+// keep Chrome's UI responsive. Others that may be interesting, but are not
+// watched:
+//  - com.apple.hitoolbox.windows.transitionmode
+//  - com.apple.hitoolbox.windows.flushmode
+const CFStringRef kAllModes[] = {
+    kCFRunLoopCommonModes, kMessageLoopExclusiveRunLoopMode,

[Mark Mentovai, 2017/05/10 14:00:18]

I guess I’m not supposed to complain if clang-format wrote this, but the second
item on this line is kind of hiding.

[Robert Sesek, 2017/05/10 14:02:49]

I complain all the time about this because clang-format is wrong, and I'm not
going to apologize for that :).

I've started using "// clang-format off" around lists of things to compensate.

[tapted, 2017/05/11 00:24:06]

I added a comment, 

// Mode that only sees Chrome work sources.
kMessageLoopExclusiveRunLoopMode,


(Incidentally... and probably orthogonal, do we still need
kMessageLoopExclusiveRunLoopMode?

`git grep` only shows it being passed to CFRunLoopAddObserver(), but then only
in combination with kCFRunLoopCommonModes -- it looks like we only ever spin a
run loop in the kMessageLoopExclusiveRunLoopMode in a couple of unit test
files).

[Robert Sesek, 2017/05/11 16:12:36]

I was wondering the same thing. The Exclusive mode was added a while ago, but I
don't think it was ultimately used as intended. I can't tell if the few cases
where it's currently used are actually required, or if CommonModes are
sufficient.

+
+    // Process work when NSMenus are fading out.
+    CFSTR("com.apple.hitoolbox.windows.windowfadingmode"),
+
+    // Process work when AppKit is highlighting an item on the main menubar.
+    CFSTR("NSUnhighlightMenuRunLoopMode"),
+};
+
 void CFRunLoopAddSourceToAllModes(CFRunLoopRef rl, CFRunLoopSourceRef source) {

[Robert Sesek, 2017/05/10 13:37:27]

One last thought: it would be nice if we could limit this to the
MessagePumpNSApplication, but I don't see a clean way to do that since these are
installed in the MessagePumpCFRunLoopBase and the members are private, not
protected. Maybe Mark has an idea.

[tapted, 2017/05/11 00:24:05]

We could pass a std::vector of scoped_cftyperef<CFStringRef> to the
MessagePumpCFRunLoopBase constructor? Or a vector<size_t> of kAllModes indices.

I don't know enough about the message pump stuff to know how crucial this is..
Leaving as is, but LMK if I should explore it now, or in a follow-up.

[Robert Sesek, 2017/05/11 16:12:36]

I don't think it's critical since the mode should only be used on the AppKit
main thread. Passing a vector of additional modes (in addition to CommonModes)
to the MessagePumpCFRunLoopBase constructor seems reasonable. If this CL sticks,
I think it's worth considering.

-  CFRunLoopAddSource(rl, source, kCFRunLoopCommonModes);
-  CFRunLoopAddSource(rl, source, kMessageLoopExclusiveRunLoopMode);
+  for (const CFStringRef& mode : kAllModes)
+    CFRunLoopAddSource(rl, source, mode);
 }
 
 void CFRunLoopRemoveSourceFromAllModes(CFRunLoopRef rl,
                                        CFRunLoopSourceRef source) {
-  CFRunLoopRemoveSource(rl, source, kCFRunLoopCommonModes);
-  CFRunLoopRemoveSource(rl, source, kMessageLoopExclusiveRunLoopMode);
+  for (const CFStringRef& mode : kAllModes)
+    CFRunLoopRemoveSource(rl, source, mode);
 }
 
 void CFRunLoopAddTimerToAllModes(CFRunLoopRef rl, CFRunLoopTimerRef timer) {
-  CFRunLoopAddTimer(rl, timer, kCFRunLoopCommonModes);
-  CFRunLoopAddTimer(rl, timer, kMessageLoopExclusiveRunLoopMode);
+  for (const CFStringRef& mode : kAllModes)
+    CFRunLoopAddTimer(rl, timer, mode);
 }
 
 void CFRunLoopRemoveTimerFromAllModes(CFRunLoopRef rl,
                                       CFRunLoopTimerRef timer) {
-  CFRunLoopRemoveTimer(rl, timer, kCFRunLoopCommonModes);
-  CFRunLoopRemoveTimer(rl, timer, kMessageLoopExclusiveRunLoopMode);
+  for (const CFStringRef& mode : kAllModes)
+    CFRunLoopRemoveTimer(rl, timer, mode);
 }
 
 void CFRunLoopAddObserverToAllModes(CFRunLoopRef rl,
                                     CFRunLoopObserverRef observer) {
-  CFRunLoopAddObserver(rl, observer, kCFRunLoopCommonModes);
-  CFRunLoopAddObserver(rl, observer, kMessageLoopExclusiveRunLoopMode);
+  for (const CFStringRef& mode : kAllModes)
+    CFRunLoopAddObserver(rl, observer, mode);
 }
 
 void CFRunLoopRemoveObserverFromAllModes(CFRunLoopRef rl,
                                          CFRunLoopObserverRef observer) {
-  CFRunLoopRemoveObserver(rl, observer, kCFRunLoopCommonModes);
-  CFRunLoopRemoveObserver(rl, observer, kMessageLoopExclusiveRunLoopMode);
+  for (const CFStringRef& mode : kAllModes)
+    CFRunLoopRemoveObserver(rl, observer, mode);
 }
 
 void NoOp(void* info) {
 }
 
 const CFTimeInterval kCFTimeIntervalMax =
     std::numeric_limits<CFTimeInterval>::max();
 
 #if !defined(OS_IOS)
 // Set to true if MessagePumpMac::Create() is called before NSApp is
@@ skipping 804 matching lines @@
     [NSApplication sharedApplication];
     g_not_using_cr_app = true;
     return new MessagePumpNSApplication;
 #endif
   }
 
   return new MessagePumpNSRunLoop;
 }
 
 }  // namespace base