295858 - Use blank windows/tabs to open external application links

Status: VERIFIED FIXED

(Camino Graveyard :: Tabbed Browsing, enhancement)

Description

[Bruce Murphy]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.8) Gecko/20050427 Camino/0.8.4
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.8) Gecko/20050427 Camino/0.8.4

Currently, if Camino is set up to open links passed from external applications
in a new tab, it will always create a new tab to display them. This is the wrong
behavior if you have

1) a newly created blank window, this window should be used without any tabs
being created
2) A newly created blank tab, the new tab should be used. It would probably be
non-ideal to use newly created blank tabs that are not the rightmost in a window.

This behavior is very nicely handled in Safari, and would be nice to have in
camino, particularly when you're using Quicksilver or similar external
application to open folders of links in tabs.

It would also be nice if these opened tabs became current. This is the Right
Thing in most cases.

Reproducible: Always

Steps to Reproduce:
0. Set camino up to open application links in a new tab in the current window
1. Create a new window
2. Open a link from an external application
3. Observe with wonder, the blank page with which you are faced, and the new tab
with the content you wanted to the right.

Actual Results:  
See 3,

Expected Results:  
See summary.

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This is very similar to the issue in bug 248527, but I don't know if the
codepaths are shared or not, so I'm confirming.

If we can tell whether the new window/tab is blank, it should be relatively easy
to fix, no?  Putting on the 1.2 list to start out....

Comment 2

[Bruce Murphy]

This bug is probably a more general case of what's in bug 248527, I suspect
fixing this properly will fix a raft of similar issues. 

To generalise from 'blank' a bit more, a 'blank' window is one in which nothing
has been opened. IF you have your new Camino windows set to open a particular
page, and you've never gone to any new locations since that window-opening
location, the page should be considered 'blank'.

I suspect this will need a per-document (tab/window) flag on whether the window
has gone to a location that specifically excludes automatically
open-in-new-window locations, but includes everything else. Again, see Safari
for reference implementation of this.

Comment 3

[Torben]

Attachment: Reuses frontmost tab/window if empty

This will make Camino reuse the _frontmost_ tab or window if it is empty no
matter what your pref is. Doesn't check whether any other windows/tabs are
empty, wich I think would be strange behavior anyway (haven't tested what
Safari does though).

I doubt (not tested) this will not have any affect on bug 248527, that code is
somewhere else (BrowserWindowController, I might look into that if I get
bored).

Comment 4

[Torben]

Attachment: Reuse frontmost tab/window if empty, v 1.1

Fixed a possible glitch: added a check to make sure that a window that is
loading a page but has not got any content yet is not treated as empty.

Comment 5

[Chris Lawson (gone)]

Can we get some check-in lovin' here? This is something several folks have been
asking for of late. For example, see bug 308692.

cl

Comment 6

[Chris Lawson (gone)]

Comment on attachment 186137 [details] [diff] [review]
Reuse frontmost tab/window if empty, v 1.1

Requesting r from pink. This would be really nice polish for 1.0.

cl

Comment 7

[Mark Mentovai]

Comment on attachment 186137 [details] [diff] [review]
Reuse frontmost tab/window if empty, v 1.1

   BrowserWindowController* controller = (BrowserWindowController*)[[self getFrontmostBrowserWindow] windowController];
