Last Comment Bug 280725 - Use generic/automatic remoting in xremote
: Use generic/automatic remoting in xremote
Product: Toolkit
Classification: Components
Component: Startup and Profile System (show other bugs)
: unspecified
: x86 Linux
-- normal (vote)
: mozilla1.8beta2
Assigned To: Benjamin Smedberg [:bsmedberg]
: Benjamin Smedberg [:bsmedberg]
: 263868 297618 (view as bug list)
Depends on:
Blocks: 407800
  Show dependency treegraph
Reported: 2005-02-01 17:18 PST by Benjamin Smedberg [:bsmedberg]
Modified: 2008-07-31 03:01 PDT (History)
14 users (show)
benjamin: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

xremote support for generic commandline remoting, rev. 1 (187.31 KB, patch)
2005-03-04 07:35 PST, Benjamin Smedberg [:bsmedberg]
caillon: first‑review+
Details | Diff | Splinter Review
Forgot xpfe/bootstrap (6.89 KB, patch)
2005-03-04 08:17 PST, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Splinter Review
xremote support for generic commandline remoting, rev. 1.1 (197.63 KB, patch)
2005-04-01 10:16 PST, Benjamin Smedberg [:bsmedberg]
bryner: second‑review+
Details | Diff | Splinter Review
Integrate new component in libxul, rev. 1 (2.36 KB, patch)
2005-04-07 12:16 PDT, Benjamin Smedberg [:bsmedberg]
darin.moz: first‑review+
asa: approval1.8b2+
Details | Diff | Splinter Review
fix sunbird (615 bytes, patch)
2005-04-09 05:11 PDT, Michiel van Leeuwen (email: mvl+moz@)
benjamin: first‑review+
dbaron: approval1.8b2+
Details | Diff | Splinter Review

Description User image Benjamin Smedberg [:bsmedberg] 2005-02-01 17:18:23 PST
This is a bug, following up the command-lines rework I did for the toolkit. The
goal is to have xremote automatically remote a command line the same way windows
does. The "-remote" flag would still be supported as a backwards-compatibility

I originally thought I didn't have time for this in 1.8, but with the extra
6-week beta I think I do. I need to land early to get enough testing time.
Comment 1 User image Benjamin Smedberg [:bsmedberg] 2005-03-04 07:35:47 PST
Created attachment 176285 [details] [diff] [review]
xremote support for generic commandline remoting, rev. 1

This patch appears to work properly on suite and firefox... I'm building tbird
now. The general outline is thus:

The widget code itself doesn't need to know about xremote any more. The xremote
client code still lives in widget/src/xremoteclient, but it is built as part of
tier 50.

There is a new "kind" of xremote command, which uses a different xwindow
property, and remotes an entire command line with working directory. It is only
available for toolkit apps. All of the xremote hacks in are no
longer needed. The xremote version number is bumped from "5.0" to "5.1" for
toolkit apps.

The xremote glue code is unforked between gtk/gtk2, and lives in
toolkit/components/remote. It is built by all apps (including seamonkey).

