335550 - Create default command line handler for toolkit startup

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[[e-mail]]

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

[[e-mail]]

Attachment: nsBrowserContentHandler.js

Comment 2

[Mark Banner (:standard8)]

(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

[[e-mail]]

(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

[[e-mail]]

Attachment: 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

[[e-mail]]

Attachment: To build the above

Comment 6

[jag (Peter Annema)]

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]

Comment on attachment 220508 [details]
nsBrowserContentHandler.js

can't make comments on MIME type (application/x-javascript)

Comment 8

[Andrew Schultz]

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

[[e-mail]]

(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

[[e-mail]]

(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]

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]

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

[[e-mail]]

(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]

>>>  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

[[e-mail]]

Attachment: Updated for comments and includes allmakefiles.sh

Comment 16

[Andrew Schultz]

>> \s*,?\s*([^\s]+)\s*\)?\s*
>>                      ^
>Fixed.

oops.  wrong spot.  this works:

,?\s*([^\s]+)?\s*\)\s*

Comment 17

[Andrew Schultz]

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

[[e-mail]]

(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]

>How about ,?\s*([^\s]*)\s*\)\s*

That's good.

Comment 20

[jag (Peter Annema)]

} catch (e) {
}

Comment 21

[[e-mail]]

(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

[[e-mail]]

Fix checked in.

Comment 23

[Mark Banner (:standard8)]

Attachment: 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

[[e-mail]]

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]

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...