Closed Bug 1127205 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: paul, Assigned: paul)

References

Details

Attachments

(2 files, 5 obsolete files)

B2G desktop has no XUL window, so no <menubar> element.
If there's no <xul:menubar>, we should fallback to a minimal menu.
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: graphene
Flags: needinfo?(mstange)
I agree, and I'd prefer if you gave it a try :-)
Flags: needinfo?(mstange)
Attached patch wip (obsolete) — Splinter Review
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)
(clearing needinfo, already f?)
Flags: needinfo?(mstange)
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+
Attached patch wip (obsolete) — Splinter Review
Attachment #8557008 - Attachment is obsolete: true
Attached patch v1 (obsolete) — Splinter Review
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.
Attached patch v2 (obsolete) — Splinter Review
Attachment #8558250 - Attachment is obsolete: true
Attachment #8558250 - Flags: review?(mstange)
Attachment #8559538 - Flags: review?(mstange)
(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.
Ignore comment 12. I was wrong. Cmd-Q works properly even if we use eCommand_ID_Quit.
Attached patch v2 (obsolete) — Splinter Review
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-
Attached patch v3Splinter Review
Attachment #8559540 - Attachment is obsolete: true
Attachment #8565835 - Flags: review?(mstange)
Attachment #8565835 - Flags: review?(mstange) → review+
Attached patch v3.1Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4242faa6a091

There was a leak. Using getter_AddRefs now.
Keywords: checkin-needed
Keywords: checkin-needed
Duplicate of this bug: 999430
https://hg.mozilla.org/mozilla-central/rev/c87629c34fcc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.