385740 - support multiple apps per content type (MIME type or scheme)

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, enhancement)

Description

[Myk Melez [:myk] [@mykmelez]]

Per the content handling scratchpad, we should support registration of multiple possible apps that can handle a given content type (i.e. either a MIME type or a scheme).

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1 (wip): reads list of possible apps from datasource

Here's a patch for the first half of this work (reading multiple apps from the datasource).  I'll include the second half, i.e. helper methods for adding and removing apps from the datasource, in a future patch (or should they be separate patches?).  Note: this patch requires the patch for bug 384374.

With this patch, the external helper app service reads in a list of possible applications from the datasource and makes it available via the possibleApplicationHandlers attribute of nsIHandlerInfo.  It should work for both MIME and scheme handlers, and I factored out the code that gets the preferred app so both preferred and possible apps are retrieved via the same method.  Along the way I also fixed a few style nits.

Possible applications are defined in the datasource via the NC:possibleApplication property, one per possible app.  Here's an example of a scheme handler with one preferred application (defined with the NC:externalApplication property, as before) and three possible applications:

  <RDF:Description RDF:about="urn:scheme:handler:mailto"
                   NC:alwaysAsk="false"
                   NC:saveToDisk="false"
                   NC:useSystemDefault="false"
                   NC:handleInternal="false">
    <NC:externalApplication 
         RDF:resource="urn:scheme:externalApplication:mailto"/>
    <NC:possibleApplication RDF:resource="urn:foo"/>
    <NC:possibleApplication RDF:resource="urn:bar"/>
    <NC:possibleApplication 
         RDF:resource="urn:scheme:externalApplication:mailto"/>
  </RDF:Description>

Unlike with preferred applications in the past, possible application URIs are now arbitrary rather than structured URNs, since the structure we've been using for preferred applications doesn't lend itself to multiple apps per handler, and arbitrary identifiers are generally more robust than structured ones anyway.

This does mean, however, that we either need to make preferred application URIs be arbitrary as well, change the URI for possible applications when the user makes them preferred, or store separate records for the same application when it is both on the list of possible applications and also currently the preferred one.  And which option we choose will have an impact on backwards compatibility.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: reverts to old method of determining preferred app

> This does mean, however, that we either need to make preferred application 
> URIs be arbitrary as well, change the URI for possible applications when the
> user makes them preferred, or store separate records for the same
> application when it is both on the list of possible applications and also
> currently the preferred one.  And which option we choose will have an impact
> on backwards compatibility.

If I recall correctly, dmose suggested option #2 as the safest (because most backwards-compatible, I think), and I agree, so this patch reverts to the old method of determining the preferred application, which is to look for a resource with a URI of the form:

urn:mimetype|scheme:externalApplication:<content-type>

With the original patch, I also thought there was a bug where an invalid possible app record would result in the entire FillContentHandlerProperties call failing (in which case the app would revert to the default behavior specified by the OS).  But I tested again with this patch, and that's no longer happening (or perhaps my original test was flawed).

Instead, the code properly ignores an invalid possible app record and simply returns an array of valid possible app records (if any), so such invalid records won't prevent valid records from having an effect.

dmose also clued me in that the front-end code currently twiddles the RDF datastore directly, so although we do want helper methods for adding and removing possible apps, those can go into a separate patch.  So this is either ready for review on its own or should get bundled in with a larger patch that actually uses the additional nsIHandlerInfo property this patch provides.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: unrotted, otherwise equivalent to v2

This patch is an unrotted version of patch v2.  As with patch v2, this patch doesn't include an API for modifying the list of possible application handlers, it just makes the external helper app service grab the list from the datasource and modifies nsIHandlerInfo to expose the list as an attribute.

In addition to this work, we'll want a way to modify the list, f.e. by making the list be a mutable array and then extending nsIHandlerService::store to write changes to the array to the datastore.  Or we could add methods to that interface for adding and removing individual handler apps.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #3)
> In addition to this work, we'll want a way to modify the list, f.e. by making
> the list be a mutable array and then extending nsIHandlerService::store to
> write changes to the array to the datastore.  Or we could add methods to that
> interface for adding and removing individual handler apps.

I think the mutable array plan is probably the right one.  Doing it the other way entrains the object model and persistence model more closely.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 275078 [details] [diff] [review]
patch v3: unrotted, otherwise equivalent to v2

