44847 - mozilla needs to support the -remote command line flag

Status: RESOLVED FIXED

(Core Graveyard :: X-remote, defect, P3)

Description

[Christopher Blizzard (:blizzard)]

There are two phases to this.  We need a component that supports the actual
command line flag ( easy ) and some xlib that talks to the X server and tries to
find a running browser instance and talks to it.  We can't use the existing gtk
code for this since it means that we have to start up the component, initialize
everything and if an instance is not running shutdown and restart.  We can't
really do that with the gtk component right now.

Comment 1

[Christopher Blizzard (:blizzard)]

duh

Comment 2

[Christopher Blizzard (:blizzard)]

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

Comment 3

[Christopher Blizzard (:blizzard)]

mass assign

Comment 4

[Christopher Blizzard (:blizzard)]

Attachment: xremote impl

Comment 5

[Christopher Blizzard (:blizzard)]

This has been sitting on my drive for a while. Uploading so that I don't delete
it.  akk has reviewed this code.

Comment 6

[Christopher Blizzard (:blizzard)]

Attachment: round two

Comment 7

[Christopher Blizzard (:blizzard)]

Attachment: patch for apprunner

Comment 8

[Christopher Blizzard (:blizzard)]

Ok, looking for review on this code.  Anyone up for it?

Comment 9

[Christopher Blizzard (:blizzard)]

Attachment: pass 3

Comment 10

[Christopher Blizzard (:blizzard)]

Attachment: new bootstrap patch

Comment 11

[Christopher Blizzard (:blizzard)]

added feedback from dmose + shaver

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

OK, as mentioned in IRC, I'm not really qualified to review the X stuff, as my X
foo is not strong.  Here's some miscellenous comments on the rest of the code: a
few of the comments are actually important, the rest are nits which may or may
not be worth taking the time to clean up.

HandleRemoteArguments():

- shouldn't the (argc == i-1) guard actually be (argc-1 == i)?

- is it actually necessary to shutdown() and re-init() after each
  sendCommand()?  couldn't you actually init() once
  at the first command and then shutdown when you're done?

- wisdom received from scc yesterday: using a constructor form will
be better than a copy initializer on some compilers (ie those with
crappy optimizers)

so:

nsCOMPtr<nsIXRemoteClient> client =
			   do_CreateInstance(NS_IXREMOTECLIENT_CONTRACTID);

becomes:

nsCOMPtr<nsIXRemoteClient> client(
			   do_CreateInstance(NS_XREMOTECLIENT_CONTRACTID));

XRemoteClient:

- I think NS_INIT_ISUPPORTS() is now preferred over NS_INIT_REFCOUNT(),
and NS_IMPL_ISUPPORTS* is preferred to the NSS_IMPL_{ADDREF|RELEASE}
and NS_INTERFACE_MAP* stuff.

XRemoteClient::GetLock()

- why use malloc/sprintf/uname/strncat/fprintf rather than
nsMemory::Alloc()/PR_sprintf/PR_GetSystemInfo/PR_strncat/PR_fprintf?  In
particular, I suspect some non-Linux systems will run into problems
with uname() portability.

- need to check the return value from malloc in order to prevent a
segfault in the subsequent sprintf.  alternately, you could just use
an nsAutoString for this.

XRemoteClient::DoSendCommand():

- why are True and False used as values for |done| instead of PR_TRUE
and PR_FALSE in some places?

misc nits:

- the code would be more readable if you set up a PR_LOG module for
x remote debugging and used PR_LOG (it automagically compiles away in
non-debug builds)... this would allow you get rid of all the #ifdef
XREMOTE_DEBUG cruft

- shouldn't the contractid symbol be NS_XREMOTECLIENT_CONTRACTID (no I) as
per the IRC chat with shaver?

- why are the files and class named XRemoteClient* rather than nsXRemoteClient*?

Comment 13

[Christopher Blizzard (:blizzard)]

Attachment: patch 4

Comment 14

[Christopher Blizzard (:blizzard)]

Attachment: bootstrap 3

Comment 15

[Christopher Blizzard (:blizzard)]

> OK, as mentioned in IRC, I'm not really qualified to review the X stuff, as my X
> foo is not strong.  Here's some miscellenous comments on the rest of the code: a
> few of the comments are actually important, the rest are nits which may or may
> not be worth taking the time to clean up.
>
> HandleRemoteArguments():
>
> - shouldn't the (argc == i-1) guard actually be (argc-1 == i)?

Yep.  I'm lame.

>
> - is it actually necessary to shutdown() and re-init() after each
> sendCommand()?  couldn't you actually init() once
> at the first command and then shutdown when you're done?

No, we only send one command.

>
> - wisdom received from scc yesterday: using a constructor form will
> be better than a copy initializer on some compilers (ie those with
> crappy optimizers)
>
> so:
>
> nsCOMPtr<nsIXRemoteClient> client =
> do_CreateInstance(NS_IXREMOTECLIENT_CONTRACTID);
>
> becomes:
>
> nsCOMPtr<nsIXRemoteClient> client(
> do_CreateInstance(NS_XREMOTECLIENT_CONTRACTID));

OK.

>
> XRemoteClient:
>
> - I think NS_INIT_ISUPPORTS() is now preferred over NS_INIT_REFCOUNT(),
> and NS_IMPL_ISUPPORTS* is preferred to the NSS_IMPL_{ADDREF|RELEASE}
> and NS_INTERFACE_MAP* stuff.

