98603 - rewrite X Remote support

Status: RESOLVED FIXED

(Core Graveyard :: X-remote, defect)

Description

[Christopher Blizzard (:blizzard)]

X Remote is a mess, it's architecturally unsound and it's all in widget/.  It
needs to be rewritten.

Comment 1

[Christopher Blizzard (:blizzard)]

Most of the new code lives here:

http://lxr.mozilla.org/mozilla/source/xpfe/components/xremote/

I'll attach a patch for the rest of the tree in a moment.

Comment 2

[Christopher Blizzard (:blizzard)]

Attachment: big patch

Comment 3

[Christopher Blizzard (:blizzard)]

Here's a rough description of how this code works:

The xremote service that used to live entirely out in widget/ has been broken
into two major parts.

o The bulk of the string protocol parsing lives in xpfe/components/xremote. 
This knows how to parse the request string, target the request properly and
complete the request.

o There is code that lives in widget/ that knows how to handle the X bits of the
protocol and pass it to the xpfe component.

Also, we don't blindly listen for x remote protocol requests blindly on every
toplevel window anymore.  There are hooks in navigator.js that set up that
window as a target for x remote requests.  When there are no navigator browser
windows open a proxy window is created that will respond to requests.  Through
the proxy window it's possible to open mail/news, compose messages, open browser
windows, load uris, and other assorted bits.

The code also adds the ability to:

o open mail/news
o explicitly open a browser window
o simple 'ping' to see if there's an alive browser instance
o extensible xfeDoCommand() from the 4.x days

Comment 4

[Alec Flett]

I have a few issues:
 - you're wedging this into nsAppRunner.cpp - you should be able to use the
app-startup mechanism, where you register yourself in the app-startup category,
and you get loaded at startup... 
 - why do you need to manually Shutdown()? Can't you just get shut down like any
service?
 - what happens if the first window opened isn't navigator - i.e. it's mail or
news? does the proxy window open? I'm not sure I understand why you switched
from making every top-level window handle this stuff to only browser windows..
seems like it adds even more code to the already bloated navigator.js

Comment 5

[Christopher Blizzard (:blizzard)]

>  - you're wedging this into nsAppRunner.cpp - you should be able to use the
> app-startup mechanism, where you register yourself in the app-startup category,
> and you get loaded at startup... 

Before I call Startup() on the service I need to make sure that the appshell is
initialized so I can create a native window, preferences exist so I can read
them and that the first window has been created.  If I don't wait for the first
window is created then I will create a proxy window which will promptly be
destroyed during startup.  I don't want that startup time hit.

From my reading of the code it looks like the app-startup mechanism is run
before everything that I need initialized is actually initialized.  Please
correct me if I'm wrong.  I would love to use something other than the wedge
that's there now.  The ifdefs to leave me feeling a little dirty, and not in the
good way.

>  - why do you need to manually Shutdown()? Can't you just get shut down like any
> service?

I'm not sure.  I would prefer that I could Shutdown() right before the last
window is destroyed so I don't create the proxy window during shutdown.  Is
there a way to get that notification?  Can you point me at code that observes
shutdown right now?

>  - what happens if the first window opened isn't navigator - i.e. it's mail or
> news? does the proxy window open? I'm not sure I understand why you switched
> from making every top-level window handle this stuff to only browser windows..
> seems like it adds even more code to the already bloated navigator.js
> 

The proxy window is created when there's no navigator window.  That's why I
start up the service after the first window is opened.  There are three reasons
why I moved listening for remote requests into navigator.js.  The first is that
most of the x remote commands are browser-centric.  Open requests can be
targetted at specific windows and this architure is the first step in making
specific targetted openurl requests work.

The second, and most important reason, is that the architecture pretty much
requires it.  As the code is right now we listen for X remote requests on
practically every window.  This includes dialogs, windows like the popup for
autocomplete - everything.  These windows have short lifetimes and you shouldn't
listen for requests on them.  So, the question is, how do you figure out what
windows you should listen on and which ones shouldn't you?

