273228 - remove calls to newTabsAllowed

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[Ludovic Hirlimann [:Usul]]

With some recent changes in our tree we do not need to check if we can create a
new tab in the present window. I assume this code was left so we could set a
limit to the number of tabs open. 
However it seems to me that the new tab code does handle correctly many tabs
open in a window. Hence there are lots of place where we check if we can open
new tabs before doing so. I would like to remove all these checkings and the
code that goes with it. This would speed up some of our interface and would
decrease our code size. Opening this bug so we can discuss the issue. Feel free
to wontfix or tell me to shut up.
Waiting for your input before starting the work.

Comment 1

[Ludovic Hirlimann [:Usul]]

I'll do this for 0.9

Comment 2

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

Ludovic, would the code you mention be useful for preventing opening tabs in,
say, a popup-window created via javascript and which has no toolbar, etc.?  If
someone left that window active/frontmost and then clicked on a link in Mail and
has Camino set to open links from external apps in a new tab in the active
window?  Or even if someone cmd-clicks a link in such a popup-window?

I know nothing at all about code, so just wondering aloud here.

Comment 3

[Josh Aas]

If this turns out to be a good idea, go ahead and fix it whenever but it doesn't
need to be marked 0.9 in order for it to get fixed. This would not block 0.9,
which is what the list is for.

Comment 4

[Ludovic Hirlimann [:Usul]]

Attachment: patch removing unecessary code

Comment 5

[Ludovic Hirlimann [:Usul]]

Comment on attachment 168050 [details] [diff] [review]
patch removing unecessary code

asking review.

Comment 6

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #5)
This generally looks good. The only problem I see is these two hunks:

@@ -348,7 +332,7 @@
   if (overTabViewItem)
     return NSDragOperationNone;	// the tab will handle it
 
-  if (!overContentArea && ![self canMakeNewTabs])
+  if (!overContentArea)
     return NSDragOperationNone;
   
   [self showDragDestinationIndicator];	// XXX optimize
@@ -364,7 +348,7 @@
   if (overTabViewItem)
     return NSDragOperationNone;	// the tab will handle it
 
-  if (!overContentArea && ![self canMakeNewTabs])
+  if (!overContentArea)
   {
     [self hideDragDestinationIndicator];
     return NSDragOperationNone;


These two blocks should be elimintated now that we can always make new tabs.

Also, the patch has rotted. That is my fault for having it slip under my radar
for review. (sorry... *very* busy month!!) I'm re-rolling the patch with these
changes, which are what it takes to get r+ from me.

Comment 7

[Geoff "temporarily off bugmail" Beier]

Also, in this hunk:

@@ -2826,10 +2813,7 @@
 
   NSString* referrer = [[mBrowserView getBrowserView] getFocusedURLString];
 
-  if (aUseWindow || ![self newTabsAllowed])
-    [self openNewWindowWithURL: hrefStr referrer:referrer loadInBackground:
loadInBackground];
-  else
-    [self openNewTabWithURL: hrefStr referrer:referrer loadInBackground:
loadInBackground];
+  [self openNewWindowWithURL: hrefStr referrer:referrer loadInBackground:
loadInBackground];
 }

we still want to do different things based on aUseWindow. I'm including that in
my respun patch.

Comment 8

[Geoff "temporarily off bugmail" Beier]

Attachment: updated to match current cvs tree, incorporating my review comments

this is ludo's patch, re-rolled to match current cvs (since it's my fault that
it rotted) and also incorporating my review comments. i'm flagging r+ 'cause
it's really ludo's.

r=[e-mail]

Comment 9

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 171598 [details] [diff] [review]
updated to match current cvs tree, incorporating my review comments

sr=pink

Comment 10

[Josh Aas]

landed on trunk