90722 - Need a way to target URI loads with nsIWebNavigation

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, enhancement)

Description

[Stuart Ballard]

I'm not sure whether this is the correct component, which is why I'm filing this
bug as UNCONFIRMED. If somebody who knows better than me could set the correct
component and confirm the bug, I'd appreciate it (my current choice is based on
what other nsIWebNavigation/loadURI bugs are filed as).

There is a bug in the implementation of the link toolbar in bug 87428 that it
doesn't accept the "target" attribute of the <link> tag. The implementation of
this is non-trivial because pages are currently loaded through the loadURI
method of nsIWebNavigation, which provides no API for loading a URL with a
"target" parameter.

Here is a relevant snippet from bug 87428:

------- Additional Comments From Tim 'Tool-Man' Taylor 2001-07-12 08:12 -------

OK, I think I found the necessary info to handle window targets in the
nsIDocShell API:

nsIDocShell::loadURI
<http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIDocShell.idl#50>

nsIDocShell::InternalLoad
<http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIDocShell.idl#95>

nsIDocShell::createLoadInfo
<http://lxr.mozilla.org/seamonkey/source/docshell/base/nsIDocShell.idl#125>

Of course, having a convienience function, loadURI(uri, target), in
nsIWebNavigation would still be a valid RFE.



------- Additional Comments From Boris Zbarsky 2001-07-12 12:25 -------

Tim, nsIDocShell::loadURI and nsIDocShell::InternalLoad are noscript. 
InternalLoad is so for good reason -- it really should not be used from a
script.

I'm talking to Rick Potts right now about a function on nsIWebNavigation that
will be pretty much like nsIDocShell::loadURI.  I need it to do loads from
cache, but it would likely work for this as well....  nsIDocShellLoadInfo
objects include a target, as you noticed.

------ End included comments -------

The proposed method needs to be scriptable because the link toolbar is
implemented through script (and there's no reason for it not to be - it's
equivalent to something that *untrusted html* can do for goodness sake).

Ideally (for a hypothetical future situation in which links provided within
frames are also honored) it should be possible to specify both the document that
is performing the link (or perhaps an element inside its DOM) and the target
string as provided in the link. This should be equivalent to what happens when
<a href="foo" target="bar"> is clicked within that document.

bzbarsky commented to me in private mail that this was related to bug 40867 -
I'm not sure I completely understand why unless 40867 is fixed by providing a
generalized API that also includes this parameter. Since it's certainly possible
to imagine solving 40867 *without* also solving this issue, I thought having a
separate bug would be a good idea.

bzbarsky, you mentioned that Rick Potts was involved in this issue. Should he
also be cc'd here?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Assigning to Rick, and ccing Judson Valeski, since they're talking about this
stuff right now.

Thanks for filing this, Stuart.

Comment 2

[rpotts (gone)]

As far as I can see, there are three possible solutions to this problem:
1. Add a target-name argument to loadURI(...).  The downside of this solution is
that it adds yet another way to perform window targeting.

2. Require 'targeted' loads to be done via the nsIWindowWatcher interface and
*not* nsIWebNavigation.  The dowside of this solution is that while
nsIWindowWatcher::OpenWindow(...) allows you to target the load to a particular
window, it *does not* allow load flags or post-data to be supplied.

3. Provide a FindNamedWindow("name", "current DOMWindow") method on
nsIWindowWatcher which returns a nsIDOMWindow...  The "current DOMWindow"
argument is necessary to achieve the correct search order (ie. children,
siblings, ancestors, other toplevel windows).  The downside of this solution is
that it mixes nsIWebNavigation and nsIDOMWindow.  We would need a way to easily
go from one to the other...

Personally, I like option #3 :-}  It doesn't create yet-another-way to target
content.  And it provides some useful generic functionality for locating named
windows...

If option #3 sounds like the right way to go, i'll close out this bug and open
up a new one against the WindowWatcher...

-- rick

Comment 3

[rpotts (gone)]

I'm resummerizing the bug to reflect what we're talking about :-)

Comment 4

[rpotts (gone)]

i've talked this over with danm and he agreed (he didn't hit me at least) that
funneling the window targeting through the window watcher made sense...

So, i'm going to pursue the following course:

I'll add 'nsIDOMWindow FindNamedWindow(in wstring name, in nsIDOMWindow caller)'
to nsIWindowWatcher.

The semantics of FindNamedWindow(...) will be to return *existing* windows only.
 If a window of the given name does NOT exist, then the caller MUST use
nsIWindowWatcher::OpenWindow(...) to create it.  Once the window has been
created, you can QI the new nsIDOMWindow to nsIWebNavigation to load the
specific URI (if extra load arguments are required - such as post data...)

Sound good?

Comment 5

[Judson Valeski]

sounds good.

Comment 6

[Conrad Carlen (not reading bugmail)]

Sounds good to me too. Are seamonkey's nsIXULWindows tracked by windowwatcher as
well? They should be but I'm not sure if they currently are.

Comment 7

[rpotts (gone)]

