[Mac] Don't close the Wrench menu after using the zoom buttons if the menu was opened sticky.

[Mac] Don't close the Wrench menu after using the zoom buttons if the menu was opened sticky.

This runs the Task that updates the zoom information in the UI thread outside of the
normal MessageLoop and instead runs it on the native run loop.

R=mark
BUG=48679
TEST=Open Wrench menu with a click. Use zoom buttons. Menu stays open and page zooms. Other buttons still close menu.
TEST=Open Wrench menu with click-hold-drag. Hover and release on zoom buttons. Menu closes and page zooms.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56566

[Mark Mentovai, 2010-08-17 19:24:03]

http://codereview.chromium.org/3183013/diff/1/4
File chrome/browser/cocoa/wrench_menu_controller.h (right):

http://codereview.chromium.org/3183013/diff/1/4#newcode21
chrome/browser/cocoa/wrench_menu_controller.h:21: }
}  // namespace WrenchMenuControllerInternal

http://codereview.chromium.org/3183013/diff/1/5
File chrome/browser/cocoa/wrench_menu_controller.mm (left):

http://codereview.chromium.org/3183013/diff/1/5#oldcode27
chrome/browser/cocoa/wrench_menu_controller.mm:27: self = [super init];
The old implementation here was kind of stupid, wasn’t it?

http://codereview.chromium.org/3183013/diff/1/5
File chrome/browser/cocoa/wrench_menu_controller.mm (right):

http://codereview.chromium.org/3183013/diff/1/5#newcode38
chrome/browser/cocoa/wrench_menu_controller.mm:38: controller_ = nil;
Unnecessary. Do you even need a destructor declared at all?

http://codereview.chromium.org/3183013/diff/1/5#newcode44
chrome/browser/cocoa/wrench_menu_controller.mm:44: [controller_
wrenchMenuModel]->UpdateZoomControls();
|DCHECK_EQ(type.value, NotificationType::ZOOM_LEVEL_CHANGED);|?

http://codereview.chromium.org/3183013/diff/1/5#newcode44
chrome/browser/cocoa/wrench_menu_controller.mm:44: [controller_
wrenchMenuModel]->UpdateZoomControls();
You use the wrench menu model twice. You should stuff it into a local variable
so you only need to fetch it once. That will probably help you make the next
line more readable: doesn’t this way of breaking the lines flow better?:


    const string16 level =
        wrenchMenuModel->GetLabelForCommandId(IDC_ZOOM_PERCENT_DISPLAY);

http://codereview.chromium.org/3183013/diff/1/5#newcode50
chrome/browser/cocoa/wrench_menu_controller.mm:50: NotificationRegistrar
registrar_;
private:

http://codereview.chromium.org/3183013/diff/1/5#newcode51
chrome/browser/cocoa/wrench_menu_controller.mm:51: WrenchMenuController*
controller_;
Provide a setter for this. Even better, set it in the constructor.  Also, say | 
// weak|.

http://codereview.chromium.org/3183013/diff/1/5#newcode54
chrome/browser/cocoa/wrench_menu_controller.mm:54: }  //
WrenchMenuControllerInternal
Say “class” here.

http://codereview.chromium.org/3183013/diff/1/5#newcode140
chrome/browser/cocoa/wrench_menu_controller.mm:140: // The zoom buttons should
not close the menu if opened sticky.
Blank line before. Break up the flow. Make it easy to see that something new and
noteworthy is about to happen.