This looks great; sr=dmose with added documentation about the new arc in the default mimeTypes.rdf.

One nit:
 
>Index: uriloader/exthandler/nsExternalHelperAppService.h
>===================================================================
>RCS file: /cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.h,v
>retrieving revision 1.85
>diff -u -r1.85 nsExternalHelperAppService.h
>--- uriloader/exthandler/nsExternalHelperAppService.h	30 Jul 2007 23:33:16 -0000	1.85
>+++ uriloader/exthandler/nsExternalHelperAppService.h	3 Aug 2007 04:33:19 -0000
>@@ -258,6 +258,8 @@
>   nsCOMPtr<nsIRDFResource> kNC_HandleInternal;
>   nsCOMPtr<nsIRDFResource> kNC_PrettyName;
>   nsCOMPtr<nsIRDFResource> kNC_UriTemplate;
>+  nsCOMPtr<nsIRDFResource> kNC_ExternalApplication;

This doesn't appear to actually get used.

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 275078 [details] [diff] [review]
patch v3: unrotted, otherwise equivalent to v2

>     /**
>+     * Applications that can handle this content type.  The list will include
>+     * the preferred application handler, if any.
>+     */
>+    attribute nsIArray possibleApplicationHandlers;
Can we get the type[s] of handlers we'll get out of this by chance?

Comment 7

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: addresses review comments