Well, I didn't really want to have to modify all of the major toplevel window
implementations to listen for requests.  ( Think not just about mail/news but
all of the silly startup profile dialogs, etc. )  Since most people have a
browser window open most of the time and targetting requests is important it
made sense to listen for requests on browser windows.  And I also wanted to
support turbo in the future which means that there has to be a situation where
you have a single window open to listen for X remote requests but no chrome
windows.  The proxy window was needed - I just started using it in place of all
of the other windows.

The third reason is because of embedding.  I shouldn't be listening to all
toplevel windows because the chrome-driven dialogs that might be created when
mozilla is embedded might end up decorated with x remote properties and that
generates some pretty scary results.  You should see what galeon does when it
gets a new window request.  It actually loads navigator.xul into one of it's
browser windows.  This is something that decorating only a toplevel browser
window solves at least partially.

In the future I might want to use the proxy window in order to listen for
requests for embeddors and make a service so they can process the requests
themselves.  But that's for another day.

Comment 6

[Alec Flett]

Ok, I read a little more (i.e. XRemoteService.cpp) and I guess the one thing I
want to make sure is, can an xremote client target a particular window? is that
the purpose of registering each individual browser window? if so, then this all
makes sense to me - I had previously been under the impression that all xremote
could do was say "hey, open this url wherever you want!"

With that confirmation, sr=alecf on all of this, and one comment on the existing
code: in XRemoteService.cpp, when you don't have a window target, you're using
the WindowMediator service to find the last window, whereas I feel like you
should be using the URI loader to dispatch the url to be loaded by the
appropriate window.

That issue should be investigated at a later point, because the URI loader might
handle special logic such as: if a window is busy (i.e. loading content) then we
probably don't want to dispatch a url to that window if we can avoid it. (I
don't believe the URI loader does that now, but I also believe there is a bug on
it somewhere)

Comment 7

[Christopher Blizzard (:blizzard)]

> Ok, I read a little more (i.e. XRemoteService.cpp) and I guess the one thing I
> want to make sure is, can an xremote client target a particular window? is that
> the purpose of registering each individual browser window? if so, then this all
> makes sense to me - I had previously been under the impression that all xremote
> could do was say "hey, open this url wherever you want!"

Yes, you can target based on X Window ID or based on window name.  Those aren't
complete yet.  I'll open bugs to clean that up but this is the infastructure
that makes that targetting possible.

> 
> With that confirmation, sr=alecf on all of this, and one comment on the existing
> code: in XRemoteService.cpp, when you don't have a window target, you're using
> the WindowMediator service to find the last window, whereas I feel like you
> should be using the URI loader to dispatch the url to be loaded by the
> appropriate window.

Well, the most recently use window is how Netscape 4.x did it and the way that
the current X Remote code is done as well.

> 
> That issue should be investigated at a later point, because the URI loader might
> handle special logic such as: if a window is busy (i.e. loading content) then we
> probably don't want to dispatch a url to that window if we can avoid it. (I
> don't believe the URI loader does that now, but I also believe there is a bug on
> it somewhere)
> 

I think that in the end when I move to targetted named windows I'll end up using
the URI loader service for that.  However, for random requests ( load a uri
somewhere ) I'll probably use the most recently used window.  Or, if I ever hook
up the ZOrder interfaces in the window mediator service for unix I'll use that
instead.

Comment 8

[Alec Flett]

what I mean is that eventually the uri loader should be handling all the logic
for "load this url somewhere" - i.e. instead of duplicating last-window/zorder
logic, let's centralize it in the uri loader's uri dispatching mechanism.

Comment 9

[Christopher Aillon (sabbatical, not receiving bugmail)]

Sometime after this rewrite patch went into the trunk, xremote does not function
at all.  I've seen this on 2001090821 and it still does not work in current cvs.

Needless to say, clicking links in any of my other apps (x-chat, gaim, licq,
etc) no longer work.

Do we want to open a new bug for this?

Comment 10

[Christopher Blizzard (:blizzard)]

If you're having problems then please open a new bug.

Comment 11

[Asa Dotzler [:asa]]

->blizzard

Comment 12

[Christopher Blizzard (:blizzard)]

This was done a while ago.