http://codereview.chromium.org/3183013/diff/1/5#newcode142
chrome/browser/cocoa/wrench_menu_controller.mm:142: if ([sender
performSelector:@selector(isTracking)]) {
|if (a) { if (b) { c; } }| can be rewritten as |if (a && b) { c; }|. I don’t
think that splitting it up into two ifs is a readability win in this case.

http://codereview.chromium.org/3183013/diff/1/7
File chrome/browser/ui_thread_helpers.h (right):

http://codereview.chromium.org/3183013/diff/1/7#newcode9
chrome/browser/ui_thread_helpers.h:9: #include "base/task.h"
You can probably get away with a forward declaration of Task, and either a
forward declaration of tracked_objects::Location or simply an #include for
"base/tracked.h". Limit header entanglement!

http://codereview.chromium.org/3183013/diff/1/7#newcode13
chrome/browser/ui_thread_helpers.h:13: // This can be used in place of
ChromeThread::PostTask(ChromeThread::UI,...).
space before ...

http://codereview.chromium.org/3183013/diff/1/8
File chrome/browser/ui_thread_helpers_linux.cc (right):

http://codereview.chromium.org/3183013/diff/1/8#newcode7
chrome/browser/ui_thread_helpers_linux.cc:7: #include
"chrome/browser/chrome_thread.h"
Remember to #include "base/task.h" in these implementation files if you pull it
out of the header.

http://codereview.chromium.org/3183013/diff/1/8#newcode18
chrome/browser/ui_thread_helpers_linux.cc:18: 
Blank line at EOF.

http://codereview.chromium.org/3183013/diff/1/9
File chrome/browser/ui_thread_helpers_mac.mm (right):

http://codereview.chromium.org/3183013/diff/1/9#newcode17
chrome/browser/ui_thread_helpers_mac.mm:17: @public
@public?

http://codereview.chromium.org/3183013/diff/1/9#newcode39
chrome/browser/ui_thread_helpers_mac.mm:39: UITaskHelperMac* runner =
[UITaskHelperMac new];
I know you love showing off by using -new, but providing -initWithTask: or just
+uiTaskHelperWithTask: would be more orthodox, and it’d resolve your use of
@public above. You know, the one that prompted me to say “?”.

(The autoreleased version, +uiTaskWithHelperTask:, would let you get rid of the
autorelease in -runTask.)

http://codereview.chromium.org/3183013/diff/1/9#newcode45
chrome/browser/ui_thread_helpers_mac.mm:45: NSDefaultRunLoopMode, nil];
Didn’t I suggest reindenting this to suck less?

http://codereview.chromium.org/3183013/diff/1/9#newcode54
chrome/browser/ui_thread_helpers_mac.mm:54: 
Blank line at EOF v2.0.

http://codereview.chromium.org/3183013/diff/1/10
File chrome/browser/ui_thread_helpers_win.cc (right):

http://codereview.chromium.org/3183013/diff/1/10#newcode17
chrome/browser/ui_thread_helpers_win.cc:17: 
v3.0.

[Robert Sesek, 2010-08-17 20:11:55]

Addressed all comments.

http://codereview.chromium.org/3183013/diff/1/4
File chrome/browser/cocoa/wrench_menu_controller.h (right):

http://codereview.chromium.org/3183013/diff/1/4#newcode21
chrome/browser/cocoa/wrench_menu_controller.h:21: }
On 2010/08/17 19:24:03, Mark Mentovai wrote:
> }  // namespace WrenchMenuControllerInternal

Done.

http://codereview.chromium.org/3183013/diff/1/5
File chrome/browser/cocoa/wrench_menu_controller.mm (right):

http://codereview.chromium.org/3183013/diff/1/5#newcode38
chrome/browser/cocoa/wrench_menu_controller.mm:38: controller_ = nil;
On 2010/08/17 19:24:03, Mark Mentovai wrote:
> Unnecessary. Do you even need a destructor declared at all?

Nope.

http://codereview.chromium.org/3183013/diff/1/5#newcode44
chrome/browser/cocoa/wrench_menu_controller.mm:44: [controller_
wrenchMenuModel]->UpdateZoomControls();
On 2010/08/17 19:24:03, Mark Mentovai wrote:
> |DCHECK_EQ(type.value, NotificationType::ZOOM_LEVEL_CHANGED);|?

Done.

http://codereview.chromium.org/3183013/diff/1/5#newcode51
chrome/browser/cocoa/wrench_menu_controller.mm:51: WrenchMenuController*
controller_;
On 2010/08/17 19:24:03, Mark Mentovai wrote:
> Provide a setter for this. Even better, set it in the constructor.  Also, say
| 
> // weak|.

Done.

http://codereview.chromium.org/3183013/diff/1/5#newcode54
chrome/browser/cocoa/wrench_menu_controller.mm:54: }  //
WrenchMenuControllerInternal
On 2010/08/17 19:24:03, Mark Mentovai wrote:
> Say “class” here.

