393492 - move RDF code from nsExternalHelperAppService to nsHandlerService

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

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

We should move the code in nsExternalHelperAppService that accesses the mimeTypes.rdf RDF datasource to nsHandlerService to make it easier to modify that code to meet new requirements, gain the memory safety of a JS implementation, and consolidate datasource manipulation code into one place.

Comment 1

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

Attachment: patch v1: moves RDF to handler service

Here's a patch that moves all the RDF code to the handler service.  I wrote the code so the external helper app service doesn't depend on the handler service being there.  That way, if someone is compiling an app without RDF, they can leave out the handler service as well.

I also noticed that the external helper app service drops its connection to the datasource on profile change, which makes sense, since the datasources are profile-specific.  So I added code to the handler service to do the same thing.

I made a couple minor changes/bug fixes in the handler service itself, including removing the _log method (and its entrained _getAppPref method and _consoleSvc getter) because they aren't being used.

This patch also makes nsIExternalProtocolService::getProtocolHandlerInfo return a handler info object even if it doesn't have access to the datastore.  Before, the method would fail and return null in that case, even though it had already retrieved a handler info object from the OS.  The new behavior is consistent with what nsIMIMEService::getFromTypeAndExtension does, and I think it's what we want here.

I didn't add more tests, since the existing tests exercise this code when they store and then retrieve handler info objects.  But we could probably use some additional tests for broader coverage.

The one issue that needs to be addressed is whether the code that retrieves local handler app paths from the datasource needs the Mac- and Unix-specific implementations of GetFileTokenForPath, and, if so, how to access those from the handler service.  Should we expose the method on an interface and have the handler service call it?

I'll note that, as far as I can tell, only front-end code writes those paths to the datasource when the user selects an application, and the front-end code only has access to cross-platform paths, so it's not clear why native paths/specs would appear in the datasource and need to be converted into nsIFile objects using platform-specific code.

But perhaps I just don't understand the whole thing well enough.  The Mac implementation looks for HFS paths, so perhaps we used to store native paths using back-end code.  On the other hand, the Mac implementation calls GetFileTokenForPath internally on paths not retrieved from the datasource, so perhaps the Mac-specific implementation is only needed for those callers.

Comment 2

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

Attachment: patch v2: fixes minor bug

This patch just fixes a very minor bug (aTypeID should by typeID on one line).

Comment 3

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

The SeaMonkey frontend code allows people to type in paths to helper apps, I'm not sure if it does any checks for validity, so that could be a way for a non-absolute path to get into the datasource. 

Comment 4

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

Comment on attachment 278138 [details] [diff] [review]
patch v2: fixes minor bug

+  nsCOMPtr<nsIHandlerService> handlerSvc = do_GetService(kHandlerServiceCID, &rv);
+  rv = NS_ERROR_FAILURE;

There doesn't seem to be a point in passing rv to do_GetService here...

(why not get the service by contract ID?)

Hrm, I'd have left the fallback to getting via extension when there is no entry for the type in the helper app service. But this works too I guess. Somewhat unfortunate to lose the logging for that case, though.

+    rv = handlerSvc->Retrieve(*_retval, PromiseFlatCString(aFileExt).get());

Perhaps Retrieve should take an ACString? You could just pass "" where you currently pass null, and it would be more consistent with the rest of the functions on the interface




+    if (preferredHandler && !foundPreferredHandler)
+      possibleHandlers.appendElement(preferredHandler, false);

should there be a guarantee on the position of the preferred handler perhaps? Like, should it always be the first handler?

+
+    aHandlerInfo.alwaysAskBeforeHandling =
+      !this._hasValue(infoID, NC_ALWAYS_ASK) ||
+      this._getValue(infoID, NC_ALWAYS_ASK != "false");

That ) seems to be in the wrong place :-)

+      if (fileExtensionTarget instanceof Ci.nsIRDFLiteral
+          && fileExtensionTarget.Value != "")

Seems like the operators (&&) are usually placed at the end of lines here

+    handlerApp.QueryInterface(Ci.nsIHandlerApp);

is that needed? Don't both nsIWebHandlerApp and nsILocalHandlerApp inherit from nsIHandlerApp?

+      return aHandlerApp1.executable.path == aHandlerApp2.executable.path;

What's wrong with .equals()?


+// Class ID for this service.

