In bug 392957, the web based protocol handling code is moving from nsExtProtocolChannel to nsMIMEInfoImpl::LaunchWithWebHandler (some day it will eventually end up on nsWebHandlerApp). In the process, nsIHandlerInfo.launchWithURI is getting an optional window context parameter that will be used to parent the dialog, and will also be used as the window to load the web-based handler in. This leaves two problems: * there is no way for a caller to specify that a web-based handler should be loaded in a new tab / window (as per preferences). An example caller that wants this is the "Send Link" item in the context menu that drops down from a link. * many callers still use the deprecated nsIExternalProtocolService.loadUrl which doesn't allow a window context to be passed in. So we need to modify the API to support creating a new tab / window on the fly, and audit all callers to make sure that they're using it appropriately.

When fixing and verifying this bug, we also need to ensure that the parent document doesn't get stomped if somebody puts a URI that is handled by a web-handler into an <object> or <img> tag.

This is a precursor patch to clean things up in preparation for the actual fix; it will leave the code in a more maintainable / understandable state. It's a revised version of the patch that originally began in bug 393122; it allows nsLocalHandlerApp to be over-ridden on per-OS basis, and moves the bulk of the work done in nsIHandlerInfo.launchWithURI to now happen in nsIHandlerApp.launchWithURI. It also fixes bug 382019, and comes with some brand new tests.

