1127205 - Can't quit B2G Desktop on Mac (Cmd-Q or menu -> Quit)

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Paul Rouget [:paul]]

B2G desktop has no XUL window, so no <menubar> element.
If there's no <xul:menubar>, we should fallback to a minimal menu.

Comment 1

[Paul Rouget [:paul]]

The same way we have cocoa/nsMenuBarX.cpp:ConstructNativeMenus(), we should have ConstructFallbackNativeMenus() that would be called if no menubar is found.

Markus, do you agree? Do you have time to work on that or you want me to give it a try?

Comment 2

[Markus Stange [:mstange]]

I agree, and I'd prefer if you gave it a try :-)

Comment 3

[Paul Rouget [:paul]]

Attachment: wip

This is just an early draft. ConstructFallbackNativeMenus is called when there's no xul:menubar.

Markus, the menu is visible, but disabled. I think it's because I'm trying to use @selector(menuItemHit:). Any idea what I'm doing wrong?

Comment 4

[Paul Rouget [:paul]]

(clearing needinfo, already f?)

Comment 5

[Stefan [:stefanh]]

Comment on attachment 8557008 [details] [diff] [review]
wip

+  id quitMenuItem = [[[NSMenuItem alloc] initWithTitle:@"Quit" action:@selector(menuItemHit:)
+                                         keyEquivalent:@"q"] autorelease];

Hmm. This won't be possible to localize. But since this is a draft, you'll handle that in later iterations I guess?

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8557008 [details] [diff] [review]
wip

