Last Comment Bug 335550 - Create default command line handler for toolkit startup
: Create default command line handler for toolkit startup
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Build Config (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
:
Mentors:
Depends on: 550567 338571
Blocks: 334478 340006
  Show dependency treegraph
 
Reported: 2006-04-26 10:02 PDT by neil@parkwaycc.co.uk
Modified: 2010-03-07 09:17 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
nsBrowserContentHandler.js (17.92 KB, application/x-javascript)
2006-04-26 10:04 PDT, neil@parkwaycc.co.uk
no flags Details
nsBrowserContentHandler.js (18.50 KB, text/plain)
2006-05-02 07:27 PDT, neil@parkwaycc.co.uk
no flags Details
To build the above (459 bytes, patch)
2006-05-02 08:32 PDT, neil@parkwaycc.co.uk
jag-mozilla: superreview+
Details | Diff | Splinter Review
Updated for comments and includes allmakefiles.sh (23.61 KB, patch)
2006-05-12 16:26 PDT, neil@parkwaycc.co.uk
ajschult: review+
jag-mozilla: superreview+
Details | Diff | Splinter Review
follow-up remove unnecessary pref (753 bytes, patch)
2006-05-18 11:54 PDT, Mark Banner (:standard8) (afk until 26th July)
neil: review+
neil: superreview+
Details | Diff | Splinter Review

Description neil@parkwaycc.co.uk 2006-04-26 10:02:30 PDT
So that we can actually start the suite with MOZ_XUL_APP=1 we need to replace nsBrowserInstance.cpp with a toolkit-compatible startup handler. Features:
-remote (incomplete; currenly only supports openurl and openbrowser)
-browser <url>
-url <url> (obeys tabbed browsing prefs)
-chrome <url>
-preferences
-silent (shamelessly copied from firefox, forcibly prevents default window)
*general startup prefs
*default window
*content handler
Comment 1 neil@parkwaycc.co.uk 2006-04-26 10:04:35 PDT
Created attachment 219890 [details]
nsBrowserContentHandler.js
Comment 2 Mark Banner (:standard8) (afk until 26th July) 2006-04-26 10:19:06 PDT
(In reply to comment #1)
> Created an attachment (id=219890) [edit]
> nsBrowserContentHandler.js
> 

I think this would be better as nsSuiteContentHandler unless you're planning a separate handler for mailnews stuff (if there is any).
Comment 3 neil@parkwaycc.co.uk 2006-04-26 11:50:34 PDT
(In reply to comment #2)
>I think this would be better as nsSuiteContentHandler
It's actually a conversion of the xpfe nsBrowserContentHandler class.
Comment 4 neil@parkwaycc.co.uk 2006-05-02 07:27:54 PDT
Created attachment 220508 [details]
nsBrowserContentHandler.js

New and improved, catches most* exceptions and handles these -remote commands:
xfedocommand(openbrowser,param) (same as -browser)
xfedocommand(openinbox) (same as -mail)
xfedocommand(composemessage,param) (same as -compose)
mailto(param) (same as -compose)
openurl(url) (same as -url) also openurl(url,new-window)/openurl(url,new-tab)
*except e.g. unknown -remote action which should fail to the caller
Comment 5 neil@parkwaycc.co.uk 2006-05-02 08:32:04 PDT
Created attachment 220513 [details] [diff] [review]
To build the above
Comment 6 jag (Peter Annema) 2006-05-02 21:55:42 PDT
Comment on attachment 220508 [details]
nsBrowserContentHandler.js

Bah, bugzilla won't let me edit this inline.

Anyways:

    if (savedMilestone == "ignore")

savedmstone surely, or make the var actually be called savedMilestone.

Can you name that function needHomePageOverride() to match getHomePageGroup()?

    argstring = Components.classes["@mozilla.org/supports-string;1"]
                            .createInstance(nsISupportsString);

Nit: fix indentation on the wrapped bit.

          uriParam = resolveURIInternal(cmdLine, uriParam);

%s/urlParam/uriParam/g ;-)

Move the general-startup prefix into a const that you can re-use for BROWSER_CONTRACTID

Let me look at this some more later.
Comment 7 Andrew Schultz 2006-05-02 21:58:43 PDT
Comment on attachment 220508 [details]
nsBrowserContentHandler.js

can't make comments on MIME type (application/x-javascript)
Comment 8 Andrew Schultz 2006-05-02 22:25:59 PDT
Comment on attachment 220508 [details]
nsBrowserContentHandler.js

>  var argstring = null;
>  if (args != null) {
>    argstring = Components.classes["@mozilla.org/supports-string;1"]
>                            .createInstance(nsISupportsString);
>    argstring.data = args;
>  }
>  return wwatch.openWindow(parent, url, "", features, argstring);

Passing null is not the same as passing an empty string.  See bug 117114 comment 14 and on.
Passing null kicks windowwatcher into psuedo-dialog mode and ignores various features.  args is null to openwindow for the openinbox (bad) and for handling -chrome (probably bad?).
Comment 9 neil@parkwaycc.co.uk 2006-05-03 02:06:34 PDT
(In reply to comment #7)
>(From update of attachment 220508 [details])
>can't make comments on MIME type (application/x-javascript)
There's a bug filed on bugzilla for this? ;-)
Comment 10 neil@parkwaycc.co.uk 2006-05-03 04:55:17 PDT
(In reply to comment #6)
>(From update of attachment 220508 [details] [edit])
>>    if (savedMilestone == "ignore")
>savedmstone surely, or make the var actually be called savedMilestone.
Fixed.

>Can you name that function needHomePageOverride() to match getHomePageGroup()?
Fixed.

>>    argstring = Components.classes["@mozilla.org/supports-string;1"]
>>                            .createInstance(nsISupportsString);
>Nit: fix indentation on the wrapped bit.
Fixed! (actually, by the fix for ajschult's bug)

>>          uriParam = resolveURIInternal(cmdLine, uriParam);
>%s/urlParam/uriParam/g ;-)
Actually, it's the param to -url so I used %s/uriParam/urlParam/g :-P

>Move the general-startup prefix into a const that you can re-use for
>BROWSER_CONTRACTID
Fixed.

(In reply to comment #8)
>(From update of attachment 220508 [details] [edit])
>>  var argstring = null;
>>  if (args != null) {
>>    argstring = Components.classes["@mozilla.org/supports-string;1"]
>>                            .createInstance(nsISupportsString);
>>    argstring.data = args;
>>  }
>>  return wwatch.openWindow(parent, url, "", features, argstring);
>Passing null is not the same as passing an empty string.
I fixed it to always create the nsISupportsString.
Comment 11 Robert Kaiser 2006-05-03 05:32:55 PDT
Comment on attachment 220513 [details] [diff] [review]
To build the above

>Index: Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/suite/app/Makefile.in,v

Actually, after an IRC discussion with Neil, I wonder if it would better to place the .js in suite/browser/ and add it in the Makefile there (along with creating this Makefile and adding the browser dir to the main suite/ Makefile).
It's got "browser" in its name and handles browser stuff basically (only -remote hooks into other stuff as well), so it should probably go with browser files in the source tree.

BTW, do the mail-specific -remote commands handle the case when mailnews isn't installed?
Comment 12 Andrew Schultz 2006-05-07 02:24:08 PDT
Comment on attachment 220508 [details]
nsBrowserContentHandler.js

>function resolveURIInternal(aCmdLine, aArgument)
>{
>  var uri = aCmdLine.resolveURI(aArgument);

if aArgument is not a file and not valid URI, resolveURI throws.  This matches FF's code, but I don't know how it could work.

>  if (!(uri instanceof nsIFileURL)) {

if (uri && !(uri...

and as long as you're checking for nsIFileURL, can't you combine with the next check instead of try/catching it

>      var width = cmdLine.handleFlagWithParam("width", false);

>      var height = cmdLine.handleFlagWithParam("height", false);

these don't actually work for me.  I traced them getting passed into a gtk2 nsWindow object, but didn't follow beyond there.  The gtk2 nsWindow thought it wasn't a top-level window (mIsTopLevel), which is what I would have expected.  It hit this case 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/widget/src/gtk2/nsWindow.cpp&rev=1.170&mark=3049#3049
which seems to have no effect.

>    try {
>      var remote = cmdLine.handleFlagWithParam("remote", true);
>      if (/^\s*(\w+)\s*\(\s*([^\s,]+)\s*,?\s*([^\s]+)\s*\)\s*$/.test(remote)) {

the argument after the "," is optional:
\s*,?\s*([^\s]+)\s*\)?\s*
                     ^

>        case "ping":

ping gets screened out by nsGTKRemoteService
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/remote/nsGTKRemoteService.cpp&rev=1.5&mark=295,314#285

>      var urlParam = cmdLine.handleFlagWithParam("url", false);

we never see -url because composition grabs it
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/compose/src/nsMsgComposeService.cpp&rev=1.122#1389
perhaps it just needs an #ifndef MOZ_SUITE so it all comes here
Comment 13 neil@parkwaycc.co.uk 2006-05-07 04:44:21 PDT
(In reply to comment #12)
>(From update of attachment 220508 [details])
>>function resolveURIInternal(aCmdLine, aArgument)
>>{
>>  var uri = aCmdLine.resolveURI(aArgument);
>if aArgument is not a file and not valid URI, resolveURI throws.  This matches
>FF's code, but I don't know how it could work.
Ah, so you're thinking this should try the fixup in that case?

>>  if (!(uri instanceof nsIFileURL)) {
>if (uri && !(uri...
instanceof is null-safe...

>and as long as you're checking for nsIFileURL, can't you combine with the next
>check instead of try/catching it
I guess so.

>>      var width = cmdLine.handleFlagWithParam("width", false);
>>      var height = cmdLine.handleFlagWithParam("height", false);
>these don't actually work for me.
IIRC there's a bug filed on the width and height window features not working, but I wanted to be ready in case it got fixed ;-)

>>    try {
>>      var remote = cmdLine.handleFlagWithParam("remote", true);
>>      if (/^\s*(\w+)\s*\(\s*([^\s,]+)\s*,?\s*([^\s]+)\s*\)\s*$/.test(remote)) {
>the argument after the "," is optional:
> \s*,?\s*([^\s]+)\s*\)?\s*
>                      ^
Fixed.

>>        case "ping":
>ping gets screened out by nsGTKRemoteService
Fixed.

>>      var urlParam = cmdLine.handleFlagWithParam("url", false);
>we never see -url because composition grabs it
Sigh. Why can't Scott get anything right? If Mac is passing -url then he should fix his mac registration to pass -compose :-/ I guess this affects us too, making external mailto: links open a browser window/tab.

>perhaps it just needs an #ifndef MOZ_SUITE so it all comes here
Will you file the bug or should I?
Comment 14 Andrew Schultz 2006-05-07 11:26:26 PDT
>>>  var uri = aCmdLine.resolveURI(aArgument);
>>if aArgument is not a file and not valid URI, resolveURI throws.  This matches
>>FF's code, but I don't know how it could work.
>Ah, so you're thinking this should try the fixup in that case?

I filed bug 337020 for getting composition to stop handling -url.

I also noticed that nsBrowserContentHandler has nothing to handle URLs on the commandline without an associated flag (like "-browser http://www.foo.com").  firefox has a nsDefaultCommandLineHandler that does this as well as -url.
Comment 15 neil@parkwaycc.co.uk 2006-05-12 16:26:51 PDT
Created attachment 221867 [details] [diff] [review]
Updated for comments and includes allmakefiles.sh
Comment 16 Andrew Schultz 2006-05-12 21:48:12 PDT
>> \s*,?\s*([^\s]+)\s*\)?\s*
>>                      ^
>Fixed.

oops.  wrong spot.  this works:

,?\s*([^\s]+)?\s*\)\s*
Comment 17 Andrew Schultz 2006-05-13 00:12:28 PDT
Comment on attachment 221867 [details] [diff] [review]
Updated for comments and includes allmakefiles.sh

Just some nits:
> const nsIWebNavigation       = Components.interfaces.nsIWebNavigation;

this is unused

>+function needHomePageOverride(prefs)

>+  } catch (e) {}

for consistency,
} catch (e) {
}

as you have elsewhere when the catch block is empty.
When it's not empty, you have

}
catch (e) {

except for...

>+    } catch (e) {
>+      if (cmdLine.handleFlag("browser", false)) {

with those fixed and the regex fixed (previous comment)

r=ajschult
Comment 18 neil@parkwaycc.co.uk 2006-05-13 01:02:11 PDT
(In reply to comment #16)
>>> \s*,?\s*([^\s]+)\s*\)?\s*
>>>                      ^
>>Fixed.
>oops.  wrong spot.  this works:
>,?\s*([^\s]+)?\s*\)\s*
How about ,?\s*([^\s]*)\s*\)\s*

(In reply to comment #17)
>(From update of attachment 221867 [details] [diff] [review])
>Just some nits:
>> const nsIWebNavigation       = Components.interfaces.nsIWebNavigation;
>this is unused
Fixed.

>>+  } catch (e) {
>>+  }

>>+  } catch (e) {}

>>+  }
>>+  catch (e) {

>>+    } catch (e) {
>>+      if (cmdLine.handleFlag("browser", false)) {
jag, it looks like you get to choose:
a) } catch { or catch { on its own line
b) catch {} or } on its own line?
Comment 19 Andrew Schultz 2006-05-13 09:34:05 PDT
>How about ,?\s*([^\s]*)\s*\)\s*

That's good.
Comment 20 jag (Peter Annema) 2006-05-14 17:17:05 PDT
} catch (e) {
}
Comment 21 neil@parkwaycc.co.uk 2006-05-15 17:01:00 PDT
(In reply to comment #19)
>>How about ,?\s*([^\s]*)\s*\)\s*
>That's good.

(In reply to comment #20)
>} catch (e) {
>}

Both fixed locally.
Comment 22 neil@parkwaycc.co.uk 2006-05-17 04:43:38 PDT
Fix checked in.
Comment 23 Mark Banner (:standard8) (afk until 26th July) 2006-05-18 11:54:01 PDT
Created attachment 222522 [details] [diff] [review]
follow-up remove unnecessary pref

Now that the main patch has landed, we can remove the toolkit.defaultChromeURI pref as our command line handler overrides toolkit's one and uses browser.chromeURL. My suiterunner build on linux works fine without this pref.
Comment 24 neil@parkwaycc.co.uk 2006-05-18 14:06:23 PDT
Comment on attachment 222522 [details] [diff] [review]
follow-up remove unnecessary pref

> // defaultChromeURI is only needed until our default command line handler knows what to launch
>-pref("toolkit.defaultChromeURI","chrome://navigator/content/navigator.xul");
r+sr=me if the comment dies too.
Comment 25 Karsten Düsterloh 2007-06-03 14:57:16 PDT
Comment on attachment 221867 [details] [diff] [review]
Updated for comments and includes allmakefiles.sh

>Index: suite/browser/nsBrowserContentHandler.js
>===================================================================
>+function openWindow(parent, url, features, arg)
>+{
>+  var wwatch = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
>+                         .getService(nsIWindowWatcher);
>+  var argstring = Components.classes["@mozilla.org/supports-string;1"]
>+                            .createInstance(nsISupportsString);
>+  argstring.data = arg;
>+  return wwatch.openWindow(parent, url, "", features, argstring);
>+}
...
>+  /* nsIContentHandler */
>+  handleContent: function handleContent(contentType, context, request) {
>+    var webNavInfo = Components.classes["@mozilla.org/webnavigation-info;1"]
>+                               .getService(nsIWebNavigationInfo);
>+    if (!webNavInfo.isTypeSupported(contentType, null))
>+      throw NS_ERROR_WONT_HANDLE_CONTENT;
>+
>+    var parentWin;
>+    try {
>+      parentWin = context.getInterface(nsIDOMWindow);
>+    } catch (e) {
>+    }
>+
>+    request.QueryInterface(nsIChannel);
>+    openWindow(parentWin, request.URI, null, null);

This can't work. request.URI is an nsIURI, and casting that to a string gives you something like "[xpconnect wrapped nsIURI @ 0x28f4c660 (native @ 0x28f4a434)]" as the url and SM somehow can't open that...

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