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

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

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

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 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 on attachment 220508 [details] nsBrowserContentHandler.js can't make comments on MIME type (application/x-javascript)

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

(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? ;-)

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

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

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

>> \s*,?\s*([^\s]+)\s*\)?\s* >> ^ >Fixed. oops. wrong spot. this works: ,?\s*([^\s]+)?\s*\)\s*

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

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

>How about ,?\s*([^\s]*)\s*\)\s* That's good.

} catch (e) { }

(In reply to comment #19) >>How about ,?\s*([^\s]*)\s*\)\s* >That's good. (In reply to comment #20) >} catch (e) { >} Both fixed locally.

Fix checked in.

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