Last Comment Bug 323810 - [FIXr]Move forcing into tabs and current windows out of Gecko
: [FIXr]Move forcing into tabs and current windows out of Gecko
Status: RESOLVED FIXED
For docs, see comment 13 and comment 14
: dev-doc-needed, fixed1.8.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz]
: Hixie (not reading bugmail)
Mentors:
Depends on: 323970 342504 344257 380383
Blocks: 277972 184994 caminoSWM 325816 326006 326009 326291 327098
  Show dependency treegraph
 
Reported: 2006-01-17 13:57 PST by Boris Zbarsky [:bz]
Modified: 2009-02-17 18:53 PST (History)
37 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
My summary of what the code looks like before my changes (2.29 KB, text/plain)
2006-01-17 21:26 PST, Boris Zbarsky [:bz]
no flags Details
Thoughts on some tests to run (1.80 KB, text/plain)
2006-01-17 21:28 PST, Boris Zbarsky [:bz]
no flags Details
Proposed patch (118.68 KB, patch)
2006-01-17 22:07 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Updated per discussion with jst (112.64 KB, patch)
2006-01-20 22:57 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Same as previous as diff -w for ease of review (110.61 KB, patch)
2006-01-20 23:06 PST, Boris Zbarsky [:bz]
jst: superreview+
Details | Diff | Splinter Review
Updated to comments (112.21 KB, patch)
2006-01-26 20:47 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Same as diff -w (110.18 KB, patch)
2006-01-26 20:48 PST, Boris Zbarsky [:bz]
benjamin: review+
jst: superreview+
Details | Diff | Splinter Review
Updated to comments (111.96 KB, patch)
2006-02-07 12:47 PST, Boris Zbarsky [:bz]
jst: approval‑branch‑1.8.1+
Details | Diff | Splinter Review
Merged to 1.8 branch (111.75 KB, patch)
2006-02-13 21:29 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
Screenshot of a failing return window.openDialog(...) (4.04 KB, image/png)
2006-03-06 12:50 PST, HJ
no flags Details

Description Boris Zbarsky [:bz] 2006-01-17 13:57:32 PST
Right now we have code in both docshell and nsGlobalWindow to handle the "browser.link.open_newwindow" preference.  But this isn't really a Gecko preference.  Not only that, but the code as written really only works well for XUL apps, since there are assumptions about there being a toplevel DOM window that has a browserDOMWindow set on it.  In an embedding app that assumption is simply false.

So I've been working on a patch that consolidates the code we have and makes it more embedding-friendly.  First draft should be coming up soon.
Comment 1 Worcester12345 2006-01-17 19:07:40 PST
possibly related?

Bugzilla Bug 121377
Bugzilla Bug 172962
Bugzilla Bug 226826
Bugzilla Bug 227241
Bugzilla Bug 262575
Comment 2 Boris Zbarsky [:bz] 2006-01-17 19:20:10 PST
Some of those are (and in fact are in the dep tree of this bug).  Some are not.  Really doesn't matter either way, though.
Comment 3 Boris Zbarsky [:bz] 2006-01-17 21:26:37 PST
Created attachment 208824 [details]
My summary of what the code looks like before my changes
Comment 4 Boris Zbarsky [:bz] 2006-01-17 21:28:58 PST
Created attachment 208825 [details]
Thoughts on some tests to run
Comment 5 Boris Zbarsky [:bz] 2006-01-17 22:07:33 PST
Created attachment 208830 [details] [diff] [review]
Proposed patch

Starting with review on danm and jst, though I'd really like feedback on the embedding API I'm suggesting here from anyone who has a minute to look (darin, bsmedberg, smfr, pinkerton, biesi?).  The new API is nsIWindowProvider.idl; I'm interested in feedback on the API itself as well as in feedback on whether embedding/base is the right place to put it.  Also, I'm not sure where I can document that the window watcher assumes it can getInterface this sucker off a tree owner... I think we should document that somewhere, but that's not really a property of the interface.

For review ease, here's a summary of the changes and the reasons for them (where they are non-obvious), more or less in patch order:

1)  Better documentation for nsIBrowserDOMWindow
2)  Simplification of OpenAllowValue to not have allowSelf and allowExtant,
    since I think that any time an existing window is being targeted we have a
    non-popup by definition and shouldn't be doing popup stuff with it.
3)  GetOpenAllow() was only used by docshell and is now unused; I didn't remove
    it only to avoid changing nsPIDOMWindow on the branch.  On trunk we should
    remove it.
4)  Extend nsPIDOMWindow with an OpenNoSearch method that allows docshell to
    create new named windows without having the nsGlobalWindow or
    nsWindowWatcher code search (incorrectly) for them.  I couldn't use the
    window watcher directly in nsDocShell because moving the popup blocking to
    the window watcher would be an behavior change we don't want at all.  At the 
    same time, nsDocShell should fully participate in popup blocking.  So I
    either had to add a method like this or add APIs on nsPIDOMWindow to allow
    the docshell to participate in popup blocking; adding the OpenNoSearch
    method seemed to reuse more code.
