Closed Bug 355352 Opened 18 years ago Closed 18 years ago

[Cocoa] while the app is running, if no browser windows are open, clicking on the app in the dock doesn't open a browser window

Categories

(Core :: Widget: Cocoa, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: moco, Assigned: stanshebs)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

while the app is running, if no browser windows are open, clicking on the app in the doc doesn't open a browser window

I think this is a recent regression, possibly due to the cocoa widget change.
*** Bug 355815 has been marked as a duplicate of this bug. ***
Blocks: cocoa
No longer depends on: cocoa
Hardware: PC → All
Summary: while the app is running, if no browser windows are open, clicking on the app in the doc doesn't open a browser window → while the app is running, if no browser windows are open, clicking on the app in the dock doesn't open a browser window
Assignee: nobody → joshmoz
Under Carbon, we get an Apple Event telling us to open a new window when the dock is clicked and no window are open. Under Cocoa, we don't even get the event. Perhaps the OS is getting confused about the hidden window and thinking we already have a window open.
Flags: blocking1.9?
Assignee: joshmoz → nobody
Product: Firefox → Core
QA Contact: general → general
Summary: while the app is running, if no browser windows are open, clicking on the app in the dock doesn't open a browser window → [Cocoa] while the app is running, if no browser windows are open, clicking on the app in the dock doesn't open a browser window
Assignee: nobody → joshmoz
Component: General → Widget: Cocoa
QA Contact: general → cocoa
(In reply to comment #2)
> Under Carbon, we get an Apple Event telling us to open a new window when the
> dock is clicked and no window are open. Under Cocoa, we don't even get the
> event. Perhaps the OS is getting confused about the hidden window and thinking
> we already have a window open.

I wonder if that is related to bug 352646. Is this bug reproducible if there is no EM restart? I know the EM restart seems to confuse OS X system services seriously. :-(
Assignee: joshmoz → stanshebs
I may be misunderstanding everything, but it doesn't seem like the app is handling *any* apple event except oapp. Dragging a file onto an already-running app has no effect either, and quitting from the dock does not result in an orderly shutdown. Off to study Cocoa vs AE...
Hardware: All → Macintosh
Understanding is coming slowly, but after looking at the Carbon AE stuff in widget/src/mac and in xpfe, and discussing with Josh, I'm focussing on adding AE handling the pure Cocoa way a la Camino.
I think NSApplication's delegate methods should be able to help a great deal here.
Yeah, the delegate methods are logical, but so far I'm not getting them to actually run. The NS event tracing stuff shows the app is indeed receiving the apple events, so I think they are somehow getting dropped on the floor between native and gecko event handling.
Well, after some painstaking asm stepping that got me to the same place that I could have gotten by setting a breakpoint on the raw address instead of expecting a pending breakpoint to be properly installed, it's clear that 'rapp' events are getting to NSApplication properly. So now I'm looking at how the delegate stuff has been set up.
OK, I'm getting it now - we have no delegate of NSApplication at all! (I was tricked by the applicationWillTerminate method in AppShellDelegate, which btw doesn't ever seem to run either.) The options are basically to create one the proper way, making a new class and pair of files (with or without IB), or to overload AppShellDelegate, less clean but workable and less code, since NSApplication only cares that the class has certain methods.
I'm not sure it's a good idea to make gecko set the app's delegate to itself, if it wants to be embeddable.

The best way to go might be to create a application delegate in Firefox-specific code (browser/).   Camino would have to deal with it in its own way (etc).

This is reasonable because an application that is using Gecko needs to be able to control by its own what applescript actions it supports, what drag & drop on the app's icon does, etc.

Maybe you could create a helper class that does the hard work (if any), that these applications can share.

I think WebKit works like this. There are a lot of WebKit classes that help developers do common actions, but the work of binding it to the application specifics and what-not, is something the developer should be in control of.
Having Gecko change the NSApp delegate out from under Camino would break a whole, whole lot of Camino code.
Right now I'm focussing on toolkit/xre, on the theory that XUL apps all have the same Carbon AE handling built into them, so adding a Cocoa equivalent shouldn't be disruptive.
If XUL apps are the only apps that instantiate an nsAppShell, we could hook up a default NSApp delegate there, and embedders could use whatever they want without any code changes. Or we could do it from toolkit/xre if that is the same deal, where embedders don't use it.
It looks like Camino depends on nsAppShell to direct native and Gecko events to their right places, and that seems sensible. There is a bit of NSApplication dinking in the file that looks a little dubious for embedding, perhaps a redundant no-op for Camino and so nobody notices.
(In reply to comment #14)
> It looks like Camino depends on nsAppShell to direct native and Gecko events to
> their right places, and that seems sensible. There is a bit of NSApplication
> dinking in the file that looks a little dubious for embedding, perhaps a
> redundant no-op for Camino and so nobody notices.

You can't put it in widget/ since that would affect every mac app that ever uses gecko, I think the right place will have to be browser/ or toolkit/. It's probably most sensible that each (Thunderbird, Firefox, etc) have their own delegate code for NSApplication, as they will want to be able to open different files, and do different things with their Dock icon, etc.

OK, I think I'm getting the hang of this Gecko thing! New file  (because it has to be Obj-C++) in toolkit/xre seems to do the trick. A little more tinkering to get to the reviewable patch.
(I see I should have taken advantage of the comment box when posting the patch.)

This patch does both the reopen per this bug, and opening of files dragged onto the app icon. Not really that complicated, just a matter of connecting Cocoa methods to existing code.

As toolkit Mac hackery, I'm guessing that both josh and bsmedberg should review.
Attachment #247997 - Flags: review?(joshmoz)
Blocks: 363190
Comment on attachment 247997 [details] [diff] [review]
implements delegate methods for XUL apps using Cocoa on Mac

MacApplicationDelegate *mDelegate;

We don't need to hold a reference to the delegate object. You can always get it via "[NSApp delegate]".

+  nsCOMPtr<nsINativeAppSupport> nas;
+
+  // If there are windows already, nothing to do.
+  if (flag)
+    return NO;
+
+  nas = do_CreateInstance(NS_NATIVEAPPSUPPORT_CONTRACTID);
+  NS_ENSURE_TRUE(nas, NO);

Just declare "nas" down where you assign to it first, after the (flag) check.

 ifneq (,$(filter mac cocoa,$(MOZ_WIDGET_TOOLKIT)))
 CMSRCS = MacLaunchHelper.m
+CMMSRCS = MacApplicationDelegate.mm
 CPPSRCS += nsCommandLineServiceMac.cpp
 LOCAL_INCLUDES += -I$(topsrcdir)/xpfe/bootstrap/appleevents
 OS_CXXFLAGS += -fexceptions
 SHARED_LIBRARY_LIBS += $(DEPTH)/xpfe/bootstrap/appleevents/$(LIB_PREFIX)appleevents_s.$(LIB_SUFFIX)
 endif

We should probably not compile the old carbon Apple Event stuff any more, right? If you don't want to change that in this patch, please file a bug on the issue.

Also, we shouldn't be compiling this code if the toolkit is "mac" - we should only do it under the cocoa toolkit.

 #ifdef XP_MACOSX
 #include "MacLaunchHelper.h"
+void SetupMacApplicationDelegate(void);
 #endif

Same issue here, we only want to do that under cocoa, not under carbon. You can't just wrap that in XP_MACOSX.

Sorta makes me uncomfortable to not have a proper header for MacApplicationDelegate. Can you just make one and then include that in nsAppRunner instead of declaring the function there?

Other than that stuff, look pretty good! I want to do some more testing after round 2 is posted.
Attachment #247997 - Flags: review?(joshmoz) → review-
This addresses the issues raised. Both Carbon and Cocoa versions build and run correctly. I pushed the Carbon AE code issue off to 363747.
Attachment #247997 - Attachment is obsolete: true
Attachment #248569 - Flags: review+
Attachment #248569 - Flags: review+ → review-
Comment on attachment 248569 [details] [diff] [review]
New and improved patch

Just a few comments from skimming over the code...

>+void
>+SetupMacApplicationDelegate()
>+{
>+  // Create the delegate.
>+  MacApplicationDelegate *delegate = [[MacApplicationDelegate alloc] init];
>+
>+  // Designate it as the NSApplication's delegate.
>+  [[NSApplication sharedApplication] setDelegate:delegate];
>+}

I assume it's ok to leak this, since it will be around for the lifetime of the app, right? Wanna make that explicit in a comment? ;)


>+- (BOOL)application:(NSApplication*)theApplication openFile:(NSString*)filename
>+{
>+  FSRef ref;
>+  FSSpec spec;
>+  // The cast is kind of freaky, but apparently it's what all the beautiful people do.

FSRefs and FSSpecs are carbon/ancient toolbox ghosts. Please don't introduce them into new code (unless really really necessary). Cocoa (and Core Foundation) probably has all the file utilities you'll need.
Heh, the lifetime of the delegate was explicit in a comment in the first version, but Josh made me change it. :-) I'll put it back.

FSRef/FSSpec is needed to connect to the old command-line code, which is Carbon.

Comment on attachment 248569 [details] [diff] [review]
New and improved patch

If you need to use FSRef/FSSpec to connect with the command line code, I don't mind you using it. Don't rewrite another component at this point just to avoid doing that here.

+- (BOOL)applicationShouldHandleReopen:(NSApplication*)theApp hasVisibleWindows:(BOOL)flag
+{
+  nsresult rv = NS_OK;
+
+  // If there are windows already, nothing to do.
+  if (flag)
+    return NO;

You can declare rv after the flag check. It won't be used before then.

This looks pretty good! I still have to test it, but with the few minor changes that hwaara and I mentioned it looks good to go from a code standpoint.
One more thing - filing a bug about investigating the removal/disabling of the old carbon apple event code should be a prerequisite for landing this patch. I do not want to be shipping all that code in the end if it is not used. You can assign that bug to me if you want.
You saw my mention of 363747, right? Just for grins, I tried whacking the ProcessAppleEvents() call - the good news is that double-clicking works (the delegate now picking up the odoc), the bad news is that it always opens an additional window with one's homepage. So it will take a little real work to eliminate the Carbon dependency.
Blocks: 363747
Comment on attachment 248569 [details] [diff] [review]
New and improved patch

The minor changes remaining (comments mostly) can be made on checkin.
Attachment #248569 - Flags: superreview?(benjamin)
Attachment #248569 - Flags: review-
Attachment #248569 - Flags: review+
Comment on attachment 248569 [details] [diff] [review]
New and improved patch

moa=bsmedberg (I didn't review, I'm trusting josh to have looked this over thoroughly)
Attachment #248569 - Flags: superreview?(benjamin) → superreview+
landed on trunk, thanks Stan!
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Flags: blocking1.9? → in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: