391152 - support enumeration and removal of known handlers

Status: RESOLVED FIXED

(Firefox :: File Handling, enhancement)

Description

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

The handler service should support enumeration and removal of known handlers so the Download Actions dialog (which I'm modifying over in bug 377784) can enumerate and remove handlers via an API rather than by directly modifying the datasource.

Comment 1

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

Attachment: patch v1: adds enumerate and remove methods to nsIHandlerService

Here's a patch that adds enumerate and remove methods to the handler service for enumerating the list of known handlers and removing a given handler, respectively.

Note that this patch will conflict with the patch for bug 385740, but the conflicts should be trivial and easy (although time-consuming) to resolve.

Comment 2

[Shawn Wilsher :sdwilsh]

by enumerate, you mean that it will add ones not already added, correct?

Comment 3

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

(In reply to comment #2)
> by enumerate, you mean that it will add ones not already added, correct?

No, I mean that it will return a list of the ones it knows about.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 275520 [details] [diff] [review]
patch v1: adds enumerate and remove methods to nsIHandlerService

This looks good, but I'd like to see some tests for both of the new methods.  

A few nits:

>+  enumerate: function HS_enumerate() {
>+    var handlers = Cc["@mozilla.org/array;1"].
>+                   createInstance(Ci.nsIMutableArray);
>+    this._appendHandlers(handlers, CLASS_MIMEINFO);
>+    this._appendHandlers(handlers, CLASS_PROTOCOLINFO);
>+    return handlers.QueryInterface(Ci.nsIArray).enumerate();

Given that nsIMutableArray inherits from nsIArray, is a QI actually necessary here?

>+  remove: function HS_remove(aHandlerInfo) {
>+    var preferredHandlerID = this._getPreferredHandlerID(aHandlerInfo);
>+    this._removeAssertions(preferredHandlerID);
>+
>+    var infoID = this._getInfoID(aHandlerInfo);
>+    this._removeAssertions(infoID);
>+
>+    var typeID = this._getTypeID(aHandlerInfo);
>+    this._removeAssertions(typeID);
>+
>+    var typeList = this._ensureAndGetTypeList(this._getClass(aHandlerInfo));
>+    var type = this._rdf.GetResource(typeID);
>+    if (typeList.IndexOf(type) != -1)
>+      typeList.removeElementAt(typeList.IndexOf(type), true);

The above four lines are a bit difficult to follow.  Please add some commentary.

>+    // Write the changes to the database immediately so we don't lose them
>+    // if the application crashes.
>+    // XXX If we're removing a bunch of handlers at once, will flushing
>+    // after every removal cause a significant performance hit?

Could well be; if so, it should be easy to add a batching mechanism.

>   /**
>+   * Append known handlers of the given class to the given array.  The class
>+   * can be either CLASS_MIMEINFO or CLASS_PROTOCOLINFO.
>+   *
>+   * @param aHandlers   {array} the array of handlers to append to
>+   * @param aClass      {string} the class for which to append handlers
>+   */
>+  _appendHandlers: function HS__appendHandlers(aHandlers, aClass) {
>+    var typeList = this._ensureAndGetTypeList(aClass);
>+    var enumerator = typeList.GetElements();
>+
>+    while (enumerator.hasMoreElements()) {
>+      var element = enumerator.getNext();
>+      
>+      if (!(element instanceof Ci.nsIRDFResource))
>+        continue;

Can you add a comment explaining in what case the above clause would be triggered?

>+      var type = this._getValue(element.ValueUTF8, NC_VALUE);
>+      if (!type)
>+        continue;

A comment explaining exactly what's happening in the |var| line would be helpful here.

Comment 5

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

Attachment: patch v2: adds tests & fixes review nits, syntax error, & test datasource file persistence

(In reply to comment #4)
> (From update of attachment 275520 [details] [diff] [review])
> This looks good, but I'd like to see some tests for both of the new methods.  

Here's a version with tests, one of which uncovered a syntax error (removeElementAt -> RemoveElementAt) that is fixed in this version of the patch (yay tests!).


> A few nits:
> 
> >+  enumerate: function HS_enumerate() {
> >+    var handlers = Cc["@mozilla.org/array;1"].
> >+                   createInstance(Ci.nsIMutableArray);
> >+    this._appendHandlers(handlers, CLASS_MIMEINFO);
> >+    this._appendHandlers(handlers, CLASS_PROTOCOLINFO);
> >+    return handlers.QueryInterface(Ci.nsIArray).enumerate();
> 
> Given that nsIMutableArray inherits from nsIArray, is a QI actually necessary
> here?

Hmm, indeed it isn't.  I've removed the call to QueryInterface.


> >+  remove: function HS_remove(aHandlerInfo) {
> >+    var preferredHandlerID = this._getPreferredHandlerID(aHandlerInfo);
> >+    this._removeAssertions(preferredHandlerID);
> >+
> >+    var infoID = this._getInfoID(aHandlerInfo);
> >+    this._removeAssertions(infoID);
> >+
> >+    var typeID = this._getTypeID(aHandlerInfo);
> >+    this._removeAssertions(typeID);
> >+
> >+    var typeList = this._ensureAndGetTypeList(this._getClass(aHandlerInfo));
> >+    var type = this._rdf.GetResource(typeID);
> >+    if (typeList.IndexOf(type) != -1)
> >+      typeList.removeElementAt(typeList.IndexOf(type), true);
> 
> The above four lines are a bit difficult to follow.  Please add some
> commentary.

Yup, I've commented that set of statements thusly:

    // Now that there's no longer a handler for this type, remove the type
    // from the list of types for which there are known handlers.


> >+    // Write the changes to the database immediately so we don't lose them
> >+    // if the application crashes.
> >+    // XXX If we're removing a bunch of handlers at once, will flushing
> >+    // after every removal cause a significant performance hit?
> 
> Could well be; if so, it should be easy to add a batching mechanism.

Yeah.  The old Download Actions dialog had a batching mechanism, but it also had a mechanism for selecting multiple types and then removing them all at once.  It doesn't seem like the new Applications prefpane will have such functionality--or much need for it, for that matter--given the mockup and discussions in bug 377784 about grouping types by category.

I'll leave this batchless for now but leave the note in the code in case we end up wanting this at a later date.


> >   /**
> >+   * Append known handlers of the given class to the given array.  The class
> >+   * can be either CLASS_MIMEINFO or CLASS_PROTOCOLINFO.
> >+   *
> >+   * @param aHandlers   {array} the array of handlers to append to
> >+   * @param aClass      {string} the class for which to append handlers
> >+   */
> >+  _appendHandlers: function HS__appendHandlers(aHandlers, aClass) {
> >+    var typeList = this._ensureAndGetTypeList(aClass);
> >+    var enumerator = typeList.GetElements();
> >+
> >+    while (enumerator.hasMoreElements()) {
> >+      var element = enumerator.getNext();
> >+      
> >+      if (!(element instanceof Ci.nsIRDFResource))
> >+        continue;
> 
> Can you add a comment explaining in what case the above clause would be
> triggered?

Yup, I've added this comment:

      // This should never happen.  If it does, that means our datasource
      // is corrupted with type list entries that point to literal values
      // instead of resources.  If it does happen, let's just do our best
      // to recover by ignoring this entry and moving on to the next one.


> >+      var type = this._getValue(element.ValueUTF8, NC_VALUE);
> >+      if (!type)
> >+        continue;
> 
> A comment explaining exactly what's happening in the |var| line would be
> helpful here.

Sure.  I've added this comment:

      // Get the value of the element's NC:value property, which contains
      // the MIME type or scheme for which we're retrieving a handler info.


I also made some modifications to the testing framework after I noticed that in some cases datasource files were surviving the test run, which is problematic because it can hork subsequent test runs that expect to start from scratch with a default datasource file.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 275855 [details] [diff] [review]
patch v2: adds tests & fixes review nits, syntax error, & test datasource file persistence

Looks good; sr=dmose.

>+  /**
>+   * Remove the given handler info object from the datastore.  Deletes all
>+   * records associated with the object, including the preferred handler, info,
>+   * and type records plus the entry in the list of types.
>+   *
>+   * @param aHandlerInfo  the handler info object
>+   */
>+  void remove(in nsIHandlerInfo aHandlerInfo);

It's probably worth documenting here what the behavior is if aHandlerInfo doesn't exist (exception or not?).

>+  // Initialization & Destruction
>+  
>+  init: function HandlerServiceTest_init() {
>+    // Register ourselves as a directory provider for the datasource file
>+    // if there isn't one registered already.
>+    try        { this._dirSvc.get("UMimTyp", Ci.nsIFile) }
>+    catch (ex) { this._dirSvc.registerProvider(this) }

Out of curiousity, why is the try/catch clause necessary -- what's the case where we won't be registering ourselves?

Comment 7

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

Attachment: patch v3: fixes one final review nit

(In reply to comment #6)
> (From update of attachment 275855 [details] [diff] [review])
> Looks good; sr=dmose.
> 
> >+  /**
> >+   * Remove the given handler info object from the datastore.  Deletes all
> >+   * records associated with the object, including the preferred handler, info,
> >+   * and type records plus the entry in the list of types.
> >+   *
> >+   * @param aHandlerInfo  the handler info object
> >+   */
> >+  void remove(in nsIHandlerInfo aHandlerInfo);
> 
> It's probably worth documenting here what the behavior is if aHandlerInfo
> doesn't exist (exception or not?).

Sure.  Here's a version that documents that the method doesn't do anything and doesn't return an error if aHandlerInfo doesn't exist in the datastore.


> >+  // Initialization & Destruction
> >+  
> >+  init: function HandlerServiceTest_init() {
> >+    // Register ourselves as a directory provider for the datasource file
> >+    // if there isn't one registered already.
> >+    try        { this._dirSvc.get("UMimTyp", Ci.nsIFile) }
> >+    catch (ex) { this._dirSvc.registerProvider(this) }
> 
> Out of curiousity, why is the try/catch clause necessary -- what's the case
> where we won't be registering ourselves?

The clause is just future-proofing.  At the moment it isn't necessary, because there isn't a directory service provider for the UMimTyp file.  But if that ever changes (f.e. someone modifies the test harness to provide an explicit profile directory and associated files, or perhaps some command-line option like -profile results in this without even changing the test harness), then this code ensures that we use the existing directory provider rather than our own.

At least, that's what I think is going on.  I modeled this code after the Places tests, and they do it this way.

Carrying forward dmose's superreview.

Comment 8

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

Comment on attachment 275887 [details] [diff] [review]
patch v3: fixes one final review nit

+    if (typeList.IndexOf(type) != -1)
+      typeList.RemoveElementAt(typeList.IndexOf(type), true);

I'd store the result of IndexOf in a variable to avoid having to call it twice

+      if (typeList.Resource.Value == "urn:mimetypes:root")
+        handler = this._mimeSvc.getFromTypeAndExtension(type, null);
+      else
+        handler = this._protocolSvc.getProtocolHandlerInfo(type);
+      if (!handler)
+        continue;

can this actually be null? Wouldn't an exception be thrown instead?

Comment 9

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

Attachment: patch v4: review nits

(In reply to comment #8)
> (From update of attachment 275887 [details] [diff] [review])
> +    if (typeList.IndexOf(type) != -1)
> +      typeList.RemoveElementAt(typeList.IndexOf(type), true);
> 
> I'd store the result of IndexOf in a variable to avoid having to call it twice

Ok, done.


> +      if (typeList.Resource.Value == "urn:mimetypes:root")
> +        handler = this._mimeSvc.getFromTypeAndExtension(type, null);
> +      else
> +        handler = this._protocolSvc.getProtocolHandlerInfo(type);
> +      if (!handler)
> +        continue;
> 
> can this actually be null? Wouldn't an exception be thrown instead?

Hmm, indeed, if that.  I've removed the check.

Carrying forward review and superreview.  Here's the version I'll commit.

Comment 10

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

Shawn thought the patch might be horked by his checkin of bug 391279, but it wasn't.  Patch checked in to the trunk:

/tests/unit/test_handlerService.js 
Checking in uriloader/exthandler/nsHandlerService.js;
/cvsroot/mozilla/uriloader/exthandler/nsHandlerService.js,v  <--  nsHandlerService.js
new revision: 1.9; previous revision: 1.8
done
Checking in uriloader/exthandler/nsIHandlerService.idl;
/cvsroot/mozilla/uriloader/exthandler/nsIHandlerService.idl,v  <--  nsIHandlerService.idl
new revision: 1.5; previous revision: 1.4
done
Checking in uriloader/exthandler/tests/unit/head_handlerService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/head_handlerService.js,v  <--  head_handlerService.js
new revision: 1.5; previous revision: 1.4
done
Checking in uriloader/exthandler/tests/unit/tail_handlerService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/tail_handlerService.js,v  <--  tail_handlerService.js
new revision: 1.4; previous revision: 1.3
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.10; previous revision: 1.9
done