Closed Bug 606678 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 4.0b9
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dao, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #485502 - Flags: review?(gavin.sharp)
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?
Attachment #485502 - Flags: review?(gavin.sharp) → review-
blocking2.0: --- → ?
(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.
I don't see that as particularly problematic...
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #485502 - Attachment is obsolete: true
Attachment #485823 - Flags: review?(gavin.sharp)
Attached patch patch v2Splinter Review
forgot to remove fromContent in browser.js
Attachment #485823 - Attachment is obsolete: true
Attachment #485825 - Flags: review?(gavin.sharp)
Attachment #485823 - Flags: review?(gavin.sharp)
(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.
blocking2.0: ? → betaN+
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.
Attachment #485825 - Flags: review?(gavin.sharp) → review+
I included a fix for that in my patch for bug 610203.
http://hg.mozilla.org/mozilla-central/rev/c84a2abbc663
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b9
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9pre) Gecko/20101227 Firefox/4.0b9pre ID:20101227030354
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: in-litmus-
Duplicate of this bug: 621968
> 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?
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 #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.
You need to log in before you can comment on or make changes to this bug.