237283 - make x-remote aware of profiles, specific programs and usersnames

Status: RESOLVED FIXED

(Core Graveyard :: X-remote, defect)

Description

[Christopher Blizzard (:blizzard)]

This is the bug that will make it possible to use xremote with thunderbird,
firebird and "stock" mozilla.  Also, it should be possible to specify profiles,
programs and usernames on the command line for mozilla-xremote-client.  Code to
follow.

Comment 1

[Christopher Blizzard (:blizzard)]

Attachment: first pass at a patch

This patch implements much of what's required, minus the actual profile name
being stuffed onto the window.

Comment 2

[Christopher Blizzard (:blizzard)]

Attachment: firebird patch required

This patch patches the forked code that firebird uses, too.

Comment 3

[Christopher Blizzard (:blizzard)]

Attachment: combined patch with profiles

This patch should be enough to use.  Looking for reviews.  Includes profile
support.

Comment 4

[Christopher Blizzard (:blizzard)]

Still need to add support for profile switching and add more command line
handling to the mozilla client.

Comment 5

[Christopher Blizzard (:blizzard)]

Attachment: patch that includes better profile support and command line handling

This patch has better support for profile switching and adds real command line
handling to the mozilla binary(ies) proper.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 143973 [details] [diff] [review]
patch that includes better profile support and command line handling

>--- xpfe/bootstrap/nsAppRunner.cpp	8 Mar 2004 23:48:39 -0000	1.410
>+++ xpfe/bootstrap/nsAppRunner.cpp	15 Mar 2004 20:04:18 -0000
@@ -1416,43 +1416,108 @@
+  const char *remote = NULL;
+  const char *profile = NULL;
+  const char *program = NULL;
+  const char *username = NULL;

I've seen NULL cause weird compile problems on some platforms when used in C++.
 Probably safer to use 0 or nsnull.

>+
>+      // Get the remote argument and advance past it.
>+      i++;
>+      remote = argv[i];

remote = argv[++i]   might be slightly more efficient.	Same comment applies to
the other cases below where this pattern occurs.

