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

RESOLVED FIXED in Firefox 38

Status

()

Core
Widget: Cocoa
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

unspecified
mozilla38
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Comment 1

4 years ago
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?
Blocks: 1115098
Flags: needinfo?(mstange)
I agree, and I'd prefer if you gave it a try :-)
Flags: needinfo?(mstange)
(Assignee)

Comment 3

4 years ago
Created attachment 8557008 [details] [diff] [review]
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?
Assignee: nobody → paul
Status: NEW → ASSIGNED
Flags: needinfo?(mstange)
Attachment #8557008 - Flags: feedback?(mstange)
(Assignee)

Comment 4

4 years ago
(clearing needinfo, already f?)
Flags: needinfo?(mstange)

Comment 5

4 years ago
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 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
Attachment #8557008 - Flags: feedback?(mstange) → feedback+
(Assignee)

Comment 7

4 years ago
Created attachment 8558120 [details] [diff] [review]
wip
Attachment #8557008 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Created attachment 8558250 [details] [diff] [review]
v1
Attachment #8558120 - Attachment is obsolete: true
Attachment #8558250 - Flags: review?(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 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.
(Assignee)

Comment 11

4 years ago
Created attachment 8559538 [details] [diff] [review]
v2
Attachment #8558250 - Attachment is obsolete: true
Attachment #8558250 - Flags: review?(mstange)
Attachment #8559538 - Flags: review?(mstange)
(Assignee)

Comment 12

4 years ago
(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.
(Assignee)

Comment 13

4 years ago
Ignore comment 12. I was wrong. Cmd-Q works properly even if we use eCommand_ID_Quit.
(Assignee)

Comment 14

4 years ago
Created attachment 8559540 [details] [diff] [review]
v2
Attachment #8559538 - Attachment is obsolete: true
Attachment #8559538 - Flags: review?(mstange)
Attachment #8559540 - Flags: review?(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.
Attachment #8559540 - Flags: review?(mstange) → review-
(Assignee)

Comment 16

4 years ago
Created attachment 8565835 [details] [diff] [review]
v3
Attachment #8559540 - Attachment is obsolete: true
Attachment #8565835 - Flags: review?(mstange)
Attachment #8565835 - Flags: review?(mstange) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Duplicate of this bug: 999430
https://hg.mozilla.org/mozilla-central/rev/c87629c34fcc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.