Please don't put class IDs in IDL files. They are supposed to describe just interfaces. I'd put it in nsCExternalHelperAppService.idl, if you need it from C++ at all.

Comment 5

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

Attachment: patch v3: move type overrides back into helper app service; fix review nits

(In reply to comment #4)
> (From update of attachment 278138 [details] [diff] [review])
> +  nsCOMPtr<nsIHandlerService> handlerSvc = do_GetService(kHandlerServiceCID,
> &rv);
> +  rv = NS_ERROR_FAILURE;
> 
> There doesn't seem to be a point in passing rv to do_GetService here...

Indeed.  rv removed.


> (why not get the service by contract ID?)

I was just mimicking the way the RDF service was obtained, but getting the service by contract ID looks much better.  I've switched to that.


> Hrm, I'd have left the fallback to getting via extension when there is no 
> entry for the type in the helper app service. But this works too I guess.
> Somewhat unfortunate to lose the logging for that case, though.

Hmm, indeed, I think you're right, and it's much better to have the helper app service take responsibility for trying to get by extension when getting by type fails.  Not only does it make the behavior of the helper app service clearer (and allow logging of that behavior), it also makes the handler service's API and code simpler and clearer.

In particular, it means that nsIHandlerService::retrieve no longer accepts a parameter that makes no sense in certain situations (i.e. when the handler info being retrieved is a protocol info).

And since the handler service has getTypeByExtension anyway, it is relatively simple for the helper app service to use that to try to retrieve a type, and, if successful, to use that to retrieve the handler info.

Ok, I have switched back to this behavior.  I didn't reintroduce FillMIMEInfoForExtensionFromDS, which is how the helper app service used to try to retrieve by extension, because that method was small and had only one caller anyway.  So instead I have just inlined the code from that method (updated to the new API, of course) into GetFromTypeAndExtension.

Note that I did make nsIHandlerInfo::type writable as part of this change.  I could have added a "type" parameter to nsIHandlerService::retrieve that would override the type specified in its aHandlerInfo parameter, but I think this way is simpler and cleaner.  I'm happy to switch to an override parameter if you think that's better, however.


> +    rv = handlerSvc->Retrieve(*_retval, PromiseFlatCString(aFileExt).get());
> 
> Perhaps Retrieve should take an ACString? You could just pass "" where you
> currently pass null, and it would be more consistent with the rest of the
> functions on the interface

Ah, good idea.  I'd originally tried to make it an ACString but found I couldn't pass null to it; I didn't think of passing an empty string.  I'd make it an ACString now, but with the change above it's no longer necessary at all, so I've removed the parameter entirely.


> +    if (preferredHandler && !foundPreferredHandler)
> +      possibleHandlers.appendElement(preferredHandler, false);
> 
> should there be a guarantee on the position of the preferred handler perhaps?
> Like, should it always be the first handler?

I don't think so.  It might simplify some things, but it would make other things more complicated.

For example, currently callers who set preferredApplicationHandler are responsible for adding that app to possibleApplicationHandlers.  Requiring them to add it to the beginning of the array would add a constraint that is easily violated (increasing the potential for bugs).

Of course we could ensure that preferredApplicationHandler is always the first element in possibleApplicationHandlers by putting it there ourselves when someone sets the preferred app.  It's unclear whether that's a good idea (I convinced dmose that it wasn't on IRC a few nights ago, but I'm open to having my mind changed).

But even if it was, I think in general adding this promise (and constraint) makes things unnecessarily complicated and doesn't provide enough benefit to callers, who already know that the preferred app is a member of the list (so they don't need to check this themselves) and can figure out which one it is without much trouble, if it matters to them.


> +    aHandlerInfo.alwaysAskBeforeHandling =
> +      !this._hasValue(infoID, NC_ALWAYS_ASK) ||
> +      this._getValue(infoID, NC_ALWAYS_ASK != "false");
> 
> That ) seems to be in the wrong place :-)

Indeed!  Fixed.


> +      if (fileExtensionTarget instanceof Ci.nsIRDFLiteral
> +          && fileExtensionTarget.Value != "")
> 
> Seems like the operators (&&) are usually placed at the end of lines here

Err, right, they are.  Good catch, fixed.


> +    handlerApp.QueryInterface(Ci.nsIHandlerApp);
> 
> is that needed? Don't both nsIWebHandlerApp and nsILocalHandlerApp inherit 
> from nsIHandlerApp?