OK, done.

>
> XRemoteClient::GetLock()
>
> - why use malloc/sprintf/uname/strncat/fprintf rather than
> nsMemory::Alloc()/PR_sprintf/PR_GetSystemInfo/PR_strncat/PR_fprintf?  In
> particular, I suspect some non-Linux systems will run into problems
> with uname() portability.

Didn't know about PR_GetSystemInfo().  It's used now.  I'm using PR forms of
functions now, too.

>
> - need to check the return value from malloc in order to prevent a
> segfault in the subsequent sprintf.  alternately, you could just use
> an nsAutoString for this.

Using an nsCString as a member now.  Thanks.

>
> XRemoteClient::DoSendCommand():
>
> - why are True and False used as values for |done| instead of PR_TRUE
> and PR_FALSE in some places?

Because it's somewhat cut-n-pasted.  See the copyright for where from.  Fixed.

>
> misc nits:
>
> - the code would be more readable if you set up a PR_LOG module for
> x remote debugging and used PR_LOG (it automagically compiles away in
> non-debug builds)... this would allow you get rid of all the #ifdef
> XREMOTE_DEBUG cruft

Ugh.  Fixed.

>
> - shouldn't the contractid symbol be NS_XREMOTECLIENT_CONTRACTID (no I) as
> per the IRC chat with shaver?

Yep.  I'm lame.

>
> - why are the files and class named XRemoteClient* rather than nsXRemoteClient*?

Because XRemoteClient is the name of the class and nsI is what I use for
interfaces.  Like IFoo for MS interface names.

I also added a timeout to the code so it will give up after 10 seconds if it
doesn't get a lock.  This is something that's annoyed me forever in Netscape.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

> > - why are the files and class named XRemoteClient* rather than
> >   nsXRemoteClient*?
>
> Because XRemoteClient is the name of the class and nsI is what I use for
> interfaces.  Like IFoo for MS interface names.

My impression was that the convention was for class names to also begin with ns,
but perhaps that's now obsolete.

Also, there's still a superfluous I in NS_IXREMOTECLIENT_CID.

Modulo that, r=dmose.

Comment 17

[Christopher Blizzard (:blizzard)]

OK, the 'I' has been removed.  I'm not going to bother re-uploading it, though.

Comment 18

[Christopher Blizzard (:blizzard)]

Ugh.  I forgot that I need to return a non-zero exit status when there was an
error with -remote - either the browser isn't running or that it was unparsable.
 I forgot about that.

Comment 19

[Christopher Blizzard (:blizzard)]

Attachment: nsAppRunner.cpp patch

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

M-x replace-string
fprintf(stderr
PR_fprintf(PR_STDERR

in that last patch and you've got r=dmose

Comment 21

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I'd like to make a plug here for some system (such as the one I propose in
56654) that doesn't require hacking nsAppRunner.cpp.

Other than that, the patch looks good.  sr=shaver, if mine will do.

Comment 22

[Christopher Blizzard (:blizzard)]

I agree about the ugliness of the way that the command line stuff is implemented
but I don't want to wait for that to be checked in before I get this in.  This
will be easy to migrate for sure.

Comment 23

[Christopher Blizzard (:blizzard)]

Checked in.

Comment 24

[R.K.Aa.]

does this actually work?
Could someone give an example of a "mozilla -remote" command that would open a
local html-file on disk in an existing window, please.

Comment 25

[Christopher Blizzard (:blizzard)]

mozilla -remote 'openURL(file:///tmp)'

Comment 26

[R.K.Aa.]

Here is the reason i asked:

I have wv (wordview) and two scripts that works under NC4.75 to convert from
.doc and open
a html version of the file in Netscape.

In NC 4.75 i add this in prefs for helper apps:

Description: Winword-file
MIMEType: application/msword
Suffixes: doc
(Application:) /usr/doc/wv-0.5.44/helper-scripts/mswordview.wrapper %s

I add this similarly in mozilla.

The script is very simple stuff - here tuned for mozilla:

mswordview.wrapper

#!/bin/sh
PATH=/usr/bin
HTML=/tmp/mswordview.$$.html
wvHtml "$*" > $HTML
mozilla -remote 'openUrl(file://'$HTML')'
(sleep 30; rm -f $HTML)&

The script is inspired by one copyrighted 1998 by Jager Enterprises and Todd
Stiers, and distributed with the WV application/suite of scripts.

I modified the mozilla -remote line to read file:/// instead - same result:
It does not work in mozilla. In moz i get a dialog-box instead:

"This file has mime type application/octet-stream and cannot be
viewed using Mozilla. You can open it with another applications, or
save it to disk."

Studying what happens in /tmp it turns out that the script never gets as far as
to engage wvHtml binary - the file left behind there is unexpectedly a
rendomized filename with extention .doc - and the contens are identical to the
original .doc file - it's still a winword file.

So what seems to happen is that most of the script is ignored by moz, instead it
does things "on it's own" and the only thing it understands of this is which
file it was supposed to do something with.

Comment 27

[Christopher Blizzard (:blizzard)]

What you're describing here sounds like a problem that's related to external app
handlers, not the -remote command line.  I've added mscott for some possible
advice or a pointer to the right person.