364497 - When app is opened from a link, the link should respect the external app pref for opening in the same window as saved sessions

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[froodian (Ian Leue)]

STR:

1. Have some session information, and session saving on.
2. Quit
3. From some other app, click a link that will launch Camino

What happens: URL always opens in a new window
What should happen: URL should respect the "links from other applications" pref, and open in a new tab in the frontmost window from the saved sessions.

What's probably happening here is that the session information hasn't loaded yet when the pref gets checked, so it always thinks there isn't an open window to load in.

1.1 for now, but early candidate for kickage.

Comment 1

[Stuart Morgan]

(In reply to comment #0)
> What's probably happening here is that the session information hasn't loaded
> yet when the pref gets checked, so it always thinks there isn't an open window
> to load in.

The pref is never checked at all; handling a launch-and-open follows a very different code path than everything else at the moment.

Comment 2

[Sean Murphy]

I addressed this issue on bug 358689 comment 23 since a fix required moving around the window state restoration code from that bug.

Comment 3

[Sean Murphy]

I'm still trying to resolve this bug and any further patches will be posted here.

An additional observation of opening a URL in a new window on launch along with a restored session is that the newly opened window will open front and key with the exact frame as the topmost restored window.  The only visual indication that there is a restored window underneath is the increased intensity of the two window shadows overlapping each other.  The newly opened window on launch should probably exhibit behavior consistent to choosing File->New Window and offset itself down and to the right of the previous window.  It should not open directly on top of another window.

This may warrant a separate bug report though, since obeying "links from other applications" will not explicitly prevent such behavior from occurring if the preference has been set to use a new window; a fix here will just minimize exposure to it since opening in tabs becomes an available option.

Comment 4

[Chris Lawson (gone)]

Murph: any more progress on this? With the other bug being fixed, we sort of forgot about it.

Comment 5

[Sean Murphy]

Attachment: fix

Thanks cl for reminding me about this one.  It did actually slip my mind because I forgot to assign myself to it and didn't notice my address when searching through open stuff.

There are two different locations in our code where external URLs are opened.  URLs dropped onto the dock icon come in by way of Apple Events and are handled by GetURLCommand.mm.  Clicking on an external link with Camino as the default browser, or informing Finder to open a saved URL with Camino, is handled by the NSApplication delegate method application:openFile: in MainController.mm.

Both of these two methods are called and finished executing before -applicationDidFinishLaunching:, which is where the window state is restored.  So, what happens currently is that a the external URL is opened, and it will see that there's nothing already opened and therefore even though it is obeying all preferences, it has to simply open a new window.

The solution is to defer to opening of external links until after the window state is restored and in place.  After doing so, the opened link will notice the open windows and then behave in accordance with the preferences.

In each case, performing showURL: on MainController with a delay of "0" using performSelector:withObject:afterDelay results in the actual execution of that method being bumped to the end of the current event loop.  This means that if there's other existing code in the event loop that's waiting to be processed (such as applicationDidFinishLaunching:), it will be allowed to execute ahead of showURL:.

Using a delay of "0" will permit all other times - when Camino is already launched and there's no need to defer the sending of showURL: - to avoid incurring any noticeable negative impact on the time it takes to open a URL.

So, when Camino is opened via a link or dock drop and window state information is restored along with it, the opened link will properly obey the prefs and appear in a new (rightmost) tab or a new window offset down and to the right (not directly above anymore either as I talked about previously).  Dropping/opening multiple URLs works properly as well.

As far as the code goes:

There is some repetition, since there's two paths which an opened link can take.  The identical code could be pulled out and abstracted into another method which simply calls showURL: after the delay.  I didn't go that route yet and introduce another method, but if it is in fact a better approach we can do so.

Also, I didn't remove it yet, but is it now unnecessary to call [self ensureGeckoInitted] each time?  After querying ensureGeckoInitted, the only succeeding method is the call to showURL:, which should now always be executed after all initialization messages have been performed.

Comment 6

[Stuart Morgan]

Do we really want to be returning from an AE handler without having opened the window? That seems like it could have really unexpected results for a script that opens a window then take some action on it.

Comment 7

[Sean Murphy]

I'm glad you thought of that consequence; I've been trying to work up another idea and should have something soon.

The general approach I've been taking is to ensure that the session has been restored before any open events have been processed.  As a user, if I were to launch Camino with a URL to visit upon opening, I'd expect my previous session to show up prior to navigating to the new URL.

Comment 8

[Sean Murphy]

A bit of time has passed since I last commented on this bug, so I just wanted to mention that it hasn't fallen off my radar and I'm busy working on another solution.  I plan on submitting that patch tomorrow, after I've looked the code over some more.  Sorry for the delay everyone.

Comment 9

[Sean Murphy]

Attachment: another approach

Ensures that the session is restored before we handle the opening of any URLs which caused Camino to launch.

The script command is suspended if performDefaultImplementation is called before MainController is fully initialized.  I chose to use Key-Value Observing to later execute and resume the script command when initialization is completed.  Without KVO, MainController would have to keep track of pending commands itself and resume them as part of the initialization routine.

Comment 10

[Stuart Morgan]

Comment on attachment 260087 [details] [diff] [review]
another approach

This approach looks good; just a few changes.

>+- (BOOL)isInitialized;
> - (void)ensureGeckoInitted;

Since you are removing the only external dependency on ensureGeckoInitted, go ahead and remove it from the header.

Also, since there is now a general initialization method that will always be called at the right time, please move this:

>  // To work around a bug on Tiger where the view hookup order has been changed from postfix to prefix
>  // order, we need to set a user default to return to the old behavior.
>  [[NSUserDefaults standardUserDefaults] setBool:NO forKey:@"NSViewSetAncestorsWindowFirst"];

to just after initing Gecko in your new method. It has nothing to do with Gecko, there just wasn't a good place to put it before. And since you'll be touching it anyway, please change the comment to:
  // To work around bugs on Tiger caused by the view hookup order having been
  // changed from postfix to prefix order, we need to set a user default to
  // return to the old behavior.
since the current comment isn't really accurate.

> - (void)applicationDidFinishLaunching:(NSNotification*)aNotification
> {
>+  [self finishInitialization];
>+}

Put the new initialization method before this, so the application* methods stay grouped

>+- (void)finishInitialization
>+{
>+  if ([self isInitialized])
>+    return;

Since it's safe to call more than once, how about ensureInitializationCompleted?

>+  if (![self isInitialized])
>+    [self finishInitialization];

No need for the if-check here.

Comment 11

[Sean Murphy]

Attachment: previous approach updated

Same approach, with the changes suggested in comment #10.

Comment 12

[Stuart Morgan]

Comment on attachment 261608 [details] [diff] [review]
previous approach updated

r=me

Comment 13

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 261608 [details] [diff] [review]
previous approach updated

sr=pink

Comment 14

[froodian (Ian Leue)]

Checked in on trunk and MOZILLA_1_8_BRANCH.