Nope, it's not needed.  Removed.


> +      return aHandlerApp1.executable.path == aHandlerApp2.executable.path;
> 
> What's wrong with .equals()?

Erm, right, equals is better.  Changed.


> +// Class ID for this service.
> 
> Please don't put class IDs in IDL files. They are supposed to describe just
> interfaces. I'd put it in nsCExternalHelperAppService.idl, if you need it from
> C++ at all.

Ok, removed.  I've just left it out entirely, as it's no longer necessary now that I'm retrieving the service by contract ID.  I instead added a macro for the handler service's contract ID to nsCExternalHelperAppService.idl.

The changes I've made seem too extensive to carry forward biesi's review, so re-requesting review from him.

Comment 6

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

Comment on attachment 278567 [details] [diff] [review]
patch v3: move type overrides back into helper app service; fix review nits

Hm... I think I'd prefer specifying the type as an argument to retrieve. People might expect that changing the type would also affect launchWithFile behaviour (with respect to the system default), or the other default properties. Having the type readonly avoids that.

Oh, re the XXX comment about stored()... I do think that "exists" would be a better name.

Comment 7

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

Attachment: patch v4: addresses review comments

(In reply to comment #6)
> (From update of attachment 278567 [details] [diff] [review])
> Hm... I think I'd prefer specifying the type as an argument to retrieve. 
> People might expect that changing the type would also affect launchWithFile
> behaviour (with respect to the system default), or the other default
> properties. Having the type readonly avoids that.

Ok, here's a version that does that.

I do start to wonder if there's an even better way to do this, perhaps a method for retrieving specifically by extension.  But I think this is good for now.


> Oh, re the XXX comment about stored()... I do think that "exists" would be a
> better name.

Ok, I've renamed it.

Comment 8

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

Comment on attachment 278690 [details] [diff] [review]
patch v4: addresses review comments

Hmm, this patch has a problem.  Obsoleting while I figure it out.

Comment 9

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

Attachment: patch v5: fixes bug in previous patch

That last patch had a bug that I didn't catch until running tests (after I had attached the patch, natch; guess I was irrationally exuberant about having a new version).  Here's an update with the fix (pre-tested this time).

I'll note one worry I have, which is that moz-icon: loads appear to be querying the handler service for a handler.  moz-icon: is used in a number of different places, and some of them may be performance sensitive.  Will this patch cause a performance regression?

It's strange that moz-icon: loads would query the handler service in the first place, since as far as I know that's an "internal" type, and we shouldn't allow users to configure the application to hand them off to helper app.

Is there a way to shortcut the process for such known internal types, and if so, why isn't it working for moz-icon?

Comment 10

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

Note to self: fix this typo on checkin:

      return source..Value;

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 278712 [details] [diff] [review]
patch v5: fixes bug in previous patch

For any doxygen tools to see the relevant comments, I think they need to start with /** instead of /*

>@@ -1413,51 +898,45 @@ nsExternalHelperAppService::GetProtocolH
>   // instead of implementating a separate nsIHandlerInfo object.
>   PRBool exists;
>   *aHandlerInfo = GetProtocolInfoFromOS(aScheme, &exists).get();
>   if (!(*aHandlerInfo)) {
>     // Either it knows nothing, or we ran out of memory
>     return NS_ERROR_FAILURE;
>   }
> 
>-  nsresult rv = FillProtoInfoForSchemeFromDS(aScheme, *aHandlerInfo);     
>-  if (NS_ERROR_NOT_AVAILABLE == rv) {
>+  nsresult rv = NS_ERROR_FAILURE;
>+  nsCOMPtr<nsIHandlerService> handlerSvc = do_GetService(NS_HANDLERSERVICE_CONTRACTID);
>+  if (handlerSvc)

This is kind of a nit, but checking handlerSvc is the wrong thing here; it's within the allowed XPCOM semantics to write junk into an out param or retval, then detect an error and simply return a failure code.  This code wouldn't catch that.  Please fix this as well as the other instances of this pattern.

>+  /* Whether or not two handler apps are equivalent.  Two apps are equivalent
>+   * if they are both either local or web handler apps and their local paths
>+   * (or URI templates, for web apps) are the same in a string comparison.
>+   * Useful for comparing a preferred handler app to possible handler apps
>+   * to determine whether the preferred app is in the set of possible apps.
>+   *
>+   * @param aHandlerApp1  {nsIHandlerApp}  the first handler app to compare
>+   * @param aHandlerApp2  {nsIHandlerApp}  the second handler app to compare
>+   *
>+   */
>+  _handlerAppsAreEquivalent: function HS_handlerAppsAreEquivalent(aHandlerApp1,
>+                                                                  aHandlerApp2) {

This totally wants to be nsIHandlerApp.equals().

>   /**
>-   * Return the unique identifier for a type info record, which stores
>+   * Return the unique identifier for a handler info record, which stores
>    * the preferredAction and alwaysAsk fields plus a reference to the preferred
>-   * handler.  Roughly equivalent to the nsIHandlerInfo interface.
>+   * handler app.  Roughly equivalent to the nsIHandlerInfo interface.
>    * 
>    * |urn:<class>:handler:<type>|
>    * 
>    * FIXME: the type info record should be merged into the type record,
>    * since there's a one to one relationship between them, and this record
>    * merely stores additional attributes of a content type.
>    * 
>-   * @param aHandlerInfo {nsIHandlerInfo} the handler for which to get the ID
>+   * @param aHandlerInfo  {nsIHandlerInfo}  a handler info object representing
>+   *                                        the type for which to get an info ID
>+   * @param aOverrideType {string}          a type to use instead of the one
>+   *                                        specified in the handler info 

Pushing the override type down this far seems kinda weird to me; it makes this API somewhat obtuse.  Maybe it's the best way; I'm not sure.  In any case, the doyxgen comment needs to be more clear on the semantics of aOverrideType, and that it's optional.  Additionally, having the function impl be a single long expression w/multiple boolean subexpressions makes it pretty hard to read.  These comments apply to all three of _getPreferredHandlerID, _getTypeID, and _getInfoID.

>+  _getSourceForLiteral: function HS__getSourceForLiteral(propertyURI, value){ 
>+    var property = this._rdf.GetResource(propertyURI);
>+    var target = this._rdf.GetLiteral(value);
>+
>+    var source = this._ds.GetSource(property, target, true);
>+    if (source)
>+      return source..Value;
>+
>+    return null;
>+  },

Isn't .. only valid when used with e4x stuff?  Also, according to the IDL, .Value is deprecated in favor of .ValueUTF8.
 
>+  /*
>+   * Retrieve a handler info object from the datastore.
>+   *
>+   * Note: because of the way the external helper app service currently mixes
>+   * OS and user handler info in the same handler info object, this method
>+   * takes an existing handler info object (probably retrieved from the OS)
>+   * and "fills it in" with information from the datastore, overriding any
>+   * existing properties on the object with properties from the datastore.

>+  void retrieve(in nsIHandlerInfo aHandlerInfo, in ACString aOverrideType);

I'd prefer calling this fillHandlerInfo() or something to make its function clearer to folks reading calling code.  Then when we simplify the object model, we can rename it to get() or retrieve() or something like that.

>+
>+  /**
>+   * Get the MIME type mapped to the given file extension in the datastore.

>+   *
>+   * @returns the MIME type, if any; otherwise null

Throwing a failure code here instead of returning null seems like it would be a cleaner API and would make the calling code simpler too.

Comment 12

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

>This is kind of a nit, but checking handlerSvc is the wrong thing here; it's
>within the allowed XPCOM semantics to write junk into an out param or retval,
>then detect an error and simply return a failure code. 

do_GetService isn't an XPCOM function as such; it is a C++ helper for XPCOM. Its implementation makes sure that failure returns end up in a null pointer (http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsComponentManagerUtils.cpp#254)

Comment 13

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

Attachment: patch v6: super-review fixes

(In reply to comment #11)
> (From update of attachment 278712 [details] [diff] [review])
> For any doxygen tools to see the relevant comments, I think they need to start
> with /** instead of /*

Err, right, fixed.


> This is kind of a nit, but checking handlerSvc is the wrong thing here; it's
> within the allowed XPCOM semantics to write junk into an out param or retval,
> then detect an error and simply return a failure code.  This code wouldn't
> catch that.  Please fix this as well as the other instances of this pattern.

Leaving these as is per biesi's comment 12 and our conversation on IRC.


> >+  _handlerAppsAreEquivalent: function HS_handlerAppsAreEquivalent(aHandlerApp1,
> >+                                                                  aHandlerApp2) {
> 
> This totally wants to be nsIHandlerApp.equals().

Yeah, definitely.  This patch includes an implementation of that method.  Per our conversations on IRC, passing in a null pointer throws NS_ERROR_NULL_POINTER, but otherwise the method always returns a boolean.


> Pushing the override type down this far seems kinda weird to me; it makes this
> API somewhat obtuse.  Maybe it's the best way; I'm not sure.  In any case, the
> doyxgen comment needs to be more clear on the semantics of aOverrideType, and
> that it's optional.  Additionally, having the function impl be a single long
> expression w/multiple boolean subexpressions makes it pretty hard to read. 
> These comments apply to all three of _getPreferredHandlerID, _getTypeID, and
> _getInfoID.

After our conversation on IRC, I decided to minimize how far the override type goes, so I changed the way retrieve and the three ID getting methods work.  Instead of taking a handler info, the ID getting methods now take a class and type.  And retrieve now does most of the work inline, although it calls out to a couple methods.


> >+      return source..Value;
> >+
> >+    return null;
> >+  },
> 
> Isn't .. only valid when used with e4x stuff?  Also, according to the IDL,
> .Value is deprecated in favor of .ValueUTF8.

Yeah, that's a typo.  Fixed, and I'm now using .ValueUTF8 (found and fixed a few other cases of this as well).


> >+  void retrieve(in nsIHandlerInfo aHandlerInfo, in ACString aOverrideType);
> 
> I'd prefer calling this fillHandlerInfo() or something to make its function
> clearer to folks reading calling code.  Then when we simplify the object model,
> we can rename it to get() or retrieve() or something like that.

Ok, I've renamed it to fillHandlerInfo.


> >+  /**
> >+   * Get the MIME type mapped to the given file extension in the datastore.
> 
> >+   *
> >+   * @returns the MIME type, if any; otherwise null
> 
> Throwing a failure code here instead of returning null seems like it would be a
> cleaner API and would make the calling code simpler too.

Yup, the code now returns NS_ERROR_NOT_AVAILABLE if there's no MIME type with the given extension.

These changes seem significant enough (at least the changes to nsLocalHandlerApp and nsWebHandlerApp) to require re-review, so requesting that from biesi.

Comment 14

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

Attachment: patch v7: one bug fix, one comment fix

A final read-through of patch v6 uncovered one bug and one comment nit that I have fixed in this version of the patch.

Comment 15

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

Comment on attachment 279071 [details] [diff] [review]
patch v7: one bug fix, one comment fix

+  if (!aHandlerApp) return NS_ERROR_NULL_POINTER;

NS_ENSURE_ARG_POINTER

 	

  // If either handler app doesn't have an executable, then they aren't
  // the same app.

Hm... seems that if neither application has a helper app, then they are equal... but perhaps that's rare enough that it doesn't matter

Comment 16

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

Attachment: patch v8: review, bug, test fixes

(In reply to comment #15)
> (From update of attachment 279071 [details] [diff] [review])
> +  if (!aHandlerApp) return NS_ERROR_NULL_POINTER;
> 
> NS_ENSURE_ARG_POINTER

Right, fixed.


>   // If either handler app doesn't have an executable, then they aren't
>   // the same app.
> 
> Hm... seems that if neither application has a helper app, then they are
> equal... but perhaps that's rare enough that it doesn't matter

If neither handler app has an executable, then I would argue that the result is indeterminate, while dmose would (I think) argue that they aren't equal.

But as you note, this is rare enough that it doesn't matter.  In fact, it should be exceedingly rare and only happen if there's a bug, as the executable field is required for handler app records.

In this patch I also add back in a function (_retrieveFileExtensions) from patch v5 that I mistakenly dropped from later patches, fix a logic error (|if (a && b || c)| should be |if (a && (b || c))|), and update the tests to use the new nsIHandlerApp::equals method.

These changes seem minor enough to carry forward biesi's review.

Comment 17

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

Note: despite Bugzilla's warning, interdiff between patch v5 (the last one dmose superreviewed) and patch v8 (the one currently up for superreview) does show the correct set of changes between the two patches:

https://bugzilla.mozilla.org/attachment.cgi?oldid=278712&action=interdiff&newid=279255&headers=1

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #16)
> 
> If neither handler app has an executable, then I would argue that the result is
> indeterminate, while dmose would (I think) argue that they aren't equal.
> 
> But as you note, this is rare enough that it doesn't matter.  In fact, it
> should be exceedingly rare and only happen if there's a bug, as the executable
> field is required for handler app records.
> 

Actually, I agree with biesi.  But indeed, it's not particularly important.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 279255 [details] [diff] [review]
patch v8: review, bug, test fixes

Looks good; just a few nits remaining.  sr=dmose with them addressed.  I think your API choice in w.r.t. overrideType was a good one; the complexity of override types is isolated to a smaller chunk of the code.
 
> NS_IMETHODIMP nsExternalHelperAppService::GetTypeFromExtension(const nsACString& aFileExt, nsACString& aContentType) 
> {
>
> [...]
>
>+  // Check user-set prefs
>+  nsCOMPtr<nsIHandlerService> handlerSvc = do_GetService(NS_HANDLERSERVICE_CONTRACTID);
>+  if (handlerSvc)
>+    rv = handlerSvc->GetTypeFromExtension(aFileExt, aContentType);
>+  if (NS_SUCCEEDED(rv) && !aContentType.IsEmpty())

If the IsEmpty check is really important, I think it should be pushed down into nsHandlerService.getTypeFromExtension()   and that should throw an exception in this case.

>+  getTypeFromExtension: function HS_getTypeFromExtension(aFileExtension) {
>+    var fileExtension = aFileExtension.toLowerCase();
>+    var typeID;
>+
>+    if (this._existsLiteralTarget(NC_FILE_EXTENSION, fileExtension))
>+      typeID = this._getSourceForLiteral(NC_FILE_EXTENSION, fileExtension);
>+    else if (this._existsLiteralTarget(NC_FILE_EXTENSIONS, fileExtension))
>+      typeID = this._getSourceForLiteral(NC_FILE_EXTENSIONS, fileExtension);

It would be helpful to document (in a comment, and possibly in mimeTypes.rdf) why we need both of these arcs.  I see that the old helper app service code didn't read NC_FILE_EXTENSION at all, though helperApps.js does.

>Index: uriloader/exthandler/nsIHandlerService.idl
>===================================================================

>+   * Note: if you specify an override type, then the service will fill in
>+   * the handler info object with information about that type instead of
>+   * the type specified by the object's nsIHandlerInfo::type attribute.
>+   *
>+   * This is useful when you are trying to retrieve information about a MIME
>+   * type that doesn't exist in the datastore, but you have a file extension
>+   * for that type, and nsIHandlerService::getTypeFromExtension returns another
>+   * MIME type that does exist in the datastore and can handle that extension.
>+   *
>+   * In that situation, you can call this method to fill in the handler info
>+   * object with information about that other type by passing the other type
>+   * as the aOverrideType parameter.

As part of the above commentary, can you include an example of a case when this would be necessary?  I suspect this would help people first approaching this code a fair a bit, as the above-described operation is both complex and somewhat abstract.  (As an aside, I kinda suspect that somewhere down the road, we'll win by simplifying process of type overriding even further; though that wants to wait for some of the other simplifications first, I think).

>Index: netwerk/mime/public/nsIMIMEInfo.idl
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/mime/public/nsIMIMEInfo.idl,v

Since you're already touching nsIMIMEInfo, can you add a comment that the alwaysAsk preferredAction has been obsoleted in favor of the separate attribute?  This help folks trying to understand the code, I think. 

>+ * of some sort (either a MIME type or a protocol).
>+ *
>+ * FIXME: now that we've made nsIWebHandlerApp inherit from nsIHandlerApp,
>+ * we should also try to make nsIWebContentHandlerInfo inherit from or possibly
>+ * be replaced by nsIWebHandlerApp.

Referencing the cleanup bug # here would be useful, I think.

>+    /**
>+     * Whether or not the given handler app is logically equivalent to the
>+     * invokant (i.e. they represent the same app).
>+     * 
>+     * Two apps are the same if they are both either local or web handlers
>+     * and their executables/URI templates are the same in a string comparison.
>+     *
>+     * @param aHandlerApp the handler app to compare to the invokant
>+     *
>+     * @returns true if the two are logically equivalent, false otherwise
>+     */
>+    boolean equals(in nsIHandlerApp aHandlerApp);

I think your original naming of "equivalent" was better than my suggestion of "equals", since the hanlder name isn't being.  How about changing this to "isEquivalentTo" for clarity to those reading the calling code?

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Requesting re-consideration of blocking-1.9 status.  There is a moderate amount of risk here, but this bug blocks a P1 (bug 377784), so we need it for M8.  Furthermore, this code is much more maintainable and less fragile than the code it's replacing, plus since it has almost all moved to JS, it cuts the risk of memory-safety bugs a bunch.

Comment 21

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

Attachment: patch v9: superreview fixes

(In reply to comment #19)
> >+    rv = handlerSvc->GetTypeFromExtension(aFileExt, aContentType);
> >+  if (NS_SUCCEEDED(rv) && !aContentType.IsEmpty())
> 
> If the IsEmpty check is really important, I think it should be pushed down 
> into nsHandlerService.getTypeFromExtension()   and that should throw an
> exception in this case.

Ok, nsIHandlerService::getTypeFromExtension now throws NS_ERROR_FAILURE when the type is empty (which should only happen if there is datasource corruption), and the calling code no longer checks if the string is empty.


> >+  getTypeFromExtension: function HS_getTypeFromExtension(aFileExtension) {
> >+    var fileExtension = aFileExtension.toLowerCase();
> >+    var typeID;
> >+
> >+    if (this._existsLiteralTarget(NC_FILE_EXTENSION, fileExtension))
> >+      typeID = this._getSourceForLiteral(NC_FILE_EXTENSION, fileExtension);
> >+    else if (this._existsLiteralTarget(NC_FILE_EXTENSIONS, fileExtension))
> >+      typeID = this._getSourceForLiteral(NC_FILE_EXTENSIONS, fileExtension);
> 
> It would be helpful to document (in a comment, and possibly in mimeTypes.rdf)
> why we need both of these arcs.  I see that the old helper app service code
> didn't read NC_FILE_EXTENSION at all, though helperApps.js does.

Per our conversation on IRC, removing NC_FILE_EXTENSION entirely, as it isn't actually used anymore (helperApps.js just returns nsIMIMEInfo::primaryExtension in its place, which just returns the first item from NC_FILE_EXTENSIONS).


> >Index: uriloader/exthandler/nsIHandlerService.idl
> >===================================================================
> 
> >+   * Note: if you specify an override type, then the service will fill in
> >+   * the handler info object with information about that type instead of
> >+   * the type specified by the object's nsIHandlerInfo::type attribute.
> >+   *
> >+   * This is useful when you are trying to retrieve information about a MIME
> >+   * type that doesn't exist in the datastore, but you have a file extension
> >+   * for that type, and nsIHandlerService::getTypeFromExtension returns another
> >+   * MIME type that does exist in the datastore and can handle that extension.
> >+   *
> >+   * In that situation, you can call this method to fill in the handler info
> >+   * object with information about that other type by passing the other type
> >+   * as the aOverrideType parameter.
> 
> As part of the above commentary, can you include an example of a case when 
> this would be necessary?

Ok, I've added the following commentary:

   * For example, the user clicks on a link, and the content has a MIME type
   * that isn't in the datastore, but the link has a file extension, and that
   * extension is associated with another MIME type in the datastore (perhaps
   * an unofficial MIME type preceded an official one, like with image/x-png
   * and image/png).


> I suspect this would help people first approaching this
> code a fair a bit, as the above-described operation is both complex and
> somewhat abstract.  (As an aside, I kinda suspect that somewhere down the
> road, we'll win by simplifying process of type overriding even further;
> though that wants to wait for some of the other simplifications first, I
> think).

Right!  Among other things, splitting handler info into OS-specific and app-specific objects may help in this regard.


> >Index: netwerk/mime/public/nsIMIMEInfo.idl
> >===================================================================
> >RCS file: /cvsroot/mozilla/netwerk/mime/public/nsIMIMEInfo.idl,v
> 
> Since you're already touching nsIMIMEInfo, can you add a comment that the
> alwaysAsk preferredAction has been obsoleted in favor of the separate
> attribute?  This help folks trying to understand the code, I think. 

Not changing this per our conversation in IRC, as it has been annotated with a different comment.


> >+ * of some sort (either a MIME type or a protocol).
> >+ *
> >+ * FIXME: now that we've made nsIWebHandlerApp inherit from nsIHandlerApp,
> >+ * we should also try to make nsIWebContentHandlerInfo inherit from or possibly
> >+ * be replaced by nsIWebHandlerApp.
> 
> Referencing the cleanup bug # here would be useful, I think.

Filed as bug 394710.  I've added the bug number to the comment.


> >+    boolean equals(in nsIHandlerApp aHandlerApp);
> 
> I think your original naming of "equivalent" was better than my suggestion of
> "equals", since the hanlder name isn't being.  How about changing this to
> "isEquivalentTo" for clarity to those reading the calling code?

Like this method, nsIMIMEInfo::equals and nsIWebContentHandlerInfo::equals both match on determinative information like type and URI but not descriptive information like name.  Perhaps those methods should be called isEquivalentTo as well, but per our conversation on IRC, I'll leave this as is for consistency with those other two methods.

Comment 22

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

Comment on attachment 279415 [details] [diff] [review]
patch v9: superreview fixes

Requesting checkin approval.  This fixes a wanted-1.9 bug for which blocking-1.9 has been re-requested as it blocks a Firefox 3 blocker (bug 377784).

The patch moves the RDF code in the C++ nsExternalHelperAppService to the JS nsHandlerService, fixing bug 393500 in the process and also improving memory safety and maintainability.

This patch has moderate risk due to sheer size.  Risk is mitigated, however, by the fact that little new logic has been added.  Mostly we just migrated logic from C++ into JS.

Comment 23

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

Checking in uriloader/exthandler/Makefile.in;
/cvsroot/mozilla/uriloader/exthandler/Makefile.in,v  <--  Makefile.in
new revision: 1.69; previous revision: 1.68
done
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.340; previous revision: 1.339
done
Checking in uriloader/exthandler/nsExternalHelperAppService.h;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.h,v  <--  nsExternalHelperAppService.h
new revision: 1.88; previous revision: 1.87
done
Checking in uriloader/exthandler/nsCExternalHandlerService.idl;
/cvsroot/mozilla/uriloader/exthandler/nsCExternalHandlerService.idl,v  <--  nsCExternalHandlerService.idl
new revision: 1.9; previous revision: 1.8
done
Checking in uriloader/exthandler/nsHandlerService.js;
/cvsroot/mozilla/uriloader/exthandler/nsHandlerService.js,v  <--  nsHandlerService.js
new revision: 1.11; previous revision: 1.10
done
Removing uriloader/exthandler/nsHelperAppRDF.h;
/cvsroot/mozilla/uriloader/exthandler/nsHelperAppRDF.h,v  <--  nsHelperAppRDF.h
new revision: delete; previous revision: 1.8
done
Checking in uriloader/exthandler/nsIHandlerService.idl;
/cvsroot/mozilla/uriloader/exthandler/nsIHandlerService.idl,v  <--  nsIHandlerService.idl
new revision: 1.6; previous revision: 1.5
done
Checking in uriloader/exthandler/nsLocalHandlerApp.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsLocalHandlerApp.cpp,v  <--  nsLocalHandlerApp.cpp
new revision: 1.2; previous revision: 1.1
done
Checking in uriloader/exthandler/nsWebHandlerApp.js;
/cvsroot/mozilla/uriloader/exthandler/nsWebHandlerApp.js,v  <--  nsWebHandlerApp.js
new revision: 1.2; previous revision: 1.1
done
Checking in netwerk/mime/public/nsIMIMEInfo.idl;
/cvsroot/mozilla/netwerk/mime/public/nsIMIMEInfo.idl,v  <--  nsIMIMEInfo.idl
new revision: 1.37; previous revision: 1.36
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.12; previous revision: 1.11
done

Comment 24

[Alex Vincent [:WeirdAl]]

This is causing LOTS of errors:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Component is not available' when calling method: [nsIHandlerService::getTypeFromExtension]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///home/ajvincent/trunk/fx-debug/dist/bin/components/nsBrowserGlue.js :: anonymous :: line 289"  data: no]
************************************************************
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007090420 Minefield/3.0a8pre

Jesse Ruderman reports:

i get a zillion of these when i start firefox
************************************************************
* Call to xpconnect wrapped JSObject produced this error: *
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: file:///Users/jruderman/trunk/mozilla/obj-debug/dist/MinefieldDebug.app/Contents/MacOS/components/nsHandlerService.js :: anonymous :: line 656" data: no]
************************************************************
<Jesse> debug build from earlier today, mac

Comment 25

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

(In reply to comment #24)
> This is causing LOTS of errors:

Yes, that's bug 393627.  The problem is not in this change.  This change merely exposes the problem more.