The old style of xremote requests are still supported by all apps, but the
handling of these requests is pushed into app-specific code. For the toolkit
apps, this is the nsICommandLineHandlers. For the suite, this code lives in
xpfe/components/xremote (the nsIXRemoteService interface has been renamed
Comment 2 User image Benjamin Smedberg [:bsmedberg] 2005-03-04 08:17:46 PST
Created attachment 176288 [details] [diff] [review]
Forgot xpfe/bootstrap

I forgot xpfe/bootstrap in the first patch.
Comment 3 User image Jungshik Shin 2005-03-04 08:24:44 PST
Did you test connecting to a mozilla application running in a locale different
from the locale in which xremote command is invoked? See bug 229755
Comment 4 User image Benjamin Smedberg [:bsmedberg] 2005-03-04 08:35:11 PST
re: multiple locales, no I didn't, and I'm inclined to WONTFIX that bug.
Multiple users/machines xremoting to one display is not a use-case I think we
should worry about.
Comment 5 User image Jungshik Shin 2005-03-04 09:01:12 PST
That bug is  primarily about  passing a non-ASCII command line between a client
and a server (so that bug has to be fixed at least for a single locale/machine
case. perhaps, you fixed it with your patch), but a client and a server can be
on differen machines/locales so that multiple locales/machines issue come out of
that   bug. 
Comment 6 User image Christian :Biesinger (don't email me, ping me on IRC) 2005-03-04 09:50:38 PST
would it be possible to pass a wide string, or an UTF-8 string, over X-Remote,
irrespective of what the locale is?
Comment 7 User image Benjamin Smedberg [:bsmedberg] 2005-03-04 17:32:44 PST
For the toolkit my patch treats all xremote strings as native charset and runs
NS_CopyNativeToUnicode on them. It doesn't touch seamonkey's (broken) behavior.
Comment 8 User image Jungshik Shin 2005-03-04 17:39:16 PST
re comment #6:
Yes. see my patch for bug 229755. That patch works only where UTF8_STRING is
supported (i.e. where XFree86 or recent a version of implementations is
used: virtually all Linux/*BSD/Mac OS X). I'm not sure what AIX/Solaris/HPUX etc
have these days. It's also possible to make it work where UTF8_STRING is not
supported although it's a bit more complicated (but not as complicated as what I
wrote in bug 229755).
Comment 9 User image Christian :Biesinger (don't email me, ping me on IRC) 2005-03-05 06:53:23 PST
(In reply to comment #7)
> For the toolkit my patch treats all xremote strings as native charset and runs
> NS_CopyNativeToUnicode on them. It doesn't touch seamonkey's (broken) behavior.

the approach described in comment 8 does sound like a better way to do this to me...
Comment 10 User image Benjamin Smedberg [:bsmedberg] 2005-03-07 14:45:03 PST
*** Bug 263868 has been marked as a duplicate of this bug. ***
Comment 11 User image Christopher Aillon (sabbatical, not receiving bugmail) 2005-03-31 22:33:17 PST
Comment on attachment 176285 [details] [diff] [review]
xremote support for generic commandline remoting, rev. 1

>Index: mail/components/nsMailDefaultHandler.js

>+function mayOpenURI(uri) {
>+  var ext = Components.classes[";1"]
>+    .getService(Components.interfaces.nsIExternalProtocolService);
>+  return ext.isExposedProtocol(uri.scheme);
>+function openURI(uri)

Nit: pick a style for function declarations (brace on same line or not).

>@@ -57,16 +91,90 @@ var nsMailDefaultHandler = {
>     throw Components.results.NS_ERROR_NO_INTERFACE;
>   },
>   /* nsICommandLineHandler */
>   handle : function mdh_handle(cmdLine) {
>     var uri;
>+    try {
>+      var remoteCommand = cmdLine.handleFlagWithParam("remote", true);
>+    }
>+    catch (e) {
>+      throw NS_ERROR_ABORT;

Is NS_ERROR_ABORT guaranteed defined here?

>Index: widget/src/xremoteclient/XRemoteClient.cpp

>@@ -513,16 +569,20 @@ XRemoteClient::FindBestWindow(const char
>                 XFree(data_return);
>                 continue;
>             }
>             XFree(data_return);
>         }
>     }
>+    // Check to see if the window supports the new command-line passing
>+    // protocol, if that is requested.

Nit: one too many blank lines

>@@ -566,134 +626,213 @@ XRemoteClient::FreeLock(Window aWindow)

>+/* like strcpy, but return the char after the final null */
>+static char*
>+estrcpy(char* s, char* d)
>+  while (*s)
>+    *d++ = *s++;
>+  *d++ = '\0';
>+  return d + 1;

Don't you want |return d|?  The post-increment takes care of the +1 for you.


>Index: widget/src/xremoteclient/nsRemoteClient.h
>RCS file: widget/src/xremoteclient/nsRemoteClient.h
>diff -N widget/src/xremoteclient/nsRemoteClient.h
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ widget/src/xremoteclient/nsRemoteClient.h	4 Mar 2005 15:25:18 -0000
>@@ -0,0 +1,104 @@
>+/* -*- Mode: IDL; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:expandtab:shiftwidth=4:tabstop=4:
>+ */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ *
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Christopher Blizzard. Portions created Christopher Blizzard are Copyright (C) Christopher Blizzard.  All Rights Reserved.
>+ * Portions created by the Initial Developer are Copyright (C) 2001
>+ * the Initial Developer. All Rights Reserved.

Wanna fix this?

Same for:

>Index: toolkit/components/remote/nsGTKRemoteService.cpp

>+#ifdef MOZ_XUL_APP

Does it make sense to just define and get this atom even ifndef MOZ_XUL_APP? 
We won't use it, but getting it isn't going to waste any resources now that its
all retrieved one on trip to the xserver, and it will reduce the amount of
#ifdef-age.  Your call I guess.

>Index: xpfe/components/xremote/src/XRemoteService.cpp
>@@ -1021,27 +700,27 @@ XRemoteService::XfeDoCommand(nsCString &
>   // pass the second argument as parameter
>   arg->SetData(NS_ConvertUTF8toUCS2(restArgument));
>   // someone requested opening mail/news
>   if (aArgument.LowerCaseEqualsLiteral("openinbox")) {
>     // check to see if it's already running
>-    nsCOMPtr<nsIDOMWindowInternal> domWindow;
>+    nsCOMPtr<nsIDOMWindowInternal> aWindow;

Hmm, not sure I like this renaming.  Traditionally aFoo gets applied only to
items passed into the current function directly as an argument.

Think the rest is fine, but it would really be good to get a second set of eyes
on this code (blizzard ideally, but...)
Comment 12 User image Benjamin Smedberg [:bsmedberg] 2005-04-01 10:16:19 PST
Created attachment 179290 [details] [diff] [review]
xremote support for generic commandline remoting, rev. 1.1

Bryner, can you look at this today? I would like to checkin before the weekend
to get a little time to squash any regressions before the freeze next week.
Comment 13 User image Brian Ryner (not reading) 2005-04-04 11:30:25 PDT
Comment on attachment 179290 [details] [diff] [review]
xremote support for generic commandline remoting, rev. 1.1

Looks good.
Comment 14 User image Benjamin Smedberg [:bsmedberg] 2005-04-04 14:01:51 PDT
Fixed, with two bustage changes: first, I didn't add widget/src/xremoteclient to
the root / (it is part of tier_50_dirs now). Second, I didn't add any
EXTRA_DSO_LDOPTS to toolkit/components/remote/, because it built and
worked for me without them. But not for btek, so I added them. I also added
installer packaging for the new files.
Comment 15 User image Philip K. Warren 2005-04-04 16:13:36 PDT
Checked in AIX bustage fix (with r+sr=tor).

Checking in nsGTKRemoteService.h;
/cvsroot/mozilla/toolkit/components/remote/nsGTKRemoteService.h,v  <-- 
new revision: 1.2; previous revision: 1.1
Comment 16 User image Stelian Pop 2005-04-05 06:07:07 PDT
Hmm, something is wrong after the checkins (while trying to compile sunbird):

gmake[6]: Entering directory
ccache g++ -o XRemoteService.o -c -I../../../../dist/include/system_wrappers
-include ../../../../config/gcc_hidden.h -DOSTYPE=\"Linux2.6.10-1\"
-DOSARCH=\"Linux\" -DBUILD_ID=0000000000  -I../../../../dist/include/xpcom
-I../../../../dist/include/string -I../../../../dist/include/dom
-I../../../../dist/include/widget -I../../../../dist/include/uriloader
-I../../../../dist/include/docshell -I../../../../dist/include/pref
-I../../../../dist/include/windowwatcher -I../../../../dist/include/necko
-I../../../../dist/include/appshell -I../../../../dist/include/toolkitcomps
-I../../../../dist/include/appcomps -I../../../../dist/include/exthandler
-I../../../../dist/include/profile -I../../../../dist/include/appcomps
-I../../../../dist/include -I../../../../dist/include/nspr   
-I/usr/X11R6/include   -fPIC  -I/usr/X11R6/include -fno-rtti -fno-exceptions
-Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth
-Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic
-fshort-wchar -pthread  -DDEBUG -D_DEBUG -DDEBUG_admin -DTRACING -g -fno-inline
 -I/usr/X11R6/include -DMOZILLA_CLIENT -include ../../../../mozilla-config.h
-Wp,-MD,.deps/XRemoteService.pp XRemoteService.cpp
XRemoteService.cpp:73:31: nsICmdLineHandler.h: No such file or directory
XRemoteService.cpp: In member function `nsresult
XRemoteService::XfeDoCommand(nsCString&, nsIDOMWindow*)':
XRemoteService.cpp:738: error: `nsICmdLineHandler' undeclared (first use this
XRemoteService.cpp:738: error: (Each undeclared identifier is reported only once
for each function it appears in.)
XRemoteService.cpp:738: error: template argument 1 is invalid
XRemoteService.cpp:738: error: invalid type in declaration before '=' token
XRemoteService.cpp:739: error: cannot convert `const nsGetServiceByContractID'
to `int' in initialization
XRemoteService.cpp:745: error: base operand of `->' is not a pointer
XRemoteService.cpp:748: error: base operand of `->' is not a pointer
gmake[6]: *** [XRemoteService.o] Error 1
gmake[6]: Leaving directory
Comment 17 User image Benjamin Smedberg [:bsmedberg] 2005-04-07 07:01:17 PDT
Reopening for libxul fixup, and I can patch Sunbird if they haven't already.
Comment 18 User image Benjamin Smedberg [:bsmedberg] 2005-04-07 12:16:51 PDT
Created attachment 179972 [details] [diff] [review]
Integrate new component in libxul, rev. 1
Comment 19 User image Asa Dotzler [:asa] 2005-04-07 21:47:21 PDT
Comment on attachment 179972 [details] [diff] [review]
Integrate new component in libxul, rev. 1

Comment 20 User image Darin Fisher 2005-04-07 22:00:29 PDT
Landed "Integrate new component in libxul" patch on trunk.

Does that make this bug fixed?
Comment 21 User image Michiel van Leeuwen (email: mvl+moz@) 2005-04-09 05:11:23 PDT
Created attachment 180158 [details] [diff] [review]
fix sunbird

sunbird is still broken. This patch fixes it for me.
Comment 22 User image Michiel van Leeuwen (email: mvl+moz@) 2005-04-09 10:36:43 PDT
Comment on attachment 180158 [details] [diff] [review]
fix sunbird

patch checked in.
Comment 23 User image Benjamin Smedberg [:bsmedberg] 2005-06-14 08:09:06 PDT
*** Bug 297618 has been marked as a duplicate of this bug. ***
Comment 24 User image Akkana Peck 2005-06-27 16:54:39 PDT
Could this checkin be the reason why firefox will no longer remotely open urls
with commas in them, or handle quoted urls?  The timing seems about right. For

firefox -remote

now chops off everything after the "0,", where it used to work. Quoting the url
inside the parens doesn't help; it gives "Error: Failed to send command: 500
command not parseable", where quoting used to work (and was useful when using
additional arguments such as ,new-tab). This breaks -remote on a lot of common urls.
Comment 25 User image Benjamin Smedberg [:bsmedberg] 2005-06-27 17:39:33 PDT
Akkana, it is likely that the usage you describe was broken by this patch.
Please file a followup bug with a list of old usages that no longer work, and
I'll take a look or find someone to do so.
Comment 26 User image Akkana Peck 2005-06-27 17:59:02 PDT
Filed bug 298960.

Note You need to log in before you can comment on or make changes to this bug.