(In reply to comment #4)
> I think the mutable array plan is probably the right one.  Doing it the other
> way entrains the object model and persistence model more closely.

Ok, I have changed the possibleApplicationHandlers attribute to a mutable array.


(In reply to comment #5)
> (From update of attachment 275078 [details] [diff] [review])
> This looks great; sr=dmose with added documentation about the new arc in the
> default mimeTypes.rdf.

Strangely, the three default mimeTypes.rdf files for Firefox don't have any docs at all, but the default mimeTypes.rdf files for mail, calendar, and suite do have documentation, so I've added info about this arc to those files.

In the process, I also updated the docs to include protocol handling, since they were MIME-handling specific.


> One nit:
...
> >+  nsCOMPtr<nsIRDFResource> kNC_ExternalApplication;
> 
> This doesn't appear to actually get used.

Yup, you're right.  I've removed it in this patch.


(In reply to comment #6)
> (From update of attachment 275078 [details] [diff] [review])
> >     /**
> >+     * Applications that can handle this content type.  The list will include
> >+     * the preferred application handler, if any.
> >+     */
> >+    attribute nsIArray possibleApplicationHandlers;
> Can we get the type[s] of handlers we'll get out of this by chance?

Yup, I've changed this comment to specify that the attribute is an array of nsIHandlerApp objects.

I also removed the part of the comment that says the list will include the preferred application handler, if any, since it's not clear that'll always be the case.  Once we get some front-end callers of this interface, it'll become clearer how we use the list.

And I made GetPossibleApplicationHandlers always return an array, even if the handler info object hasn't been initialized from the datasource.  I think that's the right approach, because then callers don't have to first check if the attribute is an array, they can just check its length to see if there are any possible applications.

Since that wasn't a fix for one of his review comments, re-requesting superreview from dmose.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 275078 [details] [diff] [review]
patch v3: unrotted, otherwise equivalent to v2

Oops, forgot to obsolete the previous patch.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: followup patch v1: makes handler service store possible handler apps

Here's a followup patch that makes the handler service store possible handler apps when its store method is called.  Included are tests of the functionality.

I also made a couple of terminology changes in the handler service, changing the words "resource" and "value" to "target" in a few contexts for clarity.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> I also removed the part of the comment that says the list will include the
> preferred application handler, if any, since it's not clear that'll always be
> the case.  Once we get some front-end callers of this interface, it'll become
> clearer how we use the list.
In my testing (Bug 390890) this is the case.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #7)
> 
> I also removed the part of the comment that says the list will include the
> preferred application handler, if any, since it's not clear that'll always be
> the case.  Once we get some front-end callers of this interface, it'll become
> clearer how we use the list.

Given sdwilsh's comment in comment #10, and that "get Possible Application Handlers" more or less implies all possible handlers, can you put that back?

> And I made GetPossibleApplicationHandlers always return an array, even if the
> handler info object hasn't been initialized from the datasource.  I think
> that's the right approach, because then callers don't have to first check if
> the attribute is an array, they can just check its length to see if there are
> any possible applications.
> 
> Since that wasn't a fix for one of his review comments, re-requesting
> superreview from dmose.

Sounds like a good change to me; sr=dmose.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #11)
> 
> > And I made GetPossibleApplicationHandlers always return an array, even if the
> > handler info object hasn't been initialized from the datasource.  I think
> > that's the right approach, because then callers don't have to first check if
> > the attribute is an array, they can just check its length to see if there are
> > any possible applications.

Please document that behavior in the IDL too.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 275268 [details] [diff] [review]
followup patch v1: makes handler service store possible handler apps

Looks great; these changes are all just nits.  sr=dmose with the following addressed:

>-      // Finally, make this app be the preferred app for the handler info.
>+      // Make this app be the preferred app for the handler info.
>       // Note: at least some code completely ignores this setting and assumes
>       // the preferred app is the one whose URI follows the appropriate pattern.

Which code does the above comment apply to?  It's a little unclear from what should be done, and by whom.  Since you're here, can you fix this?

>+    // First, retrieve the set of handler apps currently stored for the type,
>+    // keeping track of their IDs in a hash that we'll use to determine which
>+    // ones are no longer valid and should be removed.
>+    var invalidHandlerApps = {};
>+    var currentHandlerApps = this._getTargets(infoID, NC_POSSIBLE_APP);

How about calling the hash currentHandlerApps and calling the result of getTargets currentHandlerArcs?  I think this would be less confusing.

>+    // Finally, remove any old handler apps that aren't being used anymore,
>+    // and if those handler apps aren't being used by any other type either,
>+    // then completely remove their record from the datastore so we don't
>+    // leave it clogged up with information about handler apps we don't care
>+    // about anymore.

We want to keep an MRU list of recently used apps, but I think that's a separate bug.  Can you spin one off?

>   /**
>+   * Return the unique identifier for a handler app record, which stores
>+   * information about a possible handler for one or more content types,
>+   * including its human-readable name and the path to its executable (for a
>+   * local app) or its URI template (for a web app).
>+   *
>+   * Note: handler app IDs for preferred handlers are different.  For those,
>+   * see the _getPreferredHandlerID method.

How about naming this method _getPossibleHandlerAppID.  This will make the above distinction clear to readers of code that calls this?

>+   *
>+   * @param aHandlerApp  {nsIHandlerApp}   the handler app object
>+   */
>+  _getHandlerAppID: function HS__getHandlerAppID(aHandlerApp) {
>+    var handlerAppID = "urn:handler:";
>+
>+    if (aHandlerApp instanceof Ci.nsILocalHandlerApp)
>+      handlerAppID += "local:" + aHandlerApp.executable.path;
>+    else {
>+      aHandlerApp.QueryInterface(Ci.nsIWebHandlerApp);
>+      handlerAppID += "web:" + aHandlerApp.uriTemplate;

Is there any chance that the %s in the uriTemplate could cause any problems here?  i.e. should we be doing any escaping?

>   /**
>    * Assert an arc into the RDF datasource if there is no arc with the given
>    * source and property; otherwise, if there is already an existing arc,
>-   * change it to point to the given target.
>+   * change it to point to the given target. _setLiteral and _setResource
>+   * call this after converting their string arguments into resources
>+   * and literals, and generally speaking callers should call one of those
>+   * two methods instead of this one.

I think changing "generally speaking" to "most" might be a bit clearer here.

>+  /**
>+   * Whether or not there is an RDF source that has the given property set to
>+   * the given resource target.
>+   * 
>+   * @param propertyURI {string} the URI of the property
>+   * @param targetURI   {string} the URI of the target
>+   *
>+   * @returns {boolean} whether or not there is a source
>+   */
>+  _isResourceTarget: function HS_isResourceTarget(propertyURI, targetURI) {

Calling this _existsResourceTarget would be clearer, I think.

>+  // Helper Functions
>+
>+  function checkIsLocalHandler(handler) {
>+    do_check_eq(typeof handler, "object");
>+    do_check_eq(handler.name, localHandler.name);
>+    do_check_true(handler instanceof Ci.nsILocalHandlerApp);
>+    do_check_eq(handler.executable.path, localHandler.executable.path);
>+  }
>+  
>+  function checkIsWebHandler(handler) {
>+    do_check_eq(typeof handler, "object");
>+    do_check_eq(handler.name, webHandler.name);
>+    do_check_true(handler instanceof Ci.nsIWebHandlerApp);
>+    do_check_eq(handler.uriTemplate, webHandler.uriTemplate);
>+  }

Depending on the enclosing scope to not mess around with localHandler and webHandler at the wrong time seems a little fragile to me.  Maybe just take a second parameter?  

Finally, the QueryInterface makes the code harder to read, I think.  Can you spin off a bug about making nsHandlerApp* and friends use classinfo and excising all the unnecessary QIs?

Comment 14

[Myk Melez [:myk] [@mykmelez]]

[14:38:07]	<dmose>	myk++ for using destructuring assigment!
...
[14:42:28]	<dmose>	although using destructuring assigment in combo with the ternary operator is a bit hard to read
...
[16:04:37]	<myk>	dmose: hmm, indeed, perhaps i should do:
var localPossibleHandler, webPossibleHandler, localIndex;
if (handler1 instanceof Ci.nsILocalHandlerApp)
[localPossibleHandler, webPossibleHandler, localIndex] = [handler1, handler2, 0];
else
[localPossibleHandler, webPossibleHandler, localIndex] = [handler2, handler1, 1];
[16:06:02]	<myk>	or i could do:
var localPossibleHandler = handler1 instanceof Ci.nsILocalHandlerApp ? handler1 : handler2;
var webPossibleHandler = handler1 instanceof Ci.nsIWebHandlerApp ? handler1 : handler2;
localIndex = handler1 instanceof Ci.nsILocalHandlerApp ? 0 : 1;
[16:07:33]	<dmose>	i'd vote for the first of those two options

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: superreview nits

(In reply to comment #11)
> Given sdwilsh's comment in comment #10, and that "get Possible Application
> Handlers" more or less implies all possible handlers, can you put that back?

Yup, it's back.


(In reply to comment #12)
> (In reply to comment #11)
> > 
> > > And I made GetPossibleApplicationHandlers always return an array, even 
> > > if the handler info object hasn't been initialized from the datasource.
> > > I think that's the right approach, because then callers don't have to
> > > first check if the attribute is an array, they can just check its length
> > > to see if there are any possible applications.
> 
> Please document that behavior in the IDL too.

Yup, I have done so.

Carrying forward dmose's superreview.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Attachment: followup patch v2: fixes review nits

(In reply to comment #13)
> (From update of attachment 275268 [details] [diff] [review])
> Looks great; these changes are all just nits.  sr=dmose with the following
> addressed:
> 
> >-      // Finally, make this app be the preferred app for the handler info.
> >+      // Make this app be the preferred app for the handler info.
> >       // Note: at least some code completely ignores this setting and assumes
> >       // the preferred app is the one whose URI follows the appropriate pattern.
> 
> Which code does the above comment apply to?  It's a little unclear from what
> should be done, and by whom.  Since you're here, can you fix this?

Yup, I've expanded the comment with specifics.


> >+    // First, retrieve the set of handler apps currently stored for the type,
> >+    // keeping track of their IDs in a hash that we'll use to determine which
> >+    // ones are no longer valid and should be removed.
> >+    var invalidHandlerApps = {};
> >+    var currentHandlerApps = this._getTargets(infoID, NC_POSSIBLE_APP);
> 
> How about calling the hash currentHandlerApps and calling the result of
> getTargets currentHandlerArcs?  I think this would be less confusing.

currentHandlerApps -> currentHandlerArcs seems fine, although I changed it to currentHandlerTargets, since it's a list of targets rather than arcs.

invalidHandlerApps -> currentHandlerApps trades one problem for another, since in both cases the name is accurate at one point and inaccurate at another.  But perhaps it's better for the name to start out accurate than to end up so, so I've made this change as well.


> >+    // Finally, remove any old handler apps that aren't being used anymore,
> >+    // and if those handler apps aren't being used by any other type either,
> >+    // then completely remove their record from the datastore so we don't
> >+    // leave it clogged up with information about handler apps we don't care
> >+    // about anymore.
> 
> We want to keep an MRU list of recently used apps, but I think that's a
> separate bug.  Can you spin one off?

Yup, I have spun this off as bug 391460.


> >   /**
> >+   * Return the unique identifier for a handler app record, which stores
> >+   * information about a possible handler for one or more content types,
> >+   * including its human-readable name and the path to its executable (for a
> >+   * local app) or its URI template (for a web app).
> >+   *
> >+   * Note: handler app IDs for preferred handlers are different.  For those,
> >+   * see the _getPreferredHandlerID method.
> 
> How about naming this method _getPossibleHandlerAppID.  This will make the
> above distinction clear to readers of code that calls this?

Sure, done.


> >+   * @param aHandlerApp  {nsIHandlerApp}   the handler app object
> >+   */
> >+  _getHandlerAppID: function HS__getHandlerAppID(aHandlerApp) {
> >+    var handlerAppID = "urn:handler:";
> >+
> >+    if (aHandlerApp instanceof Ci.nsILocalHandlerApp)
> >+      handlerAppID += "local:" + aHandlerApp.executable.path;
> >+    else {
> >+      aHandlerApp.QueryInterface(Ci.nsIWebHandlerApp);
> >+      handlerAppID += "web:" + aHandlerApp.uriTemplate;
> 
> Is there any chance that the %s in the uriTemplate could cause any problems
> here?  i.e. should we be doing any escaping?

I don't think so, since we aren't actually extracting the uriTemplate from the ID later, we're just using it as a pseudo-arbitrary identifier string.  As long as we use nsIRDFService::GetResource everywhere to turn this ID string into a resource, whatever transformation that method applies should happen consistently, and the IDs should match correctly.

There's some question about whether we should be using these path/uriTemplate-based identifiers at all.  We could instead pick completely arbitrary identifiers for these records, which is generally good practice, for database design anyway.

But I think this approach is OK for now, although we might want to rethink it when we switch from RDF to SQLite.


> >   /**
> >    * Assert an arc into the RDF datasource if there is no arc with the given
> >    * source and property; otherwise, if there is already an existing arc,
> >-   * change it to point to the given target.
> >+   * change it to point to the given target. _setLiteral and _setResource
> >+   * call this after converting their string arguments into resources
> >+   * and literals, and generally speaking callers should call one of those
> >+   * two methods instead of this one.
> 
> I think changing "generally speaking" to "most" might be a bit clearer here.

Sounds good, done.


> >+  /**
> >+   * Whether or not there is an RDF source that has the given property set to
> >+   * the given resource target.
> >+   * 
> >+   * @param propertyURI {string} the URI of the property
> >+   * @param targetURI   {string} the URI of the target
> >+   *
> >+   * @returns {boolean} whether or not there is a source
> >+   */
> >+  _isResourceTarget: function HS_isResourceTarget(propertyURI, targetURI) {
> 
> Calling this _existsResourceTarget would be clearer, I think.

Sure, that makes sense.  Done.


> >+  // Helper Functions
> >+
> >+  function checkIsLocalHandler(handler) {
> >+    do_check_eq(typeof handler, "object");
> >+    do_check_eq(handler.name, localHandler.name);
> >+    do_check_true(handler instanceof Ci.nsILocalHandlerApp);
> >+    do_check_eq(handler.executable.path, localHandler.executable.path);
> >+  }
> >+  
> >+  function checkIsWebHandler(handler) {
> >+    do_check_eq(typeof handler, "object");
> >+    do_check_eq(handler.name, webHandler.name);
> >+    do_check_true(handler instanceof Ci.nsIWebHandlerApp);
> >+    do_check_eq(handler.uriTemplate, webHandler.uriTemplate);
> >+  }
> 
> Depending on the enclosing scope to not mess around with localHandler and
> webHandler at the wrong time seems a little fragile to me.  Maybe just take a
> second parameter?  

Yeah, in general there's too much in the same scope in these tests, resulting in a potential to introduce bugs.  We should fix that at some point, so I've added a FIXME comment to that effect, and in the meantime I've changed these two functions into a single function called checkHandlersAreEquivalent that takes two nsIHandlerApp objects and makes sure they represent the same handler app (whether local or web).


> Finally, the QueryInterface makes the code harder to read, I think.  Can you
> spin off a bug about making nsHandlerApp* and friends use classinfo and
> excising all the unnecessary QIs?

Yup, I've filed this as bug 391461.

I also included some trivial whitespace changes (aligning things better, removing unnecessary tabs) in uriloader/exthandler/tests/Makefile.in.

Carrying forward dmose's superreview.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Attachment: followup patch v3: tests corrected and one final nit fixed

Hrm, actually checkHandlersAreEquivalent won't work because it relies on instanceof, which doesn't return the correct result for the localHandler and webHandler objects that we've defined internally.

Here's a version that avoids using instanceof on those objects.

Boy, would classinfo be useful.

Also, I missed the one final nit in comment 14.  Here's a version of the followup patch with that nit fixed.

Carrying forward dmose's superreview.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 275865 [details] [diff] [review]
patch v5: superreview nits

nsExternalHelperAppService.cpp
+  NS_ADDREF(*aPossibleApps = possibleApps);
+
+  return rv;

please make that "return NS_OK;"

nsMIMEInfoImpl.cpp
+  if (!mPossibleApplications)
+    mPossibleApplications = do_CreateInstance(NS_ARRAY_CONTRACTID);
+
+  *aPossibleAppHandlers = mPossibleApplications;
+  NS_IF_ADDREF(*aPossibleAppHandlers);

I think you should throw when creating the array fails instead of just returning null


Not sure that you really need a setter function for this... seems like callers can just clear() the array

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v6: review nits

(In reply to comment #18)
> (From update of attachment 275865 [details] [diff] [review])
> nsExternalHelperAppService.cpp
> +  NS_ADDREF(*aPossibleApps = possibleApps);
> +
> +  return rv;
> 
> please make that "return NS_OK;"

Ok, done.


> nsMIMEInfoImpl.cpp
> +  if (!mPossibleApplications)
> +    mPossibleApplications = do_CreateInstance(NS_ARRAY_CONTRACTID);
> +
> +  *aPossibleAppHandlers = mPossibleApplications;
> +  NS_IF_ADDREF(*aPossibleAppHandlers);
> 
> I think you should throw when creating the array fails instead of just
> returning null

Ok, done.


> Not sure that you really need a setter function for this... seems like callers
> can just clear() the array

Indeed.  This version makes the attribute read-only, and the external helper app service retrieves it, clears it, and then fills it in when populating an nsIHandlerInfo object in FillContentHandlerProperties and FillPossibleAppsFromSource.

Carrying forward review and superreview.  This is the version of the patch I'll check in.

Comment 20

[Myk Melez [:myk] [@mykmelez]]

Patch v6 checked in.  Leaving this bug open pending review and checkin of the followup patch.


Checking in netwerk/mime/public/nsIMIMEInfo.idl;
/cvsroot/mozilla/netwerk/mime/public/nsIMIMEInfo.idl,v  <--  nsIMIMEInfo.idl
new revision: 1.33; previous revision: 1.32
done
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.336; previous revision: 1.335
done
Checking in uriloader/exthandler/nsExternalHelperAppService.h;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.h,v  <--  nsExternalHelperAppService.h
new revision: 1.87; previous revision: 1.86
done
Checking in uriloader/exthandler/nsHelperAppRDF.h;
/cvsroot/mozilla/uriloader/exthandler/nsHelperAppRDF.h,v  <--  nsHelperAppRDF.h
new revision: 1.8; previous revision: 1.7
done
Checking in uriloader/exthandler/nsMIMEInfoImpl.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.cpp,v  <--  nsMIMEInfoImpl.cpp
new revision: 1.65; previous revision: 1.64
done
Checking in uriloader/exthandler/nsMIMEInfoImpl.h;
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.h,v  <--  nsMIMEInfoImpl.h
new revision: 1.35; previous revision: 1.34
done
Checking in mail/app/profile/mimeTypes.rdf;
/cvsroot/mozilla/mail/app/profile/mimeTypes.rdf,v  <--  mimeTypes.rdf
new revision: 1.2; previous revision: 1.1
done
Checking in calendar/sunbird/app/profile/mimeTypes.rdf;
/cvsroot/mozilla/calendar/sunbird/app/profile/mimeTypes.rdf,v  <--  mimeTypes.rdf
new revision: 1.2; previous revision: 1.1
done
Checking in suite/locales/en-US/profile/mimeTypes.rdf;
/cvsroot/mozilla/suite/locales/en-US/profile/mimeTypes.rdf,v  <--  mimeTypes.rdf
new revision: 1.9; previous revision: 1.8
done

Comment 21

[alta88]

this scheme does not seem to accommodate arguments. all of the following should be stored and passed to the command line processor (win-centric):

C:\Programs\firefox.exe %uri
C:\Programs\firefox.exe -P myprofile %uri
['|"]C:\Program Files\firefox.exe['|"] -no-remote -P myprofile %uri

just one use case of many: open a document in an editor, and reuse the existing window.  etc etc.

or is this functionality a separate bug?

Comment 22

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #21)
> this scheme does not seem to accommodate arguments. all of the following should
> be stored and passed to the command line processor (win-centric):
> 
> C:\Programs\firefox.exe %uri
> C:\Programs\firefox.exe -P myprofile %uri
> ['|"]C:\Program Files\firefox.exe['|"] -no-remote -P myprofile %uri
> 
> just one use case of many: open a document in an editor, and reuse the existing
> window.  etc etc.
> 
> or is this functionality a separate bug?

Sound like a separate bug.  This bug is about storing and retrieving a list of possible nsIHandlerApp objects as currently defined.  If we need to add attributes to nsIHandlerApp (or, more likely, nsILocalHandlerApp), then we should do so in another bug.

Comment 23

[alta88]

thanks myk, filed https://bugzilla.mozilla.org/show_bug.cgi?id=392205.

since you're in the code, maybe the args placeholder can be snuck in..

Comment 24

[Myk Melez [:myk] [@mykmelez]]

Attachment: followup patch v4: conflicts resolved

Bug 391279 and bug 391152, which were recently checked in, conflicted with this patch, but the conflicts were trivial to resolve.  Here's the patch I'll commit.

Comment 25

[Myk Melez [:myk] [@mykmelez]]

Followup patch checked into the trunk.

Checking in uriloader/exthandler/nsHandlerService.js;
/cvsroot/mozilla/uriloader/exthandler/nsHandlerService.js,v  <--  nsHandlerService.js
new revision: 1.10; previous revision: 1.9
done
Checking in uriloader/exthandler/tests/unit/test_handlerService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/test_handlerService.js,v  <--  test_handlerService.js
new revision: 1.11; previous revision: 1.10
done
Checking in uriloader/exthandler/tests/Makefile.in;
/cvsroot/mozilla/uriloader/exthandler/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.5; previous revision: 1.4
done

Comment 26

[Joe Sabash [:JoeS1]]

On Thunderbird trunk, the checkin on 2007-08-13 caused a popup dialog to select an app when clicking on a link anyplace a link was clicked. Inbox local folders or wherever. Recently this phenom has changed (within the last few days)
Now clicking on a link in mail or local folders calls the default browser without a popup to select an app (as it should)
However, clicking on links in newsgoups does nothing at all.
Links in newsgroups are totally unusable.

Was there a core checkin to a related bug that could have caused this.
Please supply the bug # if so.

Comment 27

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #26)
> On Thunderbird trunk, the checkin on 2007-08-13 caused a popup dialog to 
> select an app when clicking on a link anyplace a link was clicked. Inbox 
> local folders or wherever. Recently this phenom has changed (within the last
> few days) Now clicking on a link in mail or local folders calls the default
> browser without a popup to select an app (as it should)
> However, clicking on links in newsgoups does nothing at all.
> Links in newsgroups are totally unusable.

Just to make sure I understand, was the behavior after the checkin on 2007-08-13 correct, or was it a regression from the behavior before the checkin, and if so, what was the behavior before the checkin?


Also, you said on mda.thunderbird:

> If you save an action, as far as opening a link or calling a helper app,
> there is absolutely no way to edit that pref. Apart from saving your
> mimeTypes.rdf (or a null version of same) you are stuck with your decision.

By "save an action", do you mean that you checked "Do this automatically for files like this from now on." in the "Opening <filename>" dialog, after which Thunderbird started applying the specified action automatically, but the entry for that content type didn't show up in the Download Actions dialog, so you weren't able to remove it by clicking "Remove Action", whereas before the checkin you were able to do that?


> Was there a core checkin to a related bug that could have caused this.
> Please supply the bug # if so.

There have been a number of core checkins, but as far as I know they were all designed to be application-generic so they don't break other apps that use this functionality.  I don't know of a particular bug, and only one bug affecting the files in the uriloader directory was fixed in the week before 2007-10-12 (bug 236389), and that seems very unlikely to cause your problem:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Furiloader&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-05&maxdate=2007-10-12&cvsroot=%2Fcvsroot

In any case, I think it would make sense to file a bug on the problem you're experiencing and mark it as a regression (using the keyword "regression") rather than trying to work it out in this bug report.

Comment 28

[Joe Sabash [:JoeS1]]

Filed bug 414493