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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: moco, Assigned: stanshebs)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jaas
:
review+
benjamin
:
superreview+
|
Details | Diff | Splinter Review |
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.
Comment 1•18 years ago
|
||
*** Bug 355815 has been marked as a duplicate of this bug. ***
Updated•18 years ago
|
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
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.
Updated•18 years ago
|
Flags: blocking1.9?
Updated•18 years ago
|
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
Updated•18 years ago
|
Assignee: nobody → joshmoz
Component: General → Widget: Cocoa
QA Contact: general → cocoa
Comment 3•18 years ago
|
||
(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 | ||
Updated•18 years ago
|
Assignee: joshmoz → stanshebs
Assignee | ||
Comment 4•18 years ago
|
||
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...
Assignee | ||
Updated•18 years ago
|
Hardware: All → Macintosh
Assignee | ||
Comment 5•18 years ago
|
||
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.
Comment 6•18 years ago
|
||
I think NSApplication's delegate methods should be able to help a great deal here.
Assignee | ||
Comment 7•18 years ago
|
||
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.
Assignee | ||
Comment 8•18 years ago
|
||
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.
Assignee | ||
Comment 9•18 years ago
|
||
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.
Comment 10•18 years ago
|
||
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.
Comment 11•18 years ago
|
||
Having Gecko change the NSApp delegate out from under Camino would break a whole, whole lot of Camino code.
Assignee | ||
Comment 12•18 years ago
|
||
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.
Comment 13•18 years ago
|
||
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.
Assignee | ||
Comment 14•18 years ago
|
||
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.
Comment 15•18 years ago
|
||
(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.
Assignee | ||
Comment 16•18 years ago
|
||
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.
Assignee | ||
Comment 17•18 years ago
|
||
Assignee | ||
Comment 18•18 years ago
|
||
(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)
Comment 19•18 years ago
|
||
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-
Assignee | ||
Comment 20•18 years ago
|
||
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+
Assignee | ||
Updated•18 years ago
|
Attachment #248569 -
Flags: review+ → review-
Comment 21•18 years ago
|
||
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.
Assignee | ||
Comment 22•18 years ago
|
||
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 23•18 years ago
|
||
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.
Comment 24•18 years ago
|
||
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.
Assignee | ||
Comment 25•18 years ago
|
||
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 26•18 years ago
|
||
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 27•18 years ago
|
||
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+
Comment 28•18 years ago
|
||
landed on trunk, thanks Stan!
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Updated•18 years ago
|
Flags: blocking1.9? → in-testsuite?
You need to log in
before you can comment on or make changes to this bug.
Description
•