5)  Extend nsPIWindowWatcher with three methods.  One is a secure version of
    getWindowByName.  This wasn't strictly needed (there was identical code to
    do the job in both nsWindowWatcher and nsGlobalWindow and I could have left
    it), but I think that we want to actually make this method public on a
    frozen interface like nsIWindowWatcher2, so I figured I'd put it in.  The
    other two methods do what openWindow and openWindowJS do, but allow the
    caller to pass in an existing docshell to to the load into and don't do a
    search by name of their own (see item 4 for why this was needed;
    nsGlobalWindow could live with us just searching twice every time like we
    used to, but for nsDocShell it's wrong, not just slow).
6)  Create an nsIWindowProvider interface that the window watcher can use to do
    things like divert open calls to new tabs, etc.
7)  Remove a bunch of targeting code from nsDocShell.  Now it just searches for
    a named item, and calls OpenNoSearch if it doesn't find it.  The charset
    munging here was consolidated with similar code in the window watcher.
8)  Remove checks of the "dom.disable_open_during_load" from XLink handling.
    Just handling those as normal links works fine now that we properly do popup
    control for links.
9)  Consolidate some copy/pasted popup-blocking stuff in nsGlobalWindow methods
    under OpenInternal.
10) Fix buglet in our popup spam window counter that was introduced by
    splitwindow -- we were decrementing the count for every inner of a popup
    outer that got destroyed (eg by page traversals).  I ran into this while
    testing this patch.  We might conceivably want this one bit on the 1.8.0.x
    branch...
11) Simplify the popup blocking stuff now that allowSelf and allowExtant are no
    longer relevant and now that we know we always have a docshell when we call
    it. 
12) Remove a bunch of targeting/diverting code from
    nsGlobalWindow::OpenInternal.
13) Implement the new methods in nsWindowWatcher.
14) Make nsWindowWatcher::OpenWindowJSInternal try to get a window from an
    nsIWindowProvider before opening one, but only in limited cases that I
    decided all nsIWindowProvider consumers would need checks for otherwise.
    All "normal" content-triggered window opens will get passed to the
    nsIWindowProvider; the cases blocked out here are those used by various
    chrome stuff.
15) Move the parsing of the window size and location spec in nsWindowWatcher
    into a separate method so that I can pass the args I want to
    nsIWindowProvider.
16) Make the charset munging handle both cases it needs to handle.
17) Make the warning-only nsWindowWatcher::CheckWindowName #ifdef DEBUG
18) Implement nsIWindowProvider on nsContentTreeOwner
19) Fix bug in seamonkey's nsIBrowserDOMWindow impl that crept in sometime when
    people were messing with the XPCNativeWrapper() stuff.

Things I tried very hard NOT to change:

1) Behavior of existing nsIWindowWatcher and nsPIWindowWatcher methods (though
   see more on this below).
2) Net result of parsing of the size and position spec in the window watcher.

Things that I'm a little concerned about:

1) This patch makes nsGlobalWindow and nsDocShell require the new interface I
   added to open new windows.  That means that if someone is overriding
   nsWindowWatcher by contract ID they will break (as in, no windows will get
   opened).  I considered getting the window watcher by CID in nsGlobalWindow,
   but that would just mean that the overrider's impl is not called at all; that
   would likely be more difficult to detect, and impossible to fix (while fixing
   the contract ID issue just involves implementing the new interface).  I tried
   hard to think of a way to not need this interface, but doing that would have
   required copy/pasting code out of the window watcher into nsDocShell (name
   handling, to be exact), and opening a window for targeted links with a null
   name to start with... I suppose I could do that, but this seemed better in
   terms of doing things in one well-specified place that's responsible for
   handling them.
2) This patch changes the behavior of the nsIWindowWatcher and nsPIWindowWatcher
   methods for cases when there is an nsIWindowProvider.  Given that the API
   already lies (the claim of "create a new window" is false if a name is passed
   in and a window with that name already exists), I decided that this doesn't
   make it lie any more.  Since the consumer of nsIWindowWatcher is generally
   exactly the same entity (embeddor, Firefox, whatever) that implements
   nsIWindowProvider, I _think_ this is ok.