It appears (after lxr'ing through the code) that the window mediator only calls
SetActiveWindow(...) on the window watcher...  So, i assume that the window
watcher will not know about XUL windows in general (for iteration) :-(

is this correct? or have i missed something?

Comment 8

[Dan M]

  WindowWatcher doesn't know about XUL Windows. That's the distinction between it 
and WindowMediator; that's why we still have both. You can build WindowWatcher 
without the xpfe directory.

Comment 9

[Conrad Carlen (not reading bugmail)]

It's possible to get the nsIDOMWindow for the chrome of an nsIXULWindow but is
it possible (or valid) to go the other way? If there is a 1:1 realtionship, then
windowwatcher could maintain the XUL windows as nsIDOMWindows. Then, consumers
in xpfe could use windowwatcher and jump through a few Get/QueryInterface hoops
where they needed the nsIXULWindow.

Comment 10

[Dan M]

  Working on the assumption that this line of inquiry is salient to this bug: (I 
have this thing about rambling bug reports. But, enough rambling from me...)
  At the time, there was no mapping from a nsIDOMWindow to a XULWindow. I think 
however that something could be cobbled together beginning with the recently 
added nsIWindowWatcher::GetChromeForWindow, without necessitating including the 
definition for nsIXULWindow in embedding source. I should look into that.
  (And in that case, only the subset of an application's valid DOM Windows 
corresponding to the XUL Window's main content would return non-null.)

Comment 11

[Stuart Ballard]

Call me dense (or at least horribly underinformed about mozilla's APIs, xpcom,
etc) but can someone explain to me in simple terms how this would be used in an
actual situation.

In the current link toolbar patch in bug 87428, the following single line in
linkToolbarOverlay.js is sufficient to load the page specified by the link.

loadURI(event.target.getAttribute("href"));

The desired behavior is to use the frame/window specified by
event.target.getAttribute("target") to perform this load. So, given the API
proposed in this bug so far, what would be the replacement for this single line
of code?

Comment 12

[rpotts (gone)]

ok...  i give in :-)

i've thought about this issue alot and i finally agree that we should allow
window  targeting via nsIWebNavigation::LoadURI(...).  i've been resistant to
the idea because from an API perspective window targeting should *really* be
done at a higher level...  but from a usability perspective (especially from
javascript) it is *very* appealling to allow this :-)

so, i finally agree that window targeting should be available from LoadURI(...)

i've just attached a patch to bug #90722 which adds a targetWindow argument to
nsIWebNavigation::LoadURI(...)

-- rick
 

Comment 13

[rpotts (gone)]

oops that should have been bug #94205 :-)

Comment 14

[jag (Peter Annema)]

Hrm.

I personally think that polluting the nsIWebNavigation interface is not The
Right Thing (tm) to do.

What's wrong with having something like:

var s = Components.classes[NS_SOMETHING_CONTRACTID]
                  .getService(nsISomething);
var w = s.openWindow(targetName); // returns newly created or existing window
w.loadURI(url, loadFlags, postData, headers);

? It's only two lines more than doing loadURI on nsIWebNavigation. I view
nsIWebNavigation to act on the current window, and not be yet another interface
to do multi-window/cross-window manipulation.

Comment 15

[Stuart Ballard]

FWIW, I wouldn't have a problem with that (and it might indeed be cleaner than
putting the API on webnavigation) but I'd suggest calling the method getWindow
rather than openWindow - after all, *opening* a window is easy using existing
APIs; the trick here is to *get* a window (or frame!) that already exists, or
determine that there isn't one (in which case we fall back to opening a new
one).

Perhaps we could compromise and put *this* method into nsIWebNavigation - thus:

webNavigation.getWindow(target).loadURI(href);

getWindow should return self if target is null, for convenience, or otherwise an
nsIWebNavigation on the target window. This wouldn't exactly be polluting
webNavigation because it's finding a window by name *relative to* the current
document displayed in the window - this is important because if two documents on
different sites use the same name, (I assume and hope) they don't get the same
window.

Comment 16

[jag (Peter Annema)]

Hmmm, I think your assumption is wrong. I suspect that if I have sites A and B,
and both "open" a window with the name MainWindow (e.g. <a href="..."
target="MainWindow">), they'll both load into the same window. Testing this
confirmed my suspiscion.

I don't think |getWindow| or |openWindow| should be on the nsIWebNavigation
interface, nor do I see a need to compromise here. Having to write two more
lines doesn't IMO warrant a convenience method.

Fwiw, we could put such a convenience method on <browser>, or make it a function
in navigator.js.

Comment 17

[rpotts (gone)]

well, i guess it's about time theat i confess my *other* motive for adding a
targetWindow argument to LoadURI(...) :-)

By providing a targetWindow to LoadURI(...) i was able to avoid adding a
'referer' argument...  The reason was that because the targeting was explicit, i
was able to correctly set the 'referer' under the covers...

If the API required an explicit targeting step (such as getWindow()) then the
'referer' must be explicitly passed in - because there is now way of knowing
whether the load was targeted (and should not use the current URI as the
referer) or not...