(Note that much of the precursor patch is not new code; it's just moving existing code around).

I don't _think_ this patch is going to break OS2, but it might.

I might be a little laggy getting to this review. Sorry; just pretty much swamped with things right now.

Comment on attachment 282730 [details] [diff] [review] precursor patch, v2 sorry for not actually getting to this yesterday, real life intruded netwerk/mime/public/nsIMIMEInfo.idl +[scriptable, uuid(f65554fb-858c-4700-988e-41f6832c2f2b)] interface nsIHandlerInfo : nsISupports { shouldn't you change the nsIHandlerApp IID instead? uriloader/exthandler/nsExternalHelperAppService.cpp + platform_local_handler_app_t *handlerApp( + new platform_local_handler_app_t(EmptyString(), aApplication)); I think this would look nicer if written as: + platform_local_handler_app_t *handlerApp = + new platform_local_handler_app_t(EmptyString(), aApplication); uriloader/exthandler/nsLocalHandlerApp.h + * Launches this application with a single argument (typically either + * a file path or a URI spec). This is meant as a helper method for + * implementations of LaunchWithURI/LaunchDefaultWithFile. well, this class doesn't have a LaunchDefaultWithFile... +#ifdef XP_MAC er... that looks wrong, given that XP_MAC is for mac classic... uriloader/exthandler/tests/browser/browser_handlerApps.js + // if we get this far without an exception, we've passed + ok(true, "webHandler launchWithURI test"); what would really be nice is if you verified that the URL was actually loaded (with an onload handler perhaps?) + exe.initWithPath("C:\\WINDOWS\\SYSTEM32\\HOSTNAME.EXE"); can you get WinD from the directory service instead? Windows doesn't have to be installed in c:\windows

This has all the changes you requested except for renaming to PlatformHandlerApp_t, which I will do before checking in. The code itself is pretty much the same; re-requesting review for the test stuff, which has changed fairly substantially: from a browserchrome test to a mochitest.

Comment on attachment 283445 [details] [diff] [review] precursor patch, v3 uriloader/exthandler/tests/mochitest/handlerApp.xhtml use spaces, not tabs :) + var location = String(window.location).split('?'); just use location.href instead of the String() thing in fact, just use location.search (that includes the question mark, but that shouldn't be a problem) uriloader/exthandler/tests/mochitest/handlerApps.js + uri, newWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor). + getInterface(Components.interfaces.nsIWebNavigation). + QueryInterface(Components.interfaces.nsIDocShell)); please just use a temporary variable for the window context + var osString = Components.classes["@mozilla.org/xre/app-info;1"]. + getService(Components.interfaces.nsIXULRuntime).OS; ... + var dirSvc = Components.classes["@mozilla.org/file/directory_service;1"] + .getService(Components.interfaces.nsIDirectoryServiceProvider); please put the dot in a consistent place. (I like to put it on the second line under the dot in Components.classes) (also for the other xpcom objects in this function) + exe.append("iCal.app"); // lingers after the tests finish, but oh well you can't use the unix codepat? :( + var ioService = Components.classes["@mozilla.org/network/io-service;1"]. + getService(Components.interfaces.nsIIOService); you already have an ioService variable... + // dispose of the running iCal process so it doesn't actually linger after the test finishes? (developers running this test on their machine might get a surprise when their iCal gets killed...) +// document.addEventListener("load", myOnLoad, false); what's with that line? uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml +RHEET! valid HTML would require this to be in a <p> or something similar :) (also in handlerApp.xhtml) +<script class="testbody" type="text/javascript"> +<![CDATA[ + + + + +]]> +</script> that doesn't seem necessary

(In reply to comment #8) > > + exe.append("iCal.app"); // lingers after the tests finish, but oh well > > you can't use the unix codepat? :( No, LaunchWithURI on Mac is for talking to application bundles, not just UNIX executables. > + // dispose of the running iCal process > > so it doesn't actually linger after the test finishes? > > (developers running this test on their machine might get a surprise when their > iCal gets killed...) Yeah, that's probably worse than having it linger around on the test machines, since there's only ever going to be a single copy of it running on OS X anyway. I'll comment that code out and include details about why.

Most of this patch is just moving existing code around. The only two pieces which are actually new code are the tests and nsWebHandlerApp.launchWithURI, which is a JavaScript version of the old C++ nsMIMEInfoBase::LaunchWithWebHandler.

Comment on attachment 283458 [details] [diff] [review] precursor patch, v4 >Index: netwerk/mime/public/nsIMIMEInfo.idl >-[scriptable, uuid(b504f39e-d88a-4435-8e0d-e13f1070f7e7)] >+[scriptable, uuid(8d298761-0963-4c90-99e2-6ea498825e82)] >+ > interface nsIHandlerApp : nsISupports { Please remove the empty line. >Index: uriloader/exthandler/nsLocalHandlerApp.cpp >+nsLocalHandlerApp::LaunchWithIProcess(const nsCString& aArg) >+{ >+ nsresult rv; >+ nsCOMPtr<nsIProcess> process = do_CreateInstance(NS_PROCESS_CONTRACTID, &rv); >+ if (NS_FAILED(rv)) >+ return rv; >+ >+ if (NS_FAILED(rv = process->Init(mExecutable))) >+ return rv; >+ >+ const char *string = aArg.get(); >+ >+ PRUint32 pid; >+ return process->Run(PR_FALSE, &string, 1, &pid); >+} Do you need to deal with %00 or ascii nulls or any such cruft in the uri here? >Index: uriloader/exthandler/nsLocalHandlerApp.h >+#ifdef XP_MACOSX >+# ifndef NSLOCALHANDLERAPPMAC_H_ >+# include "mac/nsLocalHandlerAppMac.h" >+typedef nsLocalHandlerAppMac PlatformLocalHandlerApp_t; >+# endif >+#else >+typedef nsLocalHandlerApp PlatformLocalHandlerApp_t; >+#endif I still don't understand why you need the inner ifndef here. If nsLocalHandlerAppMac.h is included first it'll define NSLOCALHANDLERAPPMAC_H_ and then include this file, when nsLocalHandlerAppMac.h is then included again it'll do nothing since the NSLOCALHANDLERAPPMAC_H_ is already defined. The result of the current solution is that PlatformLocalHandlerApp_t won't get typedefed if nsLocalHandlerAppMac.h is included first, which I'm guessing was not the intention? sr=me with that fixed or explained.

> Do you need to deal with %00 or ascii nulls or any such cruft in the uri here? On UNIX systems, I think we're OK. On Mac and Windows systems, I'm less sure. I'll spin off another bug about this, because the code in this patch is exactly equivalent to the existing code in the tree; this patch won't be introducing any new issues. > I still don't understand why you need the inner ifndef here. If > nsLocalHandlerAppMac.h is included first it'll define NSLOCALHANDLERAPPMAC_H_ > and then include this file, when nsLocalHandlerAppMac.h is then included again > it'll do nothing since the NSLOCALHANDLERAPPMAC_H_ is already defined. The inner ifndef is for the case where we're compiling nsLocalHandlerAppMac.cpp. It includes nsLocalHandlerAppMac.h which itself includes nsLocalHandlerApp.h before actually defining nsLocalHandlerAppMac. Since nsLocalHandlerApp.h tries to do the typedef before nsLocalHandlerAppMac is defined, the compiler falls over. > The result of the current solution is that PlatformLocalHandlerApp_t won't get > typedefed if nsLocalHandlerAppMac.h is included first, which I'm guessing was > not the intention? This is an irrelevant side effect. PlatformLocalHandlerApp_t is intended to be used by consumers of "nsLocalHandlerApp.h", whereas "nsLocalHandlerAppMac.h" is a platform-specific implementation detail which shouldn't be used by consumers at all.

Final patch for checkin.

Comment on attachment 283519 [details] [diff] [review] precursor patch, v5 (checked in) Precursor patch checked in.

Urgh, I screwed up and checked this in during threat-level orange without realizing that this bug hadn't yet gotten blocking-1.9+. That said, I can't imagine shipping 1.9 with major use cases of a P1 feature like this busted, so I believe that leaving it in the tree is the right thing. If people feel strongly that I should back it out; let me know, and I can do that tomorrow.

+# ifndef NSLOCALHANDLERAPPMAC_H_ +# include "mac/nsLocalHandlerAppMac.h" +typedef nsLocalHandlerAppMac PlatformLocalHandlerApp_t; +# endif another solution would be to just forward-declare nsLocalHandlerAppMac...

also, you still have a tab in uriloader/exthandler/tests/mochitest/handlerApp.xhtml

So, did we back this out? It's listed as a beta blocker. Please update.

Poke. Update? Landing?

This patch makes launchWithURI attempt to find and/or open a new browser window/tab if no window context is passed into a web handler. I'm going to split the iframe, object, etc. cases out to another bug.

Comment on attachment 285186 [details] [diff] [review] fix web-based handlers (I'm no browser peer, I didn't review the change in browser/) + windowMediator.getMostRecentWindow("navigator:browser"). :( this will not work for embeddors why can't you demand that people pass in a context? as for the actual code: + var browserDOMWin = + windowMediator.getMostRecentWindow("navigator:browser"). + getInterface(Ci.nsIWebNavigation). + QueryInterface(Ci.nsIDocShellTreeItem). + rootTreeItem.QueryInterface(Ci.nsIInterfaceRequestor). + getInterface(Ci.nsIDOMWindow). + QueryInterface(Ci.nsIDOMChromeWindow). + browserDOMWindow; Does this not work: windowMediator.getMostRecentWindow("navigator:browser") .QueryInterface(Ci.nsIDOMChromeWindow) .browserDOMWindow; hm, you're making use of javascript's weird scoping rules here... (declaring browserDOMWindow in a block, using it outside of it)

Comment on attachment 285186 [details] [diff] [review] fix web-based handlers >+ try { >+ // get browser dom window >+ var browserDOMWin = >+ windowMediator.getMostRecentWindow("navigator:browser"). >+ getInterface(Ci.nsIWebNavigation). >+ QueryInterface(Ci.nsIDocShellTreeItem). >+ rootTreeItem.QueryInterface(Ci.nsIInterfaceRequestor). >+ getInterface(Ci.nsIDOMWindow). >+ QueryInterface(Ci.nsIDOMChromeWindow). >+ browserDOMWindow; >+ } catch (ex) { >+ // if we got an exception, there are several possible reasons why: >+ // a) this gecko embedding doesn't provide an nsIBrowserDOMWindow >+ // implementation (i.e. doesn't support browser-style functionality), >+ // so we need to kick the URL out to the OS default browser. This is >+ // the subject of bug 394479. >+ // b) this embedding does provide an nsIBrowserDOMWindow impl, but >+ // there doesn't happen to be a browser window open at the moment; one >+ // should be opened. It's not clear whether this situation will really >+ // ever occur in real life. If it does, the only API that I can find >+ // that seems reasonably likely to work for most embedders is the >+ // command line handler. >+ // c) something else went wrong >+ >+ // it's not clear how one would differentiate between the three cases >+ // above, so for now we just propagate the exception upwards. >+ throw ex; >+ } This try/catch doesn't do anything for now, and you'll likely have to break up the above expression when you do want to fix the comment, so just remove the try/catch for now. Do leave the comment though.

(In reply to comment #21) > (From update of attachment 285186 [details] [diff] [review]) > (I'm no browser peer, I didn't review the change in browser/) I'll get a browser peer to do that. > + windowMediator.getMostRecentWindow("navigator:browser"). > > :( this will not work for embeddors Well, non-XUL and non-browser embeddors will have problems, yes. As per the comments in the catch clause, we'll have to handle that case in one of a few different ways. > why can't you demand that people pass in a context? In the alwaysAsk case, the caller won't know whether a web-based protocol handler is in use before calling nsIExternalProtocolService.LoadURI. If the caller doesn't have a window/tab to put the web-based handler in (e.g. non-click cases), it will need to open one, which in the case of a local handler would need to be closed, creating a weird user experience. I've thought through a bunch of possible ways to change the API to try and alleviate this problem, but none of them really work, other than having the front-end pass in an interface with an "open a front end window" method. That would be theoretically pure, but makes for an even more complex API. We could hoist the context creation up a level from inside nsWebHandlerApp.js to both exthandler and the content-dispatch-chooser, but that doesn't really buy us much (e.g. we still have a copy of the code in a non-front-end place). > Does this not work: > windowMediator.getMostRecentWindow("navigator:browser") > .QueryInterface(Ci.nsIDOMChromeWindow) > .browserDOMWindow; Yes, fixed. > hm, you're making use of javascript's weird scoping rules here... (declaring > browserDOMWindow in a block, using it outside of it) function scoping ftw! :-D

r and sr comments addressed; also fixed up the nsIMIMEInfo doxygen comments to reflect reality. Still needs rs or r from a browser peer.

Comment on attachment 285891 [details] [diff] [review] fix web-based handlers, v2 (checked in) r=me on the browser.js change.

Patch checked in; Comment 1 has been spun off into bug 400886.

dmose: Can you suggest a way that QA can verify this bug?

Set up one of the web-based mailto: handlers on <http://people.mozilla.com/~ctalbert/>. From the context menu on a web link, select "Send Link...", and makes sure it opens the compose page in the appropriate webmail app. Note that the To: address may actually be bogus (i.e. contain the word "mailto:"), but that's ok, because those test handlers aren't totally complete yet.

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102504 Minefield/3.0a9pre using the steps suggested by dmose in Comment 28.

Attachment 283519 [details] [diff] added: Index: toolkit/toolkit-makefiles.sh MAKEFILES_uriloader=" uriloader/Makefile uriloader/base/Makefile uriloader/exthandler/Makefile uriloader/exthandler/tests/Makefile + uriloader/exthandler/tests/browser/Makefile " but this directory doesn't exist in CVS. Do the tests need to be landed too, or should this change be reverted ?