181007 - cmd-[double-]click to open history link in new window fails

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[sairuh (rarely reading bugmail)]

found while testing bug 164990, using 2002.11.19.11 on 10.2.2.

0. make sure you have the Navigation > Tabbed Browsing pref "clicking a link
with the Command key opens a new window" enabled.
1. cmd-click a bookmark in the toolbar, or cmd-double-click a bookmark in the
sidebar.

results: the link is opened in the current tab (or window, if you don't have
multiple tabs in view).

expected: the link should be opened in a new window.

fortunately, cmd-clicking a link in the web page content still opens the link in
a new window.

Comment 1

[Simon Fraser [no longer active]]

*** Bug 181997 has been marked as a duplicate of this bug. ***

Comment 2

[Jasper]

WFM with the 20040101 NB. 

Probably due to changes made in the new bookmarks manager code by David Haas Bug
212630 ?

Comment 3

[S Woodside]

works in new bm manager but not in history

Comment 4

[Mike Pinkerton (not reading bugmail)]

cmd-click in the toolbar when the pref is set to open in a new window fails.
works if the pref is set to open in a new tab. odd.

probably something that can fall off the list for 0.8 if we run out of time.

Comment 5

[Mike Pinkerton (not reading bugmail)]

not fixed by work done in 200378. want to take a look smfr?

Comment 6

[Mike Pinkerton (not reading bugmail)]

off to 0.9

Comment 7

[Julian Pellico]

Simon: this doesn't look too hard. Me take?

Command-click in toolbar now WFM

Looks like just the history isn't working. Also looks like the history doesn't
obey the "opens in new tab" option -- just opens in the active tab.

Comment 8

[Julian Pellico]

Attachment: check for Command key in history view

How's this?

So, now that I have a patch, how do I proceed?

Comment 9

[Simon Fraser [no longer active]]

Seek reviews from me and pink.

Comment 10

[Julian Pellico]

Comment on attachment 160299 [details] [diff] [review]
check for Command key in history view

I would reassign this to myself, but it looks like I don't have the priviledge
yet.

Comment 11

[Geoff "temporarily off bugmail" Beier]

This looks good except for a minor formatting issue: I've been informed that the
preferred style for Camino is not to have braces on conditional statements that
contain only one line. So

+    if (![item isKindOfClass: [HistoryItem class]]) {
+      return;
+    }

should be

+    if (![item isKindOfClass: [HistoryItem class]])
+      return;

Ditto for:

+      if ([[PreferenceManager sharedInstance]
getBooleanPref:"browser.tabs.opentabfor.middleclick" withSuccess:NULL]) {
+        [mBrowserWindowController openNewTabWithURL:url referrer:nil
loadInBackground:loadInBackground];
+      }
+      else {
+        [mBrowserWindowController openNewWindowWithURL:url referrer: nil
loadInBackground:loadInBackground];  
+      }

Otherwise it looks good and works as advertised, so if formatting is fixed,
r=[e-mail].

Comment 12

[Julian Pellico]

OK - though it's not my preferred style.

Comment 13

[Julian Pellico]

Attachment: fixed braces style

Comment 14

[Mike Pinkerton (not reading bugmail)]

don't we have this code that checks the key and opens in a new window or tab
already somewhere in the BWC? Can't we make a central access point that handles
all this for anyone loading a url?

Comment 15

[Mike Pinkerton (not reading bugmail)]

cc'ing geoff to see if he knows the answer to the quetion i asked.

Comment 16

[Simon Fraser [no longer active]]

I think I had a bottleneck that handled modifier keys when opening bookmarks;
not sure if we can reuse/refactor for history.

Comment 17

[Julian Pellico]

The BWC doesn't quite have this logic from what I see -- there's something in
the MainController [loadBookmarkLwithWindowController:blah], but it expects to
take a BookmarkItem. I basically imitated the logic there.

Comment 18

[Geoff "temporarily off bugmail" Beier]

(In reply to comment #14)
> don't we have this code that checks the key and opens in a new window or tab
> already somewhere in the BWC? Can't we make a central access point that handles
> all this for anyone loading a url?

The code in BWC doesn't check the key... it simply allows callers who've already
checked the key and prefs to specify whether a url or search term should be
opened in a new tab or window and in the foreground or background.

I would perhaps be leery of checking the key where the URL is loaded because:

1. Command key during a drag may imply slightly different behavior than command
key during text entry.
2. The URL load *may* just be far enough separated from the event handling that
if we wait until the URL load to check the key the user could have released it
by then, and we'd get difficult-to-reproduce behavior.

IMO the modifier key status should be checked as close to the event handling as
possible.

Comment 19

[Julian Pellico]

I got it

So what's going on with the other review here? Is sfrase gonna do it, or should
I reassign to someone else?

Comment 20

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 160600 [details] [diff] [review]
fixed braces style

sr=pink

Comment 21

[Mike Pinkerton (not reading bugmail)]

landed