3) The OPEN_CURRENTWINDOW case has some issues (eg the charset munging is wrong
   for it, since it'll use the new document, not the old one).  I'd like to
   address this in a followup patch.
Comment 6 Boris Zbarsky [:bz] 2006-01-18 11:49:00 PST
I thought about this some more, and I guess I could eliminate the API changes if I were to open windows from docshell with an empty name and then reset the name.  I'm not sure that's a happy thing to do, but if it is we could bite the bullet on the two redundant window name searches in global window and window watcher, move some of the charset-setting code back into docshell, and duplicate the name setting between docshell and window watcher.  That would mean we could get rid of all the changes to nsPIDOMWindow and nsPIWindowWatcher that I made.

Thoughts?
Comment 7 neil@parkwaycc.co.uk 2006-01-18 17:01:38 PST
Comment on attachment 208830 [details] [diff] [review]
Proposed patch

>+        aOpener = aOpener.top;
bz: thanks for spotting this; would you mind filing a separate bug for it so it can land on the branches too?
Comment 8 Boris Zbarsky [:bz] 2006-01-18 21:32:26 PST
Another possibility would be restoring the code to much what it looked like before and manually pushing a null JSContext on the stack in docshell...
Comment 9 Boris Zbarsky [:bz] 2006-01-20 22:57:04 PST
Created attachment 209182 [details] [diff] [review]
Updated per discussion with jst

I eliminated items 4, 5, and 13 from comment 5.  Now the scriptable window methods assume they're called from script, while the non-scriptable ones assume that any script on the stack is completely unrelated to them.  In the window watcher, we still examine the JS stack for the caller, but OpenWindow and OpenWindowJS now have slightly different charset-munging behavior....  I also decided that using the parent's charset for the window.open() case was wrong and switched to using the caller's charset (caller may or may not be parent).

nsGlobalWindow::OpenInternal now always calls OpenWindow on the window watcher when it was called via the noscript methods and always calls OpenWindowJS when it was called via the scriptable methods.

Item 19 from comment 5 is no longer relevant because that change has been checked in.

This patch eliminates concern #1 I had in comment 5.
Comment 10 Boris Zbarsky [:bz] 2006-01-20 23:06:19 PST
Created attachment 209183 [details] [diff] [review]
Same as previous as diff -w for ease of review

I've tested this and all that.  Should probably be banged on some more, though.

Again, I'd love feedback on these interface comments and the new nsIWindowProvider interface from anyone who cares to comment!
Comment 11 Simon Fraser 2006-01-21 09:30:47 PST
So in the embedding space, the tree owner is nsDocShellTreeOwner, right? Would would something like Camino customize the nsIWindowProvider that nsDocShellTreeOwner supplies?
Comment 12 Simon Fraser 2006-01-21 09:43:21 PST
(In reply to comment #11)
> Would would something like Camino...

"How would"...
Comment 13 Boris Zbarsky [:bz] 2006-01-21 09:51:19 PST
Yes, in embedding the tree owner is nsDocShellTreeOwner.  When we call GetInterface on it to get the window provider, it will call GetOwnerRequestor() and call GetInterface() on that.  That effectively QIs the thing that was passed to SetWebBrowserChrome to nsIInterfaceRequestor and then calls GetInterface on that.

Put another way, whatever way it is you provide an nsIEmbeddingSiteWindow is how you can provide an nsIWindowProvider (either via QI or GetInterface on that object).

I'd like to document all this somewhere; I'm just not sure where...
Comment 14 Boris Zbarsky [:bz] 2006-01-22 09:58:49 PST
One other documentation bit... the window provider is expected to handle calling addWindow() on the watcher as needed.  Thing is, this is done automatically by the embedding glue (nsWebBrowser to be exact).  And in XUL apps it's done by the XPFE window creator.  So I'd rather not say anything about it on nsIWindowProvider, especially since addWindow() is on nsPIWindowWatcher.
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2006-01-24 18:10:24 PST
Comment on attachment 209183 [details] [diff] [review]
Same as previous as diff -w for ease of review

- In nsPIWindowWatcher.idl:

+// XXXbz that IID doesn't match the actual nsPIWindowWatcher IID
+// anymore.  _And_ it seems to be unused.  Should we remove it?
 %{C++
 // {d535806e-afaf-47d1-8d89-783ad088c62a}
 #define NS_PWINDOWWATCHER_IID \
  {0xd535806e, 0xafaf, 0x47d1, {0x8d, 0x89, 0x78, 0x3a, 0xd0, 0x88, 0xc6, 0x2a}}
 %}

Yes, please do remove it. I can't see anything good coming from leaving this in here.

-In dom/src/base/nsGlobalWindow.h:

+   * @param argv The arguments to pass to the new window the first
+   *             three args, if present, will be aURL, aName, and aOptions.  So
+   *             this param only matters if there are more than 3 arguments.

Fix the language in the first sentence.

- In nsGlobalWindow::OpenInternal():

+    // XXXbz should this check !aCalledNoScript instead of aDoJSFixups?  That
+    // is, is this relevant for openDialog calls from script?

I don't think it is relevant as only chrome can call openDialog() and all we're doing here is to set the opener script URL, which doesn't matter for chrome AFAIK.

+        nsCOMPtr<nsIDOMDocument> doc;
+        (*aReturn)->GetDocument(getter_AddRefs(doc));

Maybe call nsPIDOMWindow::EnsureInnerWindow() here? Though it may not be worth the extra QI...

More later...
Comment 16 Johnny Stenback (:jst, jst@mozilla.com) 2006-01-25 15:37:24 PST
Comment on attachment 209183 [details] [diff] [review]
Same as previous as diff -w for ease of review

- In nsWindowWatcher::CalcSizeSpec():

+  if (present)
+    aResult.mLeftSpecified = PR_TRUE;

There's a couple of instances of this, and not that it's a big deal or anything, but you could simply avoid the if-branch here in favor of always assigning aResult.mLeftSpecified to present. A bit less code, maybe...

+      aResult.mInnerHeight =  temp;

Extra space before temp (in a couple of places).

Other than that this looks really good to me. Nice cleanup of how we deal with tabs, and gives embedding apps a chance to play well here too. The new APIs look fine to me. sr=jst
Comment 17 Mike Pinkerton (not reading bugmail) 2006-01-26 11:02:34 PST
/me hugs bz :D
Comment 18 Boris Zbarsky [:bz] 2006-01-26 20:42:26 PST
> Maybe call nsPIDOMWindow::EnsureInnerWindow() here? 

I considered that, but we're really ensureing the document, not the window, so I'm leaving it as-is for now.

> but you could simply avoid the if-branch here

Good catch.  I also collapsed the width/innerWidth and height/outerHeight branches together, since they had the same code.  Mind checking that over just in case?
Comment 19 Boris Zbarsky [:bz] 2006-01-26 20:47:41 PST
Created attachment 209800 [details] [diff] [review]
Updated to comments
Comment 20 Boris Zbarsky [:bz] 2006-01-26 20:48:22 PST
Created attachment 209801 [details] [diff] [review]
Same as diff -w
Comment 21 Johnny Stenback (:jst, jst@mozilla.com) 2006-01-30 17:16:19 PST
Comment on attachment 209801 [details] [diff] [review]
Same as diff -w

Yeah, looks good. sr=jst
Comment 22 Boris Zbarsky [:bz] 2006-02-01 11:45:21 PST
Comment on attachment 209801 [details] [diff] [review]
Same as diff -w

Dan says he won't be able to review this... Peter, do you think you can?  If not, let me know and I'll keep looking, ok?
Comment 23 Boris Zbarsky [:bz] 2006-02-05 13:11:38 PST
I have one other thought on what nsIWindowProvider should do, but I'll do that in bug 326006 as a separate patch....  This patch is already big enough.  :)
Comment 24 Benjamin Smedberg [:bsmedberg] 2006-02-07 06:28:39 PST
in nsGlobalWindow::WindowExists... I know that !!namedItem is old code, but could we use something more readable like namedItem != nsnull?

in nsGlobalWindow::OpenInternal... you have NS_ENSURE_TRUE(wwatch, rv); and then there's an unneeded if-branch on wwatch which can/should be removed.

    // Now check whether it's ok to ask a window provider for a window.  Don't
    // do it if we're opening a dialog or if our parent is a chrome window or

Why the check for "parent is a chrome window"? Can't chrome windows open content windows? I think that the extension manager does, for example, and there is a bug to make it obey the tabbed-browsing prefs.

    newDocShellItem->SetName(nameSpecified &&
                             !name.LowerCaseEqualsLiteral("_blank") &&
                             !name.LowerCaseEqualsLiteral("_new") ?
                             name.get() : nsnull);

We really want to call SetName(nsnull)? What does this mean in practice (and can we document that on nsIDocShellTreeItem)?
Comment 25 Benjamin Smedberg [:bsmedberg] 2006-02-07 06:30:22 PST
Comment on attachment 209801 [details] [diff] [review]
Same as diff -w

r=me with nits and additional comments as noted
Comment 26 Boris Zbarsky [:bz] 2006-02-07 09:34:23 PST
> Why the check for "parent is a chrome window"?

Mostly because it was already there (to make it possible for chrome to open a new window if it really wants to; without that check there were bugs where the "get themes" thing in the seamonkey UI would get forced into the preferences window, which is highly undesirable).

That said, the fact that the window provider is hooked up to the _content_ tree means that toplevel chrome windows don't get window provider stuff happening anyway.  This check is only needed for non-toplevel chrome, which gets the content tree owner for reasons I don't understand (and which I suspect are bogus).

> Can't chrome windows open content windows?

Toplevel chrome windows certainly cannot via window.open() on the window, since nsChromeTreeOwner is not a nsIWebBrowserChrome (unlike nsContentTreeOwner).  For non-toplevel chrome, see above; I feel that the current behavior is a bug.

> I think that the extension manager does, for example

If it's opening windows via window.open() then it's opening chrome windows.  If not, how is it doing it?

> and there is a bug to make it obey the tabbed-browsing prefs.

People can't seriously expect opening a content window from the extension manager to open the new thing right in the extension manager window (which is one of the "tabbed-browsing prefs".  I'd need to know more about the extension manager thing to say how it fits in with this setup; any idea where I can find this out?

> We really want to call SetName(nsnull)?

We want to reset to an empty string.  For wstring (which is what we have here), null and EmptyString().get() are generally equivalent unless documented otherwise (and I'm still thinking we should switch to AString here on nsIDocShellTreeItem).  I suppose I could switch to calling the EmptyString() version here if you'd prefer.  Let me know, please?  I hope to check this in this afternoon (it's blocking other work I want to get done), so if you get a chance to comment before then, that would be great.
Comment 27 Benjamin Smedberg [:bsmedberg] 2006-02-07 09:50:24 PST
> People can't seriously expect opening a content window from the extension
> manager to open the new thing right in the extension manager window (which is
> one of the "tabbed-browsing prefs".  I'd need to know more about the extension
> manager thing to say how it fits in with this setup; any idea where I can find
> this out?

Currently we're using http://lxr.mozilla.org/mozilla/source/toolkit/mozapps/extensions/content/extensions.js#88 which unconditionally opens a new window (and is one of those toolkit ifdefs I need to take care of). What I would really like is for this to call window.open(aURL, "_blank", "external=true") or something like that; have the window-opening code check for nsIExternalProtocolService.isExposedProtocol... if the protocol is exposed, open the URL in a new window, "current" window (not the extension manager chrome, but the current browser content window, or new tab obeying preferences. This would also be exposed to web content so that my gmail-as-xulrunner-app can open links in the default browser. But we can definitely leave that for a followup bug/dicussion.

> We want to reset to an empty string.  For wstring (which is what we have here),
> null and EmptyString().get() are generally equivalent unless documented
> otherwise (and I'm still thinking we should switch to AString here on
> nsIDocShellTreeItem).  I suppose I could switch to calling the EmptyString()

I don't think it's a good idea that a null pointer == empty string... although in this case resetting the window to "unnamed" it makes sense to use a null pointer. I guess I was asking why we reset the window name at all?

In the following:
var popup = window.open(aPopupURL, "foopy");

popup.open(aNotherURL, "_self");

Shouldn't the popup window still be "named" foopy?
Comment 28 Boris Zbarsky [:bz] 2006-02-07 11:55:22 PST
> > Can't chrome windows open content windows?

> Toplevel chrome windows certainly cannot via window.open() on the window, since
> nsChromeTreeOwner is not a nsIWebBrowserChrome (unlike nsContentTreeOwner). 

Actually, I was wrong about this.  Chrome windows can open content windows if the CHROME_OPENAS_CHROME flag is not set, I think...

That said, when this happens the window.opener on the content window will be the chrome window... so I really don't think we want to be doing it.  Or we should be doing something smarter with window.opener.  Followup bug ok?

> What I would really like is for this to call window.open(aURL, "_blank",
> "external=true")

Per discussion with Benjamin, this is going in a followup bug.

> I guess I was asking why we reset the window name at all?

We need to do it in the case when we got redirected to an existing window for targets like _blank or _new....

> popup.open(aNotherURL, "_self");
> Shouldn't the popup window still be "named" foopy?

It is.  In this case windowNeedsName is going to be false.  It's only true if we failed to find an existing window with the given name, and for _self we generally find one.
Comment 29 Boris Zbarsky [:bz] 2006-02-07 12:47:29 PST
Created attachment 211047 [details] [diff] [review]
Updated to comments

I just landed this on trunk.
Comment 30 Boris Zbarsky [:bz] 2006-02-07 12:53:46 PST
Filed bug 326291.  This is fixed on trunk.
Comment 31 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-02-07 17:31:28 PST
In the future you might want to run embedding APIs past chpe@gnome.org (Epiphany).
Comment 32 neil@parkwaycc.co.uk 2006-02-08 03:45:22 PST
(In reply to comment #27)
>What I would really like is for this to call window.open(aURL, "_blank", "external=true") or something like that
What I would really like is for chrome docshells not to accept link loads.
That way, you could just write <label><a href="http://themes.mozdev.org/">Get New Themes</a></label> (in XUL) or window.QueryInterface(nsIInterfaceRequestor) .getInterface(nsIWebNavigation).loadURI("http://themes.mozdev.org", nsIWebNavigation.LOAD_FLAGS_IS_LINK, null, null, null);
Comment 33 Boris Zbarsky [:bz] 2006-02-08 07:17:20 PST
> What I would really like is for chrome docshells not to accept link loads.

That sounds like possible nsIURIContentListener2 work.  Comment in that bug?
Comment 34 Bradley Chapman (not reading bugmail, still gone but not forever) 2006-02-10 16:28:03 PST
How does this code affect the tab-related extensions? I'm curious to know if it provides any "wiggle room" (i.e. XPCOM hackery) to extensions for changing the way the code works.

Comment #5 is a bit too verbose on implementation-level details; I'm mainly wondering if it's possible to selectively enforce the tab-prefs on different windows (i.e. popup windows versus normal windows).
Comment 35 Boris Zbarsky [:bz] 2006-02-10 19:01:02 PST
You could probably override the tree owner on the relevant docshells and make it do whatever you wanted.  It'll take some work, but it's doable.
Comment 36 neil@parkwaycc.co.uk 2006-02-11 11:57:18 PST
(In reply to comment #35)
>You could probably override the tree owner on the relevant docshells and make
>it do whatever you wanted.  It'll take some work, but it's doable.
Assuming you're still replying to comment 32, a) why should I have to cut and paste the same code every time b) that doesn't work very well for XBL.
Comment 37 Boris Zbarsky [:bz] 2006-02-11 12:11:29 PST
> Assuming you're still replying to comment 32

I was replying to comment 34 (the one that was right before my comment and all).
Comment 38 Boris Zbarsky [:bz] 2006-02-13 21:29:16 PST
Created attachment 211816 [details] [diff] [review]
Merged to 1.8 branch
Comment 39 Boris Zbarsky [:bz] 2006-02-13 23:02:58 PST
Checked in on the 1.8.1 branch.
Comment 40 HJ 2006-03-06 10:45:31 PST
Yikes, this patch killed a nice MultiZilla feature. I tried to write a workaround for it but ran into the following two problems:

Returning null in openURI() opens a new window and returning aOpener loads the page in the current tab, so how do I stop it from loading anything?

Also, openURI() is only called when 'browser.link.open_newwindow' is set to either 1 or 3 so extension authors can't rely on overriding it. Again, how do we now block new windows from being opened? 
Comment 41 Boris Zbarsky [:bz] 2006-03-06 10:54:12 PST
> Yikes, this patch killed a nice MultiZilla feature.

Interesting.  I'd tried to keep behavior in Firefox/Seamonkey pretty much what it had been.  What changed?

> Returning null in openURI() opens a new window and returning aOpener loads the
> page in the current tab, so how do I stop it from loading anything?

You don't use this API?  This api is for control of where a window.open happens, not for whether the window opens... That was the case before as well, as far as I can see.

> Also, openURI() is only called when 'browser.link.open_newwindow' is set to
> either 1 or 3 so extension authors can't rely on overriding it.

That was the case before as well, no?

> Again, how do we now block new windows from being opened? 

Well, what do you want to do with the load?  Just drop it on the floor?  You could always create a new window (eg a display:none HTML iframe) and return it and then forget about it (remove it from the DOM).  But the load _will_ happen in that window.

Put another way, what behavior are you expecting web content to see?
Comment 42 HJ 2006-03-06 12:48:40 PST
(In reply to comment #41)
> > Yikes, this patch killed a nice MultiZilla feature.
> 
> Interesting.  I'd tried to keep behavior in Firefox/Seamonkey pretty much what
> it had been.  What changed?

MultiZilla overrides window.open() but that is no longer called, but that might not be introduced by this patch but with the previous patch already. That's what you get for being away for months (:

> > Returning null in openURI() opens a new window and returning aOpener loads the
> > page in the current tab, so how do I stop it from loading anything?
> 
> You don't use this API?

Yes I do, but it only works when the pref is set to 1 or 3. For example, it enables me to check permissions and load the page in the current, new tab or when needed in a new window (some banks require this) and the pref settings are only used when there's no permission set.

Oh, and this doesn't work:

case nsIBrowserDOMWindow.OPEN_NEWWINDOW:
return window.openDialog(getBrowserURL(), "_blank", "all,dialog=no", uri, null, referrer);

because it opens a small window without chrome. I will attach a screenshot of it.

> This api is for control of where a window.open
> happens, not for whether the window opens... That was the case before as well,
> as far as I can see.

Ok, so how does one control if new windows are opened or that it should load in a (current) tab if it isn't even called when the pref is set to 0, 2 or anything else?

> > Also, openURI() is only called when 'browser.link.open_newwindow' is set to
> > either 1 or 3 so extension authors can't rely on overriding it.
> 
> That was the case before as well, no?

I'm not 100% sure, and I haven't installed older builds but I will check this later today.

> > Again, how do we now block new windows from being opened? 
> 
> Well, what do you want to do with the load?  Just drop it on the floor? 

Yes, why not?

> You
> could always create a new window (eg a display:none HTML iframe) and return it
> and then forget about it (remove it from the DOM).  But the load _will_ happen
> in that window.

I better find a way good to block windows again, without loading any content that is.

> Put another way, what behavior are you expecting web content to see?

Well, it would been nice if destination 'nowhwere' was implemented, but I guess that won't happen, which is too bad because I don't know how to block the newly windows.
Comment 43 HJ 2006-03-06 12:50:59 PST
Created attachment 214216 [details]
Screenshot of a failing return window.openDialog(...)

Here's that screenshot
Comment 44 Boris Zbarsky [:bz] 2006-03-06 13:08:57 PST
> MultiZilla overrides window.open()

From script?  Or what?  There are plenty of callers of Open() in C++...

> > You don't use this API?

> Yes I do

My point was that this is NOT the api for preventing window opening it's the API for controlling where windows are opened.

> and the pref settings are only used when there's no permission set.

I'm not sure I follow.

> Ok, so how does one control if new windows are opened or that it should load
> in a (current) tab

This is the API for controlling _where_ the window will open.  The window provider implementation in nsIContentTreeOwner checks some prefs that Gecko used to check and then calls OpenURI() if the pref values are right.  If you want to affect things when those pref values are not right, you'll have to use your own window provider -- either rewrite things so that your own treeowner is being used, or something.  Note that before this checkin you couldn't affect what happened if the prefs were not "right".

There is no api for controlling _whether_ a window will open.

> Yes, why not?

Because generally speaking that breaks pages, if they call window.open and expect to get back a window object?

Again, I don't see how you could have gotten the effect you're talking about with the old code, so I'm not sure what this patch had to do with your problems.
Comment 45 HJ 2006-03-06 14:33:07 PST
(In reply to comment #44)
> > MultiZilla overrides window.open()
> 
> From script?  Or what?  There are plenty of callers of Open() in C++...

Yes, from javascript.

> My point was that this is NOT the api for preventing window opening it's the
> API for controlling where windows are opened.

I understand that, but how do I control new windows when nsBrowserAccess.openURI() isn't even called when the pref is set to 0 or 2? That seems like a major problems.

> > and the pref settings are only used when there's no permission set.
> 
> I'm not sure I follow.

I use permissions in MultiZilla like this:

  // 0 = no permission / 1 = open window / 2 = block window / 3 = open in tab
  var permission =  permissionmanager.testPermission(uri, "target");

    switch(permission)
    {
      case 0:
        if (aWhere == nsIBrowserDOMWindow.OPEN_DEFAULTWINDOW) {
          if (aContext == nsIBrowserDOMWindow.OPEN_EXTERNAL) {
            aWhere = pref.getIntPref("browser.link.open_external");
          }
          else {
            aWhere = pref.getIntPref("browser.link.open_newwindow");
          }
        }
        break;
      case 1:
        aWhere = nsIBrowserDOMWindow.OPEN_NEWWINDOW;
        break;
      case 2:
        // aWhere = ???
        break;
      case 3:
        aWhere = nsIBrowserDOMWindow.OPEN_NEWTAB;
        break;
    }

Like I said, this enables people to override the pref settings.

> > Ok, so how does one control if new windows are opened or that it should load
> > in a (current) tab
> 
> This is the API for controlling _where_ the window will open. 

Right, but you can't control _where_ the window will open when the pref is set to 0 or 2 because it doesn't seem to call nsBrowserAccess.openURI().

> The window
> provider implementation in nsIContentTreeOwner checks some prefs that Gecko
> used to check and then calls OpenURI() if the pref values are right.  If you
> want to affect things when those pref values are not right, you'll have to use
> your own window provider -- either rewrite things so that your own treeowner is
> being used, or something.  Note that before this checkin you couldn't affect
> what happened if the prefs were not "right".

You see, one cannot really control _where_ windows are opened, only when the prefs are set 'right', which in this case it doesn't look right to me.

So instead of using something that is partly available, we now have to write our own code, yikes. Also, I can't find nsIContentTreeOwner Where is that file?

> There is no api for controlling _whether_ a window will open.
> 
> > Yes, why not?
> 
> Because generally speaking that breaks pages, if they call window.open and
> expect to get back a window object?

The same thing happens when people block javascript or use content blocker, but that's up to them IMHO.

> Again, I don't see how you could have gotten the effect you're talking about
> with the old code, so I'm not sure what this patch had to do with your
> problems.

Yeah, like I said, it might be introduced by the previous patch.
Comment 46 Boris Zbarsky [:bz] 2006-03-06 14:41:32 PST
> Yes, from javascript.

Then why do any of these changes matter to start with?  If you're replacing the default window.open with your own... what difference does it make what the default window.open does?

> but how do I control new windows when nsBrowserAccess.openURI() isn't even
> called when the pref is set to 0 or 2?

What's nsIBrowserAccess?  And which pref?

> only when the prefs are set 'right'

There are some prefs are what the Firefox nsIWindowProvider implementation uses to determine what to do.  Are you talking about those prefs?  Or other ones?

It's nsContentTreeOwner, not nsIContentTreeOwner.  My apologies.

What do you mean by "previous patch"?
Comment 47 HJ 2006-03-06 15:15:46 PST
(In reply to comment #46)
> > Yes, from javascript.
> 
> Then why do any of these changes matter to start with?  If you're replacing the
> default window.open with your own... what difference does it make what the
> default window.open does?

People told me that I should replace nsBrowserAccess in navigator.js and drop our window.open method so I tried that, but it doesn't seem to work, but I would love to drop it for something better.

> > but how do I control new windows when nsBrowserAccess.openURI() isn't even
> > called when the pref is set to 0 or 2?
> 
> What's nsIBrowserAccess?  And which pref?

663     // hook up browser access support
664     window.browserDOMWindow = new nsBrowserAccess();

> > only when the prefs are set 'right'
> 
> There are some prefs are what the Firefox nsIWindowProvider implementation uses
> to determine what to do.  Are you talking about those prefs?  Or other ones?

Sorry, I should have been more clear. I was refering to 'browser.link.open_newwindow' and 'browser.link.open_external'.

> It's nsContentTreeOwner, not nsIContentTreeOwner.  My apologies.

I guess that I won't be possible with JavaScript, right?

> What do you mean by "previous patch"?

The patch that introduced 'browser.link.open_newwindow' and 'browser.link.open_external' but I have no clue what the corresponding bug number it was.

Comment 48 Boris Zbarsky [:bz] 2006-03-06 15:44:34 PST
> People told me that I should replace nsBrowserAccess in navigator.js and drop
> our window.open method

Whoever told you this is confused.

> 664     window.browserDOMWindow = new nsBrowserAccess();

Ah, ok.  That's an nsIBrowserDOMWindow, not an nsIBrowserAccess.

> I guess that I won't be possible with JavaScript, right?

No idea.  Would be tough, in any case.

> The patch that introduced 'browser.link.open_newwindow' and

That's like summer 2004, for what it's worth.  And given what's been said so far, it sounds to me like this was never the API you wanted for what you want to be doing.
Comment 49 HJ 2006-03-06 16:24:44 PST
(In reply to comment #48)
> > People told me that I should replace nsBrowserAccess in navigator.js and drop
> > our window.open method
> 
> Whoever told you this is confused.

That sucks, but my conclusion is that the current implementation is purely written to work for SeaMonkey/Mozilla Firefox because openURI() isn't called at all times, and I'm _not_ talking about blocking windows.

How else can one call this "a way to control _where_ windows open"? 

Would filing an RFE to make it call openURI() at all times make a change?

> > 664     window.browserDOMWindow = new nsBrowserAccess();
> 
> Ah, ok.  That's an nsIBrowserDOMWindow, not an nsIBrowserAccess.
> 
> > I guess that I won't be possible with JavaScript, right?
> 
> No idea.  Would be tough, in any case.
> 
> > The patch that introduced 'browser.link.open_newwindow' and
> 
> That's like summer 2004, for what it's worth.  And given what's been said so
> far, it sounds to me like this was never the API you wanted for what you want
> to be doing.

I found the bug and it was checked in on 2005-02-02 02:04
See also: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/xpfe/browser/resources/content&command=DIFF_FRAMESET&file=navigator.js&rev2=1.562&rev1=1.561
Comment 50 Boris Zbarsky [:bz] 2006-03-06 16:36:36 PST
> That sucks, but my conclusion is that the current implementation

You mean the nsIWindowProvider implementation?  That's correct.  I tried to change as little code flow as possible in the existing XUL apps.  If you want that window provider to let you hook into it, feel free to file bugs requesting that.

> Would filing an RFE to make it call openURI() at all times make a change?

That's not going to happen, but filing an RFE to allow you to add nsIWindowProviders that it'll call is a good idea.  I have no idea how it'd find out about them though, so some ideas on that would be welcome.

> I found the bug and it was checked in on 2005-02-02 02:04

On trunk.  And for the XPFE front end.  The core changes first happened on Aviary branch on 2004-09-30.

Comment 51 Mike Pinkerton (not reading bugmail) 2006-05-20 09:56:20 PDT
bz, thanks again for this patch, the implementation in camino was cake. :)
Comment 52 Eric Shepherd [:sheppy] 2009-02-17 18:53:34 PST
I intend to work on documenting this in the near future; if anyone has anything they want to call out to me as particularly interesting/important or not obvious, please do.  There's lots of stuff here, and I want to be sure I don't miss anything.

Note You need to log in before you can comment on or make changes to this bug.