Boris Zbarsky [:bzbarsky] Assignee
	Description • 19 years ago

Need to do a bit of surgery on tree owners, tabbrowser, etc.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 19 years ago

This part needs more testing, which I'll do once bug 323810 is fixed.

	neil@parkwaycc.co.uk
	Comment 3 • 19 years ago

Comment on attachment 210805 [details] [diff] [review]
Back-end changes

>+      nsCOMPtr<nsIDocShellTreeOwner> owner = do_GetInterface(parentItem);
>+      nsCOMPtr<nsIDocShellTreeOwner_MOZILLA_1_8_BRANCH> owner2 =
>+        do_QueryInterface(owner);
Would it be too much trouble to make do_GetInterface work for nsIDocShellTreeOwner_MOZILLA_1_8_BRANCH?

>+       nsRefPtr<nsXULWindow> win;
>+       xulWindow->QueryInterface(NS_GET_IID(nsXULWindow), getter_AddRefs(win));
Ugly IMHO, but I suppose it's valid in the same module.

	neil@parkwaycc.co.uk
	Comment 4 • 19 years ago

Comment on attachment 210803 [details] [diff] [review]
Front-end changes

So the use case is someone clicks on an <a target="foo"> which gets diverted into a tab, and the original page needs to be able to find it again?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 19 years ago

> Would it be too much trouble to make do_GetInterface work for
> nsIDocShellTreeOwner_MOZILLA_1_8_BRANCH?

Probably not, except I was trying to minimize the spread of nsIDocShellTreeOwner_MOZILLA_1_8_BRANCH

> Ugly IMHO, but I suppose it's valid in the same module.

Yeah.  I could also have changed (or rather extended) nsIXULWindow and gotten an enumerator from it, but this seemed simpler....

For 1.9 I'd really like to significantly change this tree owner stuff.  I just haven't figured out what arch actually makes sense.  ;)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 19 years ago

> So the use case is someone clicks on an <a target="foo"> which gets diverted
> into a tab, and the original page needs to be able to find it again?

That's one use case.  The other use case is bug 210986.

	neil@parkwaycc.co.uk
	Comment 7 • 19 years ago

So, why can't we make the root docShell FindChildWithName do the work?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 8 • 19 years ago

nsDocShell::FindItemWithName doesn't cross type boundaries, and that's an invariant I would like to leave, in general.  I suppose we could modify this by extending nsIDocShellTreeItem, but it'd take a lot closer code inspection on my part to ensure it's done safely.

More to the point, I think the idea of trees of docshells that "belong together" and should be able to target each other but not "unrelated" trees is the right conceptual abstraction here.  What we have to express that right now is tree owners...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 19 years ago

To test, load file #1 in a tab, then open a new tab in the foreground and load file #2 in the foreground tab.  When the JS executes, the load should happen in the background tab instead of opening a new window or notifying about popups or whatever.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 19 years ago

Comment on attachment 210805 [details] [diff] [review]
Back-end changes

OK, I tested this and it fixes bug 121377 and the testcase I just attached to this bug.  Doesn't seem to break anything that I can find.  So I think this is good to go.

	Benjamin Smedberg
	Comment 12 • 19 years ago

I'd suggest @status TEMPORARY or something, instead of DEPRECATED

It seems to me that, rather than adding a pseudo-IID for nsXULWindow, you could (and probably should)

1) make nsIDocShellTreeItem_MOZILLA_1_8_BRANCH inherit from nsISupports instead of nsIDocShellTreeItem
2) implement nsIDocShellTreeItem_MOZILLA_1_8_BRANCH on nsXULWindow

Except I see that doesn't work because nsContentTreeOwner::FindIt uses ->mTargetableShells directly, ugh.

I don't particularly like that as a long-term solution but I guess it's ok as a hacky interim (in the long term we should probably figure out what to do with nsIWindowMediator, too, since various embedders would like to mix "chrome" XUL windows and embedded windows, and that has lots of window-targeting issues).

Perhaps appshellservice (including xulwindow) should be merged with docshell?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 19 years ago

> Except I see that doesn't work because nsContentTreeOwner::FindIt uses
> ->mTargetableShells directly, ugh.

Right.  This is the only reason I need the new IID.  The content tree owner already has a nsXULWindow* pointer to its own nsXULWindow.  But in FindItemWithName we walk the enumerator, and get nsIXULWindow pointers out of it...

I could also have changes nsIXULWindow (or extended with a new interface for 1.8).  But that seemed to me like more work, and not really necessary...

> in the long term we should probably figure out what to do with
> nsIWindowMediator

Yes, indeed.

> since various embedders would like to mix "chrome" XUL windows and embedded
> windows

In what way?  The way I designed this stuff assumed that chrome XUL windows can only happen with XUL and embedded windows only happen with nsWebBrowser.  Are people mixing the two in a nontrivial way?

> Perhaps appshellservice (including xulwindow) should be merged with docshell?

It's all pretty XUL=app-specific.  I'm not sure there's value in merging it with the much more generic docshell....

	Benjamin Smedberg
	Comment 14 • 19 years ago

> In what way?  The way I designed this stuff assumed that chrome XUL windows can
> only happen with XUL and embedded windows only happen with nsWebBrowser.  Are
> people mixing the two in a nontrivial way?

There was some desire expressed in that regard, though AFAICT it doesn't actually involve loading content in the XUL windows. Think XUL xpinstall dialogs popped up from gtkmozembed windows to aid XULRunner app installation off the web.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 19 years ago

OK.  We should probably sort out the use cases and then figure out how to do this stuff, including what window mediator should do (and whether it should exist).

	Johnny Stenback (:jst)
	Comment 16 • 19 years ago

Comment on attachment 210805 [details] [diff] [review]
Back-end changes

sr=jst

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 20 • 19 years ago

Fixed on trunk.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 21 • 19 years ago

Fixed on the 1.8 branch.

	David E. Ross
	Comment 23 • 19 years ago

*** Bug 331056 has been marked as a duplicate of this bug. ***

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 24 • 19 years ago

*** Bug 292813 has been marked as a duplicate of this bug. ***

	Tracy Walker [:tracy]
	Comment 25 • 18 years ago

Verified fixed with Windows, Mac and Linux with 2.0.b1rc3 builds

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 26 • 18 years ago

For what it's worth... this needs to be separately verified on trunk, since the
changes were somewhat different.  Reopening so I can put it back in a "RESOLVED
FIXED" state...