606678 - use openLinkIn in nsContextMenu ("Open Link in New Tab" shouldn't add tabs to popups)

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

Follow-up to bug 586234.

STR: Load attachment 464766 [details], open the popup, right click the link and choose "Open Link in New Tab".

The return values for openNewTabWith and openNewWindowWith have been added in bug 423833 for browser_bug423833.js. That test is disabled, but I enabled it to verify that it works with my modifications.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 485502 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>-  urlSecurityCheck(href, doc.nodePrincipal);
>-  openLinkIn(href, where, { fromContent: true,
>+  openLinkIn(href, where, { nodePrincipal: doc.nodePrincipal,

I don't really like the idea of moving the securityCheck call into openLinkIn and making it dependent on the nodePrincipal argument - it kind of obfuscates the fact that a security check is being done. Someone copying a openLinkIn call might not realize that the nodePrincipal argument is required for security, whereas copying a "urlSecurityCheck" call makes that more obvious. It's also somewhat confusing that the nodePrincipal argument affects the fromContent behavior. I would prefer keeping this pattern of calling urlSecurityCheck() explicitly before openLinkIn, where needed.

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js

>   openLinkInCurrent: function() {
>   openFrameInTab: function() {
>   openFrame: function() {

These probably all want to pass in charset too.

> function openNewTabWith(aURL, aDocument, aPostData, aEvent,

>-  // As in openNewWindowWith(), we want to pass the charset of the
>-  // current document over to a new tab. 
>-  var wintype = document.documentElement.getAttribute("windowtype");
>-  var originCharset;
>-  if (wintype == "navigator:browser")
>-    originCharset = window.content.document.characterSet;

This code was kind of bogus, but removing it will have an effect on callers. For example, newTabButtonObserver.onDrop in browser.js probably now wants to call openLinkIn and pass aEvent.dataTransfer.mozSourceNode.ownerDocument.characterSet (with a null check on mozSourceNode) as aCharset, and openFrameInTab wants to pass in doc.characterSet. Might be best to just leave this in if we plan to deprecate openNewTabWith anyways... Same comment applies to openNewWindowWith.

>-function openNewWindowWith(aURL, aDocument, aPostData, aAllowThirdPartyFixup,
>-                           aReferrer)

>-  var referrerURI = aDocument ? aDocument.documentURIObject : aReferrer;

You kept this prioritization for openNewTabWith, but didn't here - that seems wrong.

These methods have returned the tab/window since Firefox 3, so I'd rather not remove that. There's no real harm in extending that behavior to openLinkIn and passing it through here, is there?

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #1)
> These methods have returned the tab/window since Firefox 3, so I'd rather not
> remove that. There's no real harm in extending that behavior to openLinkIn and
> passing it through here, is there?

It would sometimes return a tab and sometimes a window, depending on whether a new window had to be opened. This seems to be asking for trouble.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

I don't see that as particularly problematic...

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch v2

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch v2

forgot to remove fromContent in browser.js

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #3)
> I don't see that as particularly problematic...

The problem is that the caller may not realize it and assume the return value to always be a tab when where=="tab".

Nobody seems the have asked for the return values. They were added for a now-disabled test that doesn't really need them.

Comment 7

[ithinc]

There seems to be a coding error in openLinkIn. Hope it being fixed with this.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#200

200   var w = getTopWin();
201   if ((where == "tab" || where == "tabshifted") &&
202       w.document.documentElement.getAttribute("chromehidden")) {
203     w = getTopWin(true);
204     aRelatedToCurrent = false;
205   }
206 
207   if (!w || where == "window") {

w is used before !w is checked.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

I included a fix for that in my patch for bug 610203.

Comment 9

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/c84a2abbc663

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre) Gecko/20101227 Firefox/4.0b9pre ID:20101227030354

Comment 12

[Anthony Lieuallen]

> Nobody seems the have asked for the return values. They were added for a now-disabled test that doesn't really need them.

I know I'm a bit late to the party, but a bug in Greasemonkey has been caused by this change.  Please advise where I should go if commenting on this bug is the wrong thing to be be doing.

In short, we provide a method GM_openInTab() which opens a tab, and we want to return a handle to the content window thus created (like what window.open would do in content).  Previously, for this, we relied on the return value of openNewTabWith, which I've now discovered has disappeared.

Any suggestions on how best to retain this feature?

Comment 13

[Dão Gottwald [:dao]]

You can do what openNewTabWith did and call gBrowser.loadOneTab or gBrowser.addTab. However, then you have a problem with popups (see bug 641157 for some context). You can try to find a non-popup browser window, add the tab there, and pass through the returned tab. If no such window is around, you need to open a new window, but then GM_openInTab can't return the tab.

Comment 14

[ramons]

Comment #1 in Bug 626365 states that current behavior is by design and points to this bug for explanation. Unfortunately, I cannot follow the reasoning (not a programmer, work in QA/UX design).
Once the fix is in a release, will this reinstate FF3 behavior in regards to opening a new tab within a popup/secondary window? Only asking as this ends up to be a keep/toss decision in regards to FF4.