I'd rather say namespace.

http://codereview.chromium.org/3183013/diff/1/5#newcode140
chrome/browser/cocoa/wrench_menu_controller.mm:140: // The zoom buttons should
not close the menu if opened sticky.
On 2010/08/17 19:24:03, Mark Mentovai wrote:
> Blank line before. Break up the flow. Make it easy to see that something new
and
> noteworthy is about to happen.

Done.

http://codereview.chromium.org/3183013/diff/1/5#newcode142
chrome/browser/cocoa/wrench_menu_controller.mm:142: if ([sender
performSelector:@selector(isTracking)]) {
On 2010/08/17 19:24:03, Mark Mentovai wrote:
> |if (a) { if (b) { c; } }| can be rewritten as |if (a && b) { c; }|. I don’t
> think that splitting it up into two ifs is a readability win in this case.

I considered this and didn't.  But now I did.

http://codereview.chromium.org/3183013/diff/1/7
File chrome/browser/ui_thread_helpers.h (right):

http://codereview.chromium.org/3183013/diff/1/7#newcode9
chrome/browser/ui_thread_helpers.h:9: #include "base/task.h"
On 2010/08/17 19:24:03, Mark Mentovai wrote:
> You can probably get away with a forward declaration of Task, and either a
> forward declaration of tracked_objects::Location or simply an #include for
> "base/tracked.h". Limit header entanglement!

I thought of this after I sent this out.

http://codereview.chromium.org/3183013/diff/1/7#newcode13
chrome/browser/ui_thread_helpers.h:13: // This can be used in place of
ChromeThread::PostTask(ChromeThread::UI,...).
On 2010/08/17 19:24:03, Mark Mentovai wrote:
> space before ...

Done.

http://codereview.chromium.org/3183013/diff/1/9
File chrome/browser/ui_thread_helpers_mac.mm (right):

http://codereview.chromium.org/3183013/diff/1/9#newcode39
chrome/browser/ui_thread_helpers_mac.mm:39: UITaskHelperMac* runner =
[UITaskHelperMac new];
On 2010/08/17 19:24:03, Mark Mentovai wrote:
> I know you love showing off by using -new, but providing -initWithTask: or
just
> +uiTaskHelperWithTask: would be more orthodox, and it’d resolve your use of
> @public above. You know, the one that prompted me to say “?”.
> 
> (The autoreleased version, +uiTaskWithHelperTask:, would let you get rid of
the
> autorelease in -runTask.)

Autoreleasing factories annoy me for some reason.  I added a designated
initializer, though.

http://codereview.chromium.org/3183013/diff/1/9#newcode45
chrome/browser/ui_thread_helpers_mac.mm:45: NSDefaultRunLoopMode, nil];
On 2010/08/17 19:24:03, Mark Mentovai wrote:
> Didn’t I suggest reindenting this to suck less?

Allays.

[Mark Mentovai, 2010-08-17 20:23:38]

LGTM

http://codereview.chromium.org/3183013/diff/1/9
File chrome/browser/ui_thread_helpers_mac.mm (right):

http://codereview.chromium.org/3183013/diff/1/9#newcode39
chrome/browser/ui_thread_helpers_mac.mm:39: UITaskHelperMac* runner =
[UITaskHelperMac new];
rsesek wrote:
> Autoreleasing factories annoy me for some reason.

But they’re all over Cocoa!

I don’t care about the difference between +uiTaskWithTask: or -initWithTask: as
long as you chose one of them. What you have now is good.

http://codereview.chromium.org/3183013/diff/10001/11004
File chrome/browser/cocoa/wrench_menu_controller.mm (right):

http://codereview.chromium.org/3183013/diff/10001/11004#newcode52
chrome/browser/cocoa/wrench_menu_controller.mm:52: WrenchMenuController*
controller_;  // weak, owns us
Come on! Just say weak, don’t say us!

http://codereview.chromium.org/3183013/diff/10001/11008
File chrome/browser/ui_thread_helpers_mac.mm (right):

http://codereview.chromium.org/3183013/diff/10001/11008#newcode54
chrome/browser/ui_thread_helpers_mac.mm:54: NSDefaultRunLoopMode, nil];
nil to its own line, too.