346404 - nsIDOMWindowInternal::Open doesn't tell when the window was diverted into the current tab

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[Simon Bünzli]

See bug 344808 comment #15 where we currently use the length of the window's session history (alternatively we probably could also have checked that current window is the new one's opener). Shouldn't there be cleaner way of doing this?

Comment 1

[Boris Zbarsky [:bzbarsky]]

There's no reasonable way for Open() to report this that I can see.  More importantly, since the diverting is completely under the control of the embeddor the embeddor has to track this itself.  Core code has no idea that the load was diverted.

Comment 2

[Boris Zbarsky [:bzbarsky]]

In a bit more detail:

The nsIWindowProvider implementation does provide this information to the window watcher.  But the window watcher has no way to report this information back to anyone (frozen API, etc).

The two ways I see of dealing with that are either to somehow allow asking the window watcher to open a window _and_ do a reasonable load (with postdata, etc) in it or to introduce some semi-private APIs for docshell to handle window opening.

Though see my comments in bug 344808.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

option 3: special success return code

Comment 4

[Boris Zbarsky [:bzbarsky]]

That's still an API change to window watcher.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

the interface doesn't promise returning NS_OK always. although maybe that'd still be too risky for branch...

Comment 6

[Boris Zbarsky [:bzbarsky]]

You're assuming that no one but us implements window watcher.  If we change the window watcher IDL such that we have to make behavior changes to our window watcher impl to match, then we break anyone else's window watcher impl.

One could argue that others should not be implementing it, but there's no reason for someone to not override the contract to, say, examine all window open calls and then call into the default impl by CID...  If they're not _very_ careful, changing the meaning of retvals here could break them.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Note that once this is fixed we should fix the code that was added for bug 344808 accordingly.  And I feel that we should fix this one way or another for Gecko 1.9 -- either propagate this information, or remove the code that depends on it being propagated from docshell.  The latter would probably mean that we'd need to deprecate the nsIWebNavigation flag or something.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Bug 332182 adds some functionality that could be used here.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Maybe something like this?

Christian, what do you think? Too hacky?

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 233938 [details] [diff] [review]
Maybe something like this?

I actually kinda like this :)

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 233938 [details] [diff] [review]
Maybe something like this?

sr=jst

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed.