73104 - URILoader makes call to window.open

Status: VERIFIED WONTFIX

(SeaMonkey :: General, enhancement)

Description

[Peter Trudelle]

Need to change how window is invoked so it can be overridden by embedding 
clients.  Some details below, more at URL above; danm will be happy to help and 
answer questions.

UI Posed by Gecko (Up Calls)
============================
These are calls to window.open, in one form or another, which are relevant to 
embedding. They're split into calls and callers, where a call is considered to 
be a base-level call to window.open which should be API-ized like we're 
discussing, and a caller is something which currently uses window.open but 
should use one of these new APIs when they're created.

I've tried to categorize these sensibly... Each category implies a UI-posing 
component with methods for each item marked "CALL" in that category. Items 
marked "CALLER" in a given category may imply a new method in that component 
(eg. the print dialog stuff), or may just be a caller of some other extant 
method in a different component (eg. the uri loader opening a new window).

URI Loader:
CALLER: /uriloader/base/nsURILoader.cpp#697:
  in ::GetTarget, create a window based on <a target="..."> for new content

Comment 1

[Peter Trudelle]

Adding mozilla0.9 keyword, dependency.  assigning to locka, who apparently 
wrote this code.

Comment 2

[Blake Ross]

I think he prefers his Netscape address (?)

Comment 3

[Dan Rosen]

Apologies for all the bug spam. For the detailed overview of this task, see
danm's document:

  http://www.mozilla.org/xpfe/embedding-dialogs.html

See my first cut at a component-wise categorization:

  http://bugzilla.mozilla.org/showattachment.cgi?attach_id=27972

And if you want to laugh, see also my brainless incomplete IDL for these components:

  http://bugzilla.mozilla.org/showattachment.cgi?attach_id=28175

dr

Comment 4

[Adam Lock]

CC'ing Conrad who is currently doing some nsIPrompt work.

Comment 5

[Judson Valeski]

-> adam's internal addr.

Comment 6

[Conrad Carlen (not reading bugmail)]

The way this window gets put up does not have anything to do with nsIPrompt.
Currently, the call to window.open being made here ends up in my impl of
nsIWebBrowserChrome::CreateBrowserWindow, so as far as embedding goes, this is
what we want. The path it takes to end up in CreateBrowserWindow is long
(through a content handler in the mozBrowser DLL) which we don't want for
embedding. 

Comment 7

[Judson Valeski]

dan/dr,
this case appears to be one in which the CALLER (the URI loader creating a new
top-level browser window for a new target) is/should be using nsIWindowCreator,
is this correct?

Comment 8

[Adam Lock]

I think the strategy for the up callers should be to write some UI components 
in embedding/components to represent each dialog. Each UI components is an 
XPCOM object implementing an interface as hashed out by Dan, and registered 
with a component category. Embedders can substiture their own dialog in place 
of the default one by with their own component with the same category.

I'm working through separating the download progress UI out of the streamxfer 
object to see if the idea is feasible.

Comment 9

[Dan M]

  We created these bugs based on more a string search than a careful analysis of 
the use of each line of code that caught our attention. (Though I hasten to say 
there was some analysis involved.) On closer inspection, this particular 
window.open looks less like a potentially problematic dialog; more like the 
legitimate creation of a new browser window. As such, it can probably stand as 
is.
  However, Adam's strategy analysis above is right on, so he obviously gets what 
we're trying to do and I'll leave him to it. (One point: I wasn't imagining 
moving the implementations for all these UI-controlling components into the 
embedding directory. I look at them as still belonging more to the code whose UI 
they define than to the embedded world. I was thinking they'd be happier staying 
with the component whose UI they were controlling. Of course in the binary 
distribution, they all get lumped into the same directory as everyone else.)

Comment 10

[Adam Lock]

My suggestion for putting the UI components into embedding is that it makes it 
more obvious to embedders which ones they must to override. If we spread them 
over the length and breadth of the source it becomes much more difficult to find 
them.

Having said that, it does complicate matters for us because we must modify two 
modules instead of one whenever something needs to be changed in one of the 
dialogs.

Comment 11

[Adam Lock]

CC'ing [e-mail]

Bill, I understand you were doing some work to the unknown content handler 
dialogs. Can you refer me to the bug nr so we don't work at cross-purposes?

Thanks

Comment 12

[Adam Lock]

Attachment: Patch splits UI xfer into new nsDownloadProgressUI object and cleans up the interfaces

Comment 13

