Create default command line handler for toolkit startup

RESOLVED FIXED

Status

SeaMonkey
Build Config
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

(Depends on: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

12 years ago
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
(Assignee)

Comment 1

12 years ago
Created attachment 219890 [details]
nsBrowserContentHandler.js
(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).
(Assignee)

Comment 3

12 years ago
(In reply to comment #2)
>I think this would be better as nsSuiteContentHandler
It's actually a conversion of the xpfe nsBrowserContentHandler class.
(Assignee)

Comment 4

12 years ago
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
Attachment #220508 - Flags: superreview?(jag)
Attachment #220508 - Flags: review?(ajschult)
(Assignee)

Comment 5

12 years ago
Created attachment 220513 [details] [diff] [review]
To build the above
Attachment #220513 - Flags: superreview?(jag)
Attachment #220513 - Flags: review?(ajschult)

Updated

12 years ago
Attachment #220513 - Flags: superreview?(jag) → superreview+

Comment 6

12 years ago
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

12 years ago
Comment on attachment 220508 [details]
nsBrowserContentHandler.js

can't make comments on MIME type (application/x-javascript)
Attachment #220508 - Attachment mime type: application/x-javascript → text/plain

Comment 8

12 years ago
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?).
(Assignee)

Comment 9

12 years ago
(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? ;-)
(Assignee)

Comment 10

12 years ago
(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

12 years ago
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

11 years ago
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
(Assignee)

Comment 13

11 years ago
(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

11 years ago
>>>  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.
(Assignee)

Comment 15

11 years ago
Created attachment 221867 [details] [diff] [review]
Updated for comments and includes allmakefiles.sh
Attachment #219890 - Attachment is obsolete: true
Attachment #220508 - Attachment is obsolete: true
Attachment #220513 - Attachment is obsolete: true
Attachment #221867 - Flags: superreview?(jag)
Attachment #221867 - Flags: review?(ajschult)
Attachment #220513 - Flags: review?(ajschult)
Attachment #220508 - Flags: superreview?(jag)
Attachment #220508 - Flags: review?(ajschult)

Comment 16

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

oops.  wrong spot.  this works:

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

Comment 17

11 years ago
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
Attachment #221867 - Flags: review?(ajschult) → review+
(Assignee)

Comment 18

11 years ago
(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

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

That's good.

Comment 20

11 years ago
} catch (e) {
}

Updated

11 years ago
Attachment #221867 - Flags: superreview?(jag) → superreview+
(Assignee)

Comment 21

11 years ago
(In reply to comment #19)
>>How about ,?\s*([^\s]*)\s*\)\s*
>That's good.

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

Both fixed locally.
(Assignee)

Comment 22

11 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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.
Attachment #222522 - Flags: superreview?(neil)
Attachment #222522 - Flags: review?(neil)
(Assignee)

Comment 24

11 years ago
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.
Attachment #222522 - Flags: superreview?(neil)
Attachment #222522 - Flags: superreview+
Attachment #222522 - Flags: review?(neil)
Attachment #222522 - Flags: review+

Updated

11 years ago
Depends on: 338571
Blocks: 340006

Comment 25

10 years ago
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...
Depends on: 550567
You need to log in before you can comment on or make changes to this bug.