>+    else if (PL_strcasecmp(argv[i], "-b") == 0) {
>+      // someone used the -b <browser> flag - save the contents

I'd prefer if this was	 -a <application>.  We certainly have non-browser XUL
apps that could accept URLs, for example Thunderbird could 

>+  // Same with the program name
>+  if (!program) {
>+    program = MOZ_APP_NAME;
>+  }
>+

Hm, could this just be argv[0] or did you want it to be a 'pretty name' of the
app?

>+  // was there a window not running?

"not a window running" would sound better to me

All of those nsAppRunner.cpp comments apply to the toolkit/xre version as well,
obviously.

>--- xpfe/components/xremote/src/XRemoteService.cpp	10 Feb 2004 00:18:51 -0000	1.33
>+++ xpfe/components/xremote/src/XRemoteService.cpp	15 Mar 2004 20:04:18 -0000
>@@ -99,7 +103,7 @@
> XRemoteService::Observe(nsISupports *aSubject, const char *aTopic,
>                         const PRUnichar *aData)
> {
>-  if (!nsCRT::strcmp(aTopic, "quit-application")) {
>+  if (strcmp(aTopic, "quit-application")) {

Er, did you mean to reverse the test here?  This will call Shutdown() if aTopic
is _not_ quit-application.

>@@ -384,8 +388,13 @@
>     return NS_ERROR_FAILURE;
>   }
> 
>+
>+  nsCAutoString profile;
>+  GetProfileName(profile);
>+
>   nsresult rv;
>-  rv = widgetHelper->EnableXRemoteCommands(mainWidget);
>+  rv = widgetHelper->EnableXRemoteCommands(mainWidget, profile.get(),
>+					   mProgram.get());

Fix the hard tabs.  There are tons of them in the rest of the patch; I'm not
going to mark them all.

>@@ -999,6 +1012,24 @@
>     return NS_ERROR_FAILURE;
> 
>   return mediator->GetMostRecentWindow(aType, _retval);
>+}
>+
>+void
>+XRemoteService::GetProfileName(nsACString &aProfile)
>+{
>+  // Get the current profile name and save it.
>+  nsresult rv;
>+  nsCOMPtr<nsIProfile> profileMgr;
>+  profileMgr = do_GetService(NS_PROFILE_CONTRACTID, &rv);
>+  if (NS_FAILED(rv) || !profileMgr)

do_GetService will never return a success code and a null pointer, or a
non-null pointer and a failure code.  Pick one.

>+    return;
>+
>+  PRUnichar *name;
>+  rv = profileMgr->GetCurrentProfile(&name);
>+  if (NS_FAILED(rv) || !name)

Same comment as above, for GetCurrentProfile().

>+    return;
>+
>+  aProfile = NS_LossyConvertUCS2toASCII(nsDependentString(name));

You can avoid some overhead here if you do:

LossyCopyUTF16toASCII(name, aProfile);

>--- xpfe/components/xremote/src/XRemoteService.h	21 Nov 2003 00:09:45 -0000	1.8
>+++ xpfe/components/xremote/src/XRemoteService.h	15 Mar 2004 20:04:18 -0000
>@@ -104,4 +107,6 @@
>   // have we been started up from the main loop yet?
>   PRBool   mRunning;
> 
>+  // Name of our program
>+  nsCAutoString mProgram;

As a member variable, we generally use nsCString rather than nsCAutoString.

>--- widget/public/nsIXRemoteClient.idl	20 Oct 2000 05:09:06 -0000	1.1
>+++ widget/public/nsIXRemoteClient.idl	15 Mar 2004 20:04:19 -0000
>@@ -31,8 +31,34 @@
>   /**
>    * Sends a command to a running instance.  If it returns false then
>    * there is no running instance.
>+   *
>+   * @param aProgram This is the preferred program that we want to use
>+   * for this particular command.
>+   *
>+   * @param aNoProgramFallback This boolean attribute tells the client
>+   * code that if the preferred program isn't found that it should
>+   * fall not send the command to another server.

s/fall/fail/

>--- widget/src/xremoteclient/XRemoteClient.cpp	21 Nov 2003 00:09:45 -0000	1.13
>+++ widget/src/xremoteclient/XRemoteClient.cpp	15 Mar 2004 20:04:19 -0000
>+    // Check to see if it has the user atom on that window.  If there
>+    // is then we need to make sure that it matches what we have.
>+    const char *username = PR_GetEnv("LOGNAME");
>+    if (aUsername) {
>+      username = aUsername;
>+    }

const char *username;
if (aUsername) {
  username = aUserName;
} else {
  username = PR_GetEnv("LOGNAME");
}

(avoid the call to PR_GetEnv if the username is specified, which the compiler
can't optimize away because it doesn't know the side-effects)

>+    // Check to see if there's a profile name on this window.  If
>+    // there is, then we need to make sure it matches what someone
>+    // passed in.
>+    if (aProfile) {
>+      status = XGetWindowProperty(mDisplay, w, mMozProfileAtom,
>+				  0, (65536 / sizeof(long)),
>+				  False, XA_STRING,
>+				  &type, &format, &nitems, &bytesafter,
>+				  &data_return);
>+
>+      // If there's a username compare it with what we have

s/username/profile name/

>--- widget/src/xremoteclient/mozilla-xremote-client.cpp	12 Dec 2003 23:02:11 -0000	1.2
>+++ widget/src/xremoteclient/mozilla-xremote-client.cpp	15 Mar 2004 20:04:19 -0000
>@@ -34,18 +34,52 @@
>  * ***** END LICENSE BLOCK ***** */
> 
> #include <stdio.h>
>+#include <stdlib.h>
>+#include <plgetopt.h>
> #include "XRemoteClient.h"
> 
>+void print_usage(void);
>+

Make this |static|.

> int main(int argc, char **argv)
> {
>   nsresult rv;
>   XRemoteClient client;
>+  char *browser = NULL;
>+  char *profile = NULL;
>+  char *username = NULL;
>+  char *command = NULL;

Same comments as above about NULL in c++, and s/browser/program/.

r=bryner with all of that fixed.

Comment 7

[Christopher Blizzard (:blizzard)]

Attachment: patch that includes gtk 1.2 support and bryner fixes

This should resolve bryner's issues and also includes fixes for gtk 1.2.

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 144593 [details] [diff] [review]
patch that includes gtk 1.2 support and bryner fixes

Looks good.  One other thing I noticed, around where I had you change it to not
call PR_GetEnv("LOGNAME") if aUsername is specified.  Go ahead and pull that
whole thing out above the loop, it doesn't need to do that for each window it
checks.

Comment 9

[Benjamin Smedberg]

Comment on attachment 144593 [details] [diff] [review]
patch that includes gtk 1.2 support and bryner fixes

oof, this is going to midair horribly with bug 237745 and the single-profile
stuff in the new toolkit (see
http://bugzilla.mozilla.org/attachment.cgi?id=144252&action=view)

What timeframe is this landing in? Immediately is ok, I can work this into my
new profile stuff. If this is going to wait for 1.8a, we might have problems.

Comment 10

[Christopher Blizzard (:blizzard)]

I'm not sure if it will have a big impact or not.  We don't actually try and
figure out what profile is in use until someone starts up the xremote service,
which is after profiles are currently started up.  The xremote startup does
happen in the appshell.  Do we need to delay it more until someone attaches it
to a window?  What's the long-term profile strategy?  Do I need to just rip this
code right out of here?

Comment 11

[Asa Dotzler [:asa]]

Comment on attachment 144593 [details] [diff] [review]
patch that includes gtk 1.2 support and bryner fixes

a=asa (on behalf of drivers) for checkin to 1.7

Comment 12

[Christopher Blizzard (:blizzard)]

Checked in.

Comment 13

[Boris Zbarsky [:bzbarsky]]

It looks like this checkin caused bug 240166 and bug 240174 

Comment 14

[Boris Zbarsky [:bzbarsky]]

Also caused crash bug 240238 

Comment 15

[Warren Togami]

Just a thought, is the MOZ_APP_NAME macro definition in the stuff checked into
trunk?  If MOZ_APP_NAME is not defined, then the macro becomes "" and the
application does not behave well.  I personally have tested it only with the
firefox & thunderbird duo, but I suspect mozilla needs it to be defined too.

Comment 16

[Christopher Blizzard (:blizzard)]

If MOZ_APP_NAME isn't defined, it won't build. :)  Mozilla does define this -
it's just 'mozilla'.

Comment 17

[Wolfgang Rosenauer [:wolfiR]]

if I read this correctly it should now be possible to give optional application
names, usernames and profiles with mozilla -remote or mozilla-xremote-client.

But I see the following now:
- mozilla is running
- a simple mozilla-xremote-client 'ping()' is failing:
/opt/mozilla/lib/mozilla-xremote-client: Error: Failed to send command: No error
string reported..

it's a gtk2 build dated a few hours ago
I have to recheck because I remember it worked one time.

Comment 18

[Wolfgang Rosenauer [:wolfiR]]

sorry for the noise but there seems to be a hickup in my system. I can't
reproducte it anymore.

Comment 19

[Andrew Schultz]

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

Comment 20

[Bogdan Stroe]

Is there any documentation for this?
I know about this page http://www.mozilla.org/unix/remote.html, but what about
the new features that where added (profiles, usernames)?

Comment 21

[Bogdan Stroe]

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