I know, I know, this is nasty :-)  but i thought (perhaps incorrectly) that it
was simpler at this level to allow window targeting than to force the caller to
explicitly deal with the 'referer'...

The alternative is to add a 'referer' argument to LoadURI(...) and expose a
'read-only referer' attribute...

What do you think? was I wrong - and just looking for the easy way out :-)

-- rick


  

Comment 18

[Stuart Ballard]

How about just a boolean parameter that says "pass the current document's URI as
the referer"?

I can think of other cases beyond targetted loads where you *wouldn't* want to
do this - specifically, when the user typed in the new URI by hand or used a
bookmark (both of which should result in a null referer).

If you want to make it possible to pass *another* document's URI as the referer
(eg the document that triggered the target-ed load) then I suppose you'll need
to have an explicit referer parameter...

Comment 19

[jag (Peter Annema)]

I think passing in referer explicitely (or not) is cleaner.

Where would one pass in this argument though? It seems clicking a link or an
image map are the only times you'd want to grab the referer and pass that in. If
those are indeed the only places, isn't there an internal call we could use?

Comment 20

[Stuart Ballard]

Nope, the link toolbar (written in javascript and using webNavigation) would
also want to set referer. Clicking one of these toolbar items should be exactly
equivalent to clicking a standard <a href= target=> link. So if we do have
getWindow() (somewhere) then we also need to have an explicit referer parameter.

Comment 21

[jag (Peter Annema)]

Okay, I'm all for passing in referrer then.

Comment 22

[Judson Valeski]

passing in the referer can be painful. this thread alone indicates the
background state needed to do it, and doing it "right" is another story. IMO, we
don't want to push this onto embeddors if we can bury it ourselves. it leaves a
lot of room for mis-use.

Comment 23

[jag (Peter Annema)]

Misuse is in the hands of anyone who can get the source :-) Incorrect use is a
different story.

Is there a clean way to do it that doesn't "burden" embedders? I think it'll be
hard though to correctly guess (let alone know) when we'll want to set a referer
and when not, unless explicitely told. But I'd love to be proved wrong :-)

Comment 24

[Jussi-Pekka Mantere]

Triaging nsenterprise-.

Comment 25

[rpotts (gone)]

Attachment: patch to add getWindowByName to nsIWindowWatcher...

Comment 26

[rpotts (gone)]

Attachment: Patch to make window targetting work correctly with the previous patch :-)

Comment 27

[rpotts (gone)]

the patch to window targeting moves the handling of predefined window targets
(ie. _self, _parent, _top) from nsDocShell::FindTarget(...) [which is only
called by InternalLoad] over to nsDocShell:FindNamedItem(...) which is called by
everyone :-)

I left the handling of "_blank" and "_new" alone because FindNamedItem only
returns *existing* windows...

Comment 28

[Dan M]

r=danm. one quibble (he says eagerly, so rarely given a chance to be pedantic)
about the first patch (52858); I think on the Mac especially that copy/assign of
a COMPtr was found to make surprisingly larger code than a direct constructor.
If everybody stopped doing it, the whole Mac app would fit on a floppy!

Comment 29

[rpotts (gone)]

really?  i always thought that was an urban legend...  if it is really true that:

  nsCOMPtr<nsIFoo> foo;
  foo = do_QueryInterface(...)

is more expensive than:
  nsCOMPtr<nsIFoo> foo(do_QueryInterface(...));

than i'll change it...
i always use the two line version because it is easier to read - and doesn't
exceed 80 col. as easily :-)

-- rick

Comment 30

[Scott MacGregor]

Comment on attachment 52923 [details] [diff] [review]
Patch to make window targetting work correctly with the previous patch :-)

sr=mscott

Comment 31

[Scott MacGregor]

Comment on attachment 52858 [details] [diff] [review]
patch to add getWindowByName to nsIWindowWatcher...

sr=mscott

Comment 32

[jag (Peter Annema)]

From reading the code it looks like these two execute the same operations,
though the compiler could favor one over the other (anyone wanna profile this?):

1)
  nsCOMPtr<nsIFoo> foo(do_QueryInterface(...));

2)
  nsCOMPtr<nsIFoo> foo;
  foo = do_QueryInterface(...);

This next one could be more expensive (since it will theoretically have to
create a temporary nsCOMPtr since it has to go through the copy constructor),
but most compilers optimize this specific case, and it seems there are even
compilers on which it is slightly cheaper, see also
<http://www.scottcollins.net/programming/cpp/initialization.html>:

3)
  nsCOMPtr<nsIFoo> foo =
           do_QueryInterface(...);

I'd say don't worry about it for now, and use whichever form makes you happy.

Comment 33

[Conrad Carlen (not reading bugmail)]

I used the disassembler with CW and found that init-by-assignment and
init-by-construction generated *identical* code.

Comment 34

[Dan M]

Well, neato. I must have misremembered that discussion. Ignore my pedantism, then.

Comment 35

[rpotts (gone)]

closing out because the patch has been checked in ...

-- rick

Comment 36

[rpotts (gone)]

really closing :-)