-  if (reuseWindow > kOpenNewWindowOnAE && controller) {
-    if (reuseWindow == kOpenNewTabOnAE) {
+  BOOL emptyTabOrWindow = ([[controller getBrowserWrapper] isEmpty] && ![[controller getBrowserWrapper] isBusy]);

This uses controller without checking it.  I also think the logic can be cleaned up.

Comment 8

[Chris Lawson (gone)]

Taking, got Torben's blessing via e-mail. Already have a re-worked patch. Will post momentarily.

cl

Comment 9

[Chris Lawson (gone)]

Attachment: more explicit checking and logic

Mark, is this better?

Comment 10

[Torben]

Re attachment id=203092

+  if (controller) {
+    // if we have an empty tab or window, re-use it (bug 295858)
+    BOOL tabOrWindowIsEmpty = ([[controller getBrowserWrapper] isEmpty] &&
                                ![[controller getBrowserWrapper] isBusy]);
+    if (tabOrWindowIsEmpty || reuseWindow > kOpenNewWindowOnAE) {
+      if (reuseWindow == kOpenNewTabOnAE && !tabOrWindowIsEmpty) {
+        [controller openNewTabWithURL:inURLString referrer:aReferrer
                     loadInBackground:loadInBackground allowPopups:NO];
+      }
+      else
+        [controller loadURL:inURLString referrer:nil activate:YES
                     allowPopups:NO];
+      }

As far as I can tell, you end up doing nothing if 
reuseWindow == kOpenNewWindowOnAE (ie, the pref it set to open new windows).

BTW, thanks for taking.

Comment 11

[Chris Lawson (gone)]

Comment on attachment 203092 [details] [diff] [review]
more explicit checking and logic

(In reply to comment #10)
> As far as I can tell, you end up doing nothing if 
> reuseWindow == kOpenNewWindowOnAE (ie, the pref it set to open new windows).

Hah. You're right. Not only that, but it ignores the "New windows and tabs load in background" pref in this case -- if it's set to open a new window from an external app, it loads in the foreground regardless.

Clearing the review flag on that patch. This is gonna require a touch more work. Sorry for the bugspam.

cl

Comment 12

[Chris Lawson (gone)]

Accepting.

Comment 13

[Chris Lawson (gone)]

Attachment: completely re-worked patch with all-new logic

Ahhh...much better.

This one makes a lot more sense, I think you'll agree.

Also, retargeting for 1.0.

cl

Comment 14

[Chris Lawson (gone)]

CCing Mark, since he's the reviewer for this one.

cl

Comment 15

[Stuart Morgan]

Comment on attachment 203159 [details] [diff] [review]
completely re-worked patch with all-new logic

I hope Mark won't mind if I jump in here; I thought I'd review some of the patches sitting around.

+      if (reuseWindow > kOpenNewWindowOnAE) { // either new tabs or reuse front window

|reuseWindow > kOpenNewWindowOnAE| is dirty since it depends on the underlying values that defining the contants should be abstracting away, and it doesn't help since you have to pull out the specific value anyway (I know that was there already, but as long as you are rewriting logic it's a good time to get rid of it).

+    BOOL tabOrWindowIsEmpty = ([[controller getBrowserWrapper] isEmpty] && ![[controller getBrowserWrapper] isBusy]);

Since the variable is isEmpty && isBusy, don't call it fooIsEmpty.  Maybe fooIsAvailable or something.

And in general, there's still some convoluted logic here--things like checking for emptiness around the pref even when the pref is set to re-use the window, and duplicating the re-use-the-window code.

My preference for the section inside the |if (controller)| would be:
fooIsAvailable = ...
if (fooIsAvailable || reuseWindow == kReuseWindowOnAE)
  <re-use>
else if (reuseWindow == kOpenNewTabOnAE)
  <new tab>
else
  <new window>

Comment 16

[Chris Lawson (gone)]

Attachment: redone at the last minute at great expense

Comment 17

[Chris Lawson (gone)]

Attachment: the people submitting the previous patch have been sacked

Damn. Had two dangling { in the code there. Ignore that previous patch; this one actually works.

cl

Comment 18

[Stuart Morgan]

Comment on attachment 204147 [details] [diff] [review]
the people submitting the previous patch have been sacked

Much better--it's almost there.  However this patch regresses bug 167245 in the new window case because
[[[controller getBrowserWrapper] getBrowserView] setActive:YES];
is still using the old window's BWC, so it pulls the old window back to the font.  You'll need to acquire the new controller before then.

One other note for the next version: most of the comments read just like the code they are commenting. I think the logic is now clear enough that they should just be removed.  

Also, it looks like you edited the patch file, since I had to add back two missing context lines at the end before it would apply; please make sure patches still apply to clean source if you edit them.

Comment 19

[Chris Lawson (gone)]

Attachment: re-use version 2

I decided checking controller three times was less ugly than duplicating the sketchy method call on line 1117.

http://lxr.mozilla.org/mozilla/source/camino/src/application/MainController.mm#1117

cl

Comment 20

[Stuart Morgan]

Hmm.  I'm not sure that's is the right direction to go, since there's a comment in the code explicitly saying that BWC's openNewWindowWithURL is the preferred method to use, and there's a perfectly good BWC you can use to call it along that codepatch. It appears that at the least the window-size-matching won't be guaranteed for the new window that way, and there may be other differences.

The options I see are:
a) use this patch, making more use of method that's commented as being a bad way to go
b) tweak the previous version of the patch to re-set controller based on the new frontmost window
c) make openBrowserWindowWithURL return the new controller, so you can re-set it directly

I was initially thinking of b), but unless there is some reason c) would be a bad idea that seems like a better change.

I'm far from understanding the interplay of the various window-opening methods though.  Pink, Simon, any thoughts?

Comment 21

[Stuart Morgan]

Sorry, c) would be changing openNewWindowWithURL (in BWC) not openBrowserWindowWithURL (in MC).

Comment 22

[Stuart Morgan]

Comment on attachment 204193 [details] [diff] [review]
re-use version 2

I really don't think we want the extra use of the discouraged method.

Chris, can you roll a version b) patch (based on the "have been sacked" patch)?  That's a less invasive change than c), and since this isn't a high-traffic method the slight efficiency hit probably isn't worth making the change at this point.

Comment 23

[Chris Lawson (gone)]

Yeah, I'll take a look at this this afternoon.

cl

Comment 24

[Chris Lawson (gone)]

Attachment: Updated per Stuart's comments

Simon said he'd review.

This complies with Stuart's comment 18 by removing the offending line entirely. I've tested in every instance I can think of and have found no ill effects of removing that line.

This also fixes two missing casts that Simon pointed out, and a few minor whitespace errors in the file.

cl

Comment 25

[Simon Fraser [no longer active]]

Fixed on branch and trunk.

Comment 26

[Chris Lawson (gone)]

The check-in for this caused a new bug -- if a non-browser window (Downloads Manager, etc.) is frontmost when Camino receives an external link request, the non-browser window will be left in the foreground as the link loads in the (background) browser window.

cl

Comment 27

[Stuart Morgan]

That's why I didn't want to review the last version of the patch--I figured since Simon wrote that line he'd know whether it was necessary or not ;)

Comment 28

[Torben]

Re attachment id=204983

+  BOOL tabOrWindowIsAvailable = (controller && [[controller getBrowserWrapper]
   isEmpty] && ![[controller getBrowserWrapper] isBusy]);
+
+  if (controller) {
+    BOOL tabOrWindowIsAvailable = ([[controller getBrowserWrapper] isEmpty] &&
    ![[controller getBrowserWrapper] isBusy]);

Is there any reason to declare this Boolean twice? Maybe fix his while you are fixing bug 319257?

Comment 29

[Chris Lawson (gone)]

(In reply to comment #28)

> Is there any reason to declare this Boolean twice?

None at all, other than that I must have overlooked it. I just uploaded a fresh patch to bug 319257 that fixes this problem.

cl