174164 - enhancements to xremoteservice (add adressbook)

Status: RESOLVED INCOMPLETE

(Core Graveyard :: X-remote, enhancement)

Description

[Matthew Luckie]

mozilla has an option to start with the addressbook on the command line with
mozilla -addressbook, but no associated xfeDoCommand for people who wish to open
their address book with a -remote directive to a running mozilla instance.

A patch is available at http://moat.nlanr.net/~mjl/patch-xremoteservice that
adds -remote xfeDoCommand(openAddressBook) functionality.

It also ensures that when xfeDoCommand(openInbox) or xfe(openBrowser) is used,
the window is created with resize bars on it.  I consider these two enhancements
to be bug fixes.

Comment 1

[Andrew Schultz]

the unresizable scrollbars only appears with some windowmanagers.  "resizable"
is included in "all", but "all" is ignored because of bug 117114.  the patch in
bug 149126 also fixes this

please create an attachment for your patch

==> XRemote

Comment 2

[Matthew Luckie]

Attachment: adds support for xfeDoCommand(openAddressBook)

Comment 3

[Andrew Schultz]

confirming as RFE

Comment 4

[Andrew Schultz]

Comment on attachment 102701 [details] [diff] [review]
adds support for xfeDoCommand(openAddressBook)

with bug 149126 fixed, the "resizable" fixes are no longer needed.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 102701 [details] [diff] [review]
adds support for xfeDoCommand(openAddressBook)

Just a few minor nits:

>diff -uBwr src.old/XRemoteService.cpp src/XRemoteService.cpp
>--- xpfe/components/xremote/src/XRemoteService.cpp.orig	Fri Aug  2 11:12:36 2002
>+++ xpfw/components/xremote/src/XRemoteService.cpp	Sun Oct 13 11:16:58 2002
>@@ -566,6 +566,13 @@
> }
> 
> nsresult
>+XRemoteService::GetAddressBookLocation(char **_retval)
>+{
>+  *_retval = nsCRT::strdup("chrome://messenger/content/addressbook/addressbook.xul");
>+  return NS_OK;
>+}
>+

Since we no longer have to worry about Mac OS 9, we can ditch
nsCRT::strdup(const char *).  Just use strdup() here instead; many C libraries
have hand-tuned the assembly for this sort of function, and that's performance
nsCRT::strdup won't live up to.

>+nsresult
> XRemoteService::GetMailLocation(char **_retval)
> {
>   // get the mail chrome URL
>@@ -837,7 +844,7 @@
> 	return NS_ERROR_FAILURE;
> 
>       nsCOMPtr<nsIDOMWindow> newWindow;
>-      rv = OpenChromeWindow(0, mailLocation, "chrome,all,dialog=no",
>+      rv = OpenChromeWindow(0, mailLocation, "resizable,chrome,all,dialog=no",
> 			    nsnull, getter_AddRefs(newWindow));
>     }
>   }
>@@ -850,7 +857,7 @@
>       return NS_ERROR_FAILURE;
>     
>     nsCOMPtr<nsIDOMWindow> newWindow;
>-    rv = OpenChromeWindow(0, browserLocation, "chrome,all,dialog=no",
>+    rv = OpenChromeWindow(0, browserLocation, "resizable,chrome,all,dialog=no",
> 			  nsnull, getter_AddRefs(newWindow));
>   }
> 
>@@ -858,6 +865,28 @@
>   else if (aArgument.EqualsIgnoreCase("composemessage")) {
>     nsCString tempString("mailto:");
>     rv = OpenURL(tempString, nsnull, PR_FALSE);
>+  }
>+
>+  // open the address book window
>+  else if (aArgument.EqualsIgnoreCase("openaddressbook")) {
>+    nsCOMPtr<nsIDOMWindowInternal> domWindow;
>+
>+    rv = FindWindow(NS_LITERAL_STRING("mail:addressbook").get(),
>+		    getter_AddRefs(domWindow));
>+
>+    if (NS_FAILED(rv)) return rv;

Please put the if and return on separate lines.  Otherwise it's not possible to
set a breakpoint on the return in most debuggers.

>+
>+    if (domWindow) domWindow->Focus();
>+    else {
>+      nsXPIDLCString abLocation;
>+
>+      GetAddressBookLocation(getter_Copies(abLocation));
>+      if (!abLocation)	return NS_ERROR_FAILURE;

Looks like there's a tab character here instead of an EOL.  Please fix.

With the above changes, sr=dmose.

Comment 6

[[e-mail]]

>+      nsXPIDLCString abLocation;
>+
>+      GetAddressBookLocation(getter_Copies(abLocation));

don't do this. even if you use strdup() like dmose suggests, you are mismatching
the allocators (nsXPIDLString assumes the allocator that ToNewString() and
friends use; this is _not_ the same as malloc/free!).

why do you need to allocate the string anyway? just define a static string in
the global scope (eg at the top of the file):

static const char kAddressBookLocation[] =
"chrome://messenger/content/addressbook/addressbook.xul";

and pass that directly into OpenChromeWindow(). that'll work fine, no?

Comment 7

[Matthew Luckie]

Attachment: adds support for xfeDoCommand(openAddressBook)

this is a revision of a previous patch that includes suggestions made by others
in the bugzilla system on said patch.

Comment 8

[R.K.Aa.]

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

Comment 9

[R.K.Aa.]

There's another patch in the dup: attachment 147062 [details] [diff] [review]

Comment 10

[Benjamin Smedberg]

Mass resolving a bunch of old bugs in the x-remote component in preparation for archiving it. If this bug is still valid and useful, please move it to the "Toolkit: Startup and Profile System" component and reopen it.