[Adam Lock]

This patch splits out the UI part of the stream transfer object into a new 
object. It demonstrates how we'd likely have to do all the dialogs. 
Communication is done between the stream transfer object and the UI via an 
nsIDownloadProgressUI and nsIDownloadProgressCallback interface. The stream 
transfer object creates the UI via its contract id.

Note that the patch needs some cleanup and won't compile yet since I 
have just moved the UI part from embedding/components into xpfe/components/xfer 
so the makefiles needs to be fixed up yet.

Comment 14

[Bill Law]

Adam, my recommendation would be to not invest further effort into this 
particular patch.

I'm already doing all that to generalize the download progress dialog code so 
that a single implementation can be shared in place of the two implementations 
we have currently.  That will also move the code into the embedding directory, 
a requirement identified during discussion of the nsIHelperAppLauncherDialog 
API a while back.

That prompted bug 70228.  That bug doesn't say too much (and some of what it 
does say is erroneous, I believe).  A better place to find out about work on 
the helper app dialog is bug 52454.

The code I have written for that bug, in embedding/components/ui/helperAppDlg, 
is a reaonable example of packaging up dialogs/UI behind an interface, I 
think.  I'm doing the same thing with the download progress dialog.

Comment 15

[Adam Lock]

Do you want to take this bug Bill?

Comment 16

[Bill Law]

No, at least not until somebody can tell me what the hell it's all about.  It 
started off talking about the uriloader opening new windows for targetted 
links, I think.  How did the subject turn to download progress dialog?

Comment 17

[Adam Lock]

Changing the summary to reflect what this bug is about :)

Basically all the dialogs that can be thrown up by Gecko need to be overridable 
by embedders. The most obvious way to achieve this is to make them all XPCOM 
objects so that embedders can register their own objects instead.

Comment 18

[Peter Trudelle]

This bug was only intended to cover one such dialog, the larger issue is 
covered by tracking bug 65233.

Comment 19

[Adam Lock]

Bill, does this bug sound like it meshes with your work now it's been renamed?

Comment 20

[Adam Lock]

Bill, scratch my last question , I just saw a comment from Peter that I need to
clarify first.

Peter, apologies I didn't realise this bug was specific to a particular up
calling instance. The next question is where is this up calling instance? The
code I wrote in GetTarget opens a new window but there is no hardcoded chrome
URL or anything in there so I'm not sure what the issue is. Do you want me to
use some other method to open the window?

Comment 21

[Peter Trudelle]

cc danm

Comment 22

[Peter Trudelle]

Changing summary from general case back to the specifics of this bug.

Comment 23

[Dan M]

  Adam: no need to morph this bug into taking care of the whole problem. There 
are other bugs for that (see tracking bug 65233); in this one we were worried 
only about the particular window.open in nsURILoader.cpp. It must be the one 
currently at line 773.
  Again, as I've found myself saying kind of often, we may have been too 
aggressive identifying this one as a problem area. I'm looking at it now. If, as 
it appears, it's a legitimate case of opening a new (browser?) window in which to 
load a URI, there's probably nothing that need be done. I don't quite get what's 
going on here. Opening a new window with no URL?
  Anyway, as you ably pointed out in a comment above, April 9th, what we're 
trying to accomplish here is to define, implement and use an API to open windows, 
where appropriate, rather than opening a window directly. "Where appropriate" 
means, of course, if the window is really a dialog that an embedding app could 
conceivably want to replace.
  If that's not the case with this mysterious URL-less window, close the bug WFM.

Comment 24

[Adam Lock]

See bug 72491 and bug 41241. The reason for the window.open in uriloader is to 
provide a valid window context in which to push http data. 

This code prevents two consecutive http channels from being opened. The first 
channel would be aborted because there was no window context. The second would 
be opened after the new window was eventually opened. This is bad enough in 
normal circumstances, but for forms it meant they were submitted twice and the 
second channel was always a GET operation even if the first contained postdata.

Comment 25

[Peter Trudelle]

So, sounds like this was a red herring, as long as the window is hidden and
doesn't affect any of the embedder's UI.  Can we close it out?

Comment 26

[Adam Lock]

It shouldn't affect the embedders because all the JS calls eventually route 
through to the embedders CreateBrowserWindow method to do the opening. 

I'll mark it WONTFIX. Please reopen if any issues arise from the code.

Comment 27

[David Epstein]

reassign qa contact to depstein.

Comment 28

[David Epstein]

verifying