(In reply to Paul Rouget [:paul] from comment #3)
> Markus, the menu is visible, but disabled. I think it's because I'm trying
> to use @selector(menuItemHit:). Any idea what I'm doing wrong?

I think it's because you haven't set a target on the menuitem. The item needs to know what Objective-C object it should call the action selector on.

For localization, you can do something similar to what I did here:
https://dxr.mozilla.org/mozilla-central/source/widget/cocoa/nsPrintDialogX.mm#239

Comment 7

[Paul Rouget [:paul]]

Attachment: wip

Comment 8

[Paul Rouget [:paul]]

Attachment: v1

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8558250 [details] [diff] [review]
v1

Review of attachment 8558250 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/locales/en-US/chrome/global/fallbackMenubar.properties
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +quitMenuitem.label=Quit

Might be a good idea to add a comment above this line that says that this is about the native app menu on OS X, so that localizers that aren't on Mac don't need to go looking for it. Though I don't know how much of this file is exposed to localizers.

Maybe a toolkit person should look at this, too.

::: widget/cocoa/nsMenuBarX.mm
@@ +149,5 @@
> +{
> +  if (!nsMenuBarX::sNativeEventTarget)
> +    nsMenuBarX::sNativeEventTarget = [[NativeMenuItemTarget alloc] init];
> +
> +  id menubar = [[NSMenu new] autorelease];

Please use NSMenu* / NSMenuItem* instead of id, here and in the rest of the function.

@@ +150,5 @@
> +  if (!nsMenuBarX::sNativeEventTarget)
> +    nsMenuBarX::sNativeEventTarget = [[NativeMenuItemTarget alloc] init];
> +
> +  id menubar = [[NSMenu new] autorelease];
> +  id appMenuItem = [[NSMenuItem new] autorelease];

How does OS X know that this one is the item for the app menu? Does it just take the first item in the menu bar and turn that into the app menu?

@@ +153,5 @@
> +  id menubar = [[NSMenu new] autorelease];
> +  id appMenuItem = [[NSMenuItem new] autorelease];
> +  id appMenu = [[NSMenu new] autorelease];
> +
> +  nsIStringBundle    *stringBundle;

remove excessive spaces, * on the left (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Declarations )

@@ +165,5 @@
> +
> +  nsXPIDLString labelProp;
> +  nsXPIDLString keyProp;
> +
> +  stringBundle->GetStringFromName(NS_ConvertUTF8toUTF16("quitMenuitem.label").get(), getter_Copies(labelProp));

Try to break this line up a little. Official style guide calls for <80 characters per line, but even if we don't always manage that, this one is a little long.

@@ +168,5 @@
> +
> +  stringBundle->GetStringFromName(NS_ConvertUTF8toUTF16("quitMenuitem.label").get(), getter_Copies(labelProp));
> +  NSMutableString* label = [NSMutableString stringWithUTF8String:NS_ConvertUTF16toUTF8(labelProp).get()];
> +
> +  stringBundle->GetStringFromName(NS_ConvertUTF8toUTF16("quitMenuitem.key").get(), getter_Copies(keyProp));

here too

::: widget/cocoa/nsMenuBaseX.h
@@ +73,5 @@
> +  eCommand_ID_HideOthers   = 5,
> +  eCommand_ID_ShowAll      = 6,
> +  eCommand_ID_Update       = 7,
> +  eCommand_ID_Last         = 8,
> +  eCommand_ID_FallbackQuit = 9

Why are we using a new command ID instead of reusing the existing Quit one? I haven't really looked at how those IDs are used.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8558250 [details] [diff] [review]
v1

Review of attachment 8558250 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsMenuBarX.mm
@@ +146,5 @@
>  }
>  
> +void nsMenuBarX::ConstructFallbackNativeMenus()
> +{
> +  if (!nsMenuBarX::sNativeEventTarget)

{}

@@ +165,5 @@
> +
> +  nsXPIDLString labelProp;
> +  nsXPIDLString keyProp;
> +
> +  stringBundle->GetStringFromName(NS_ConvertUTF8toUTF16("quitMenuitem.label").get(), getter_Copies(labelProp));

Instead of NS_ConvertUTF8toUTF16("quitMenuitem.label").get(), use MOZ_UTF16("quitMenuitem.label"). See https://dxr.mozilla.org/mozilla-central/search?q=GetStringFromName+path%3A.cpp for other examples.

Comment 11

[Paul Rouget [:paul]]

Attachment: v2

Comment 12

[Paul Rouget [:paul]]

(In reply to Markus Stange [:mstange] from comment #9)
> ::: widget/cocoa/nsMenuBaseX.h
> @@ +73,5 @@
> > +  eCommand_ID_HideOthers   = 5,
> > +  eCommand_ID_ShowAll      = 6,
> > +  eCommand_ID_Update       = 7,
> > +  eCommand_ID_Last         = 8,
> > +  eCommand_ID_FallbackQuit = 9
> 
> Why are we using a new command ID instead of reusing the existing Quit one?
> I haven't really looked at how those IDs are used.

We could use eCommand_ID_Quit, but then, we couldn't make a difference between a Quit command from XUL and a "native" Quit command. We need to do so, otherwise, the Cmd-Q shortcut wouldn't not work. In `menuItemHit:`, we abort if gMenuItemsExecuteCommands is null.

But maybe I should set gMenuItemsExecuteCommands to NO.

Comment 13

[Paul Rouget [:paul]]

Ignore comment 12. I was wrong. Cmd-Q works properly even if we use eCommand_ID_Quit.

Comment 14

[Paul Rouget [:paul]]

Attachment: v2

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8559540 [details] [diff] [review]
v2

Review of attachment 8559540 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/cocoa/nsMenuBarX.mm
@@ +170,5 @@
> +
> +  stringBundle->GetStringFromName(labelProp, getter_Copies(labelUTF16));
> +  stringBundle->GetStringFromName(keyProp, getter_Copies(keyUTF16));
> +
> +  const char* labelUTF8 = NS_ConvertUTF16toUTF8(labelUTF16).get();

I think this can crash. NS_ConvertUTF16toUTF8 creates a temporary object, .get() refers to something inside that object, and the temporary object gets destructed at the end of the statement.

Better put everything in one statement, with a line break inside:
NSString* labelStr = [NSString stringWithUTF8String:
                                 NS_ConvertUTF16toUTF8(labelUTF16).get()];

And you can use NSString instead of NSMutableString here because you're not mutating the string.

Comment 16

[Paul Rouget [:paul]]

Attachment: v3

Comment 17

[Paul Rouget [:paul]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a126ea69316

Comment 18

[Paul Rouget [:paul]]

Attachment: v3.1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4242faa6a091

There was a leak. Using getter_AddRefs now.

Comment 19

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/c87629c34fcc

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c87629c34fcc