Closed Bug 387930 Opened 15 years ago Closed 15 years ago

API for accessing and modifying content handling datastore

Categories

(Core Graveyard :: File Handling, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: myk, Assigned: myk)

References

Details

Attachments

(1 file, 6 obsolete files)

There should be an API for accessing and modifying the content handling datastore so that code doesn't have to touch the datastore directly.  In the short run, this will enable us to refactor code that now lives in multiple places.  In the long run, it'll enable us to switch from the current RDF-based datastore to a SQLite-based one.
Note: I'm still not sure it wouldn't make more sense to add a "sync" method to nsIHandlerInfo objects, as dmose suggested.  But if we wanted to do that in JS, then I'd have to implement nsIHandlerInfo (and nsIMIMEInfo) in JS, which isn't a terrible idea, just requires some work.
Here's a version of the patch I've checked into the branch so that sdwilsh can get started integrating it into his front-end work.  It fixes a couple bugs, adds a couple comments, and includes the necessary Makefile.in changes.  Tests to follow.

RCS file: /cvsroot/mozilla/uriloader/exthandler/Attic/nsIHamburgerHelperService.idl,v
done
Checking in uriloader/exthandler/nsIHamburgerHelperService.idl;
/cvsroot/mozilla/uriloader/exthandler/Attic/nsIHamburgerHelperService.idl,v  <--  nsIHamburgerHelperService.idl
new revision: 1.1.2.1; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/uriloader/exthandler/Attic/nsHamburgerHelperService.js,v
done
Checking in uriloader/exthandler/nsHamburgerHelperService.js;
/cvsroot/mozilla/uriloader/exthandler/Attic/nsHamburgerHelperService.js,v  <--  nsHamburgerHelperService.js
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in uriloader/exthandler/Makefile.in;
/cvsroot/mozilla/uriloader/exthandler/Makefile.in,v  <--  Makefile.in
new revision: 1.62.6.2; previous revision: 1.62.6.1
done
Attachment #272093 - Attachment is obsolete: true
Here's a followup patch that fixes bugs and adds tests.  I've checked this into the branch as well:

Checking in uriloader/Makefile.in;
/cvsroot/mozilla/uriloader/Makefile.in,v  <--  Makefile.in
new revision: 1.9.66.1; previous revision: 1.9
done
Checking in uriloader/exthandler/nsHamburgerHelperService.js;
/cvsroot/mozilla/uriloader/exthandler/Attic/nsHamburgerHelperService.js,v  <--  nsHamburgerHelperService.js
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
Checking in uriloader/exthandler/nsIHamburgerHelperService.idl;
/cvsroot/mozilla/uriloader/exthandler/Attic/nsIHamburgerHelperService.idl,v  <--  nsIHamburgerHelperService.idl
new revision: 1.1.2.2; previous revision: 1.1.2.1
done
RCS file: /cvsroot/mozilla/uriloader/exthandler/tests/Attic/Makefile.in,v
done
Checking in uriloader/exthandler/tests/Makefile.in;
/cvsroot/mozilla/uriloader/exthandler/tests/Attic/Makefile.in,v  <--  Makefile.in
new revision: 1.1.2.1; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/head_hamburgerHelperService.js,v
done
Checking in uriloader/exthandler/tests/unit/head_hamburgerHelperService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/head_hamburgerHelperService.js,v  <--  head_hamburgerHelperService.js
new revision: 1.1.2.1; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/tail_hamburgerHelperService.js,v
done
Checking in uriloader/exthandler/tests/unit/tail_hamburgerHelperService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/tail_hamburgerHelperService.js,v  <--  tail_hamburgerHelperService.js
new revision: 1.1.2.1; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/test_hamburgerHelperService.js,v
done
Checking in uriloader/exthandler/tests/unit/test_hamburgerHelperService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/test_hamburgerHelperService.js,v  <--  test_hamburgerHelperService.js
new revision: 1.1.2.1; previous revision: 1.1
done
Per discussions with dmose, sdwilsh, biesi, and sayrer on IRC, I've changed the name of the service from the hamburger helper service to the handler service, and I've consolidated the various methods for storing individual properties of handler info objects into a single method |store(in nsIHandlerInfo)| that stores all properties at once (and also retrieves the values to store from the handler info object itself instead of making consumers pass in the values separately).
Attachment #272236 - Attachment is obsolete: true
Attachment #272275 - Attachment is obsolete: true
CVS checkin log from my checkin of the changes in patch v3 to the branch:

Removing uriloader/exthandler/nsHamburgerHelperService.js;
/cvsroot/mozilla/uriloader/exthandler/Attic/nsHamburgerHelperService.js,v  <--  nsHamburgerHelperService.js
new revision: delete; previous revision: 1.1.2.2
done
Removing uriloader/exthandler/nsIHamburgerHelperService.idl;
/cvsroot/mozilla/uriloader/exthandler/Attic/nsIHamburgerHelperService.idl,v  <--  nsIHamburgerHelperService.idl
new revision: delete; previous revision: 1.1.2.2
done
RCS file: /cvsroot/mozilla/uriloader/exthandler/Attic/nsHandlerService.js,v
done
Checking in uriloader/exthandler/nsHandlerService.js;
/cvsroot/mozilla/uriloader/exthandler/Attic/nsHandlerService.js,v  <--  nsHandlerService.js
new revision: 1.1.2.1; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/uriloader/exthandler/Attic/nsIHandlerService.idl,v
done
Checking in uriloader/exthandler/nsIHandlerService.idl;
/cvsroot/mozilla/uriloader/exthandler/Attic/nsIHandlerService.idl,v  <--  nsIHandlerService.idl
new revision: 1.1.2.1; previous revision: 1.1
done
Removing uriloader/exthandler/tests/unit/head_hamburgerHelperService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/head_hamburgerHelperService.js,v  <--  head_hamburgerHelperService.js
new revision: delete; previous revision: 1.1.2.1
done
RCS file: /cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/head_handlerService.js,v
done
Checking in uriloader/exthandler/tests/unit/head_handlerService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/head_handlerService.js,v  <--  head_handlerService.js
new revision: 1.1.2.1; previous revision: 1.1
done
Removing uriloader/exthandler/tests/unit/tail_hamburgerHelperService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/tail_hamburgerHelperService.js,v  <--  tail_hamburgerHelperService.js
new revision: delete; previous revision: 1.1.2.1
done
RCS file: /cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/tail_handlerService.js,v
done
Checking in uriloader/exthandler/tests/unit/tail_handlerService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/tail_handlerService.js,v  <--  tail_handlerService.js
new revision: 1.1.2.1; previous revision: 1.1
done
Removing uriloader/exthandler/tests/unit/test_hamburgerHelperService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/test_hamburgerHelperService.js,v  <--  test_hamburgerHelperService.js
new revision: delete; previous revision: 1.1.2.1
done
RCS file: /cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/test_handlerService.js,v
done
Checking in uriloader/exthandler/tests/unit/test_handlerService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/Attic/test_handlerService.js,v  <--  test_handlerService.js
new revision: 1.1.2.1; previous revision: 1.1
done
Checking in Makefile.in;
/cvsroot/mozilla/uriloader/exthandler/Makefile.in,v  <--  Makefile.in
new revision: 1.62.6.3; previous revision: 1.62.6.2
done
Attachment #272570 - Flags: superreview?(cbiesinger)
Attachment #272570 - Flags: review?(dmose)
Assignee: myk → nobody
Status: ASSIGNED → NEW
Product: Firefox → Core
QA Contact: file.handling → file-handling
Target Milestone: Firefox 3 M7 → mozilla1.9beta1
Assignee: nobody → myk
Blocks: 388389
Blocks: 377784
Blocks: 385065
Comment on attachment 272570 [details] [diff] [review]
wip patch v3: change name of service, consolidate methods

This is great work; it's a huge step forward to having a sane, documented, tested, well-defined API to interact with.  Most/all of my comments are just nits.  r+ with the following addressed:

>Index: uriloader/exthandler/nsHandlerService.js
>
> [...]
>
>+const NC_MIME_TYPES         = NC_NS + "MIME-types";

Not sure if RDF arc names are treated case-sensitively or not, but nsHelperAppRDF.h has a capital T in Types.

>+      case Ci.nsIHandlerInfo.useHelperApp:
>+      // XXX Should we throw instead of assuming the default if we don't
>+      // recognize the value?

Setting the default seems fine, but it'd be good to document that behavior in the doxygen comments for nsIHandlerInfo.store().

>+  _storePreferredHandler: function HS__storePreferredHandler(aHandlerInfo) {
>+    try {
>+      handler.QueryInterface(Ci.nsILocalHandlerApp);
>+      this._setLiteral(handlerID, NC_PATH, handler.executable.path);
>+      this._removeValue(handlerID, NC_URI_TEMPLATE);
>+    }
>+    catch(ex) {
>+      handler.QueryInterface(Ci.nsIWebHandlerApp);
>+      this._setLiteral(handlerID, NC_URI_TEMPLATE, handler.uriTemplate);
>+      this._removeValue(handlerID, NC_PATH);
>+    }

Instead of using QI and try/catch here, I think it'd make the code more reasonable to do something like 

if (handler instanceof Ci.nsILocalHandlerApp)

It's possible that that won't work because the C++ handler app impls don't have classinfo, though, in which case you're welcome to do any of: add the classinfo macros, file a bug to do that later, or simply ignore my comment.  (Given how absurdly simple that your tests have demonstrated JS implementation of these ifaces to be, I'm continuing to regret writing that code in C++; fortunately that'll be easy to remedy).

>+  /**
>+   * Return the unique identifier for a content type record, which stores
>+   * the editable and value fields plus a reference to the type's handler.
>+   * 
>+   * FIXME: the ID should be a property of nsIHandlerInfo.
>+   * 
>+   * |urn:(mimetype|scheme):<type>|

This kinda feels like an implementation detail of RDF.  Which makes me not-so-sure that it wants to be on nsIHandlerInfo.  On the other hand, I suppose it's as good a representation of the (kind, type) tuple as any.

>+  /**
>+   * Return the unique identifier for a preferred handler record, which stores
>+   * information about the preferred handler for a given content type, including
>+   * its human-readable name and the path to its executable (for a local app)
>+   * or its URI template (for a web app).
>+   * 
>+   * |urn:(mimetype|scheme):externalApplication:<type>|
>+   *
>+   * FIXME: this should be a property of nsIHandlerApp.
>+
>+   * FIXME: this should be an arbitrary ID, and we should retrieve it from
>+   * the datastore for a given content type via the NC:ExternalApplication
>+   * property rather than looking for a specific ID, so a handler doesn't
>+   * have to change IDs when it goes from being a possible handler to being
>+   * the preferred one.
>+   * 
>+   * @param aHandlerInfo {nsIHandlerInfo} the type for which to get the ID
>+   */

I think the above two FIXMEs probably want to happen at the same time.  I.e. we probably don't want to expose this on nsIHandlerApp as long it's merely an RDF implementation detail and not used by any other code.

>+  _getTypeList: function HS__getTypeList(aHandlerInfo) {

Having a function named get do a bunch of implicit creation seems likely to confuse folks reading the calling code.  Maybe change this to be _ensureTypeList or _ensureAndGetTypeList?

>+  // FIXME: this stuff should all be in a JavaScript module given that I keep
>+  // copying it from component to component.

Alternately, some of this stuff might want to live in or be replaced by stuff from FUEL.  You might add that to the FIXME.

>+  //**************************************************************************//
>+    // This causes extraneous errors to show up in the log when the directory
>+    // service asks us first for CurProcD and MozBinD.  I wish there was a way
>+    // to suppress those errors.

I presume that actually providing those values would be a bad idea?

>+  //**************************************************************************//
>+  // Utilities
>+
>+  /**
>+   * Get the datasource file, registering ourselves as a provider
>+   * of that directory if necessary.
>+   */

Why register dynamically here, rather than in the constructor?

>Index: uriloader/exthandler/tests/unit/tail_handlerService.js

I take it a tail file is required, even if it's empty?

>+  // It doesn't matter whether or not this executable exists or is executable,
>+  // only that it'll QI to nsIFile and has a path attribute, which the service
>+  // expects.
>+  var executable = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
>+  executable.initWithPath("/usr/bin/test");

If I'm reading nsLocalFileWin.cpp correctly, the above statement is going to throw on windows since it's not a windows-style path.

>+  var preferredHandler = handlerInfo.preferredApplicationHandler;
>+  do_check_eq(typeof preferredHandler, "object");
> [...]
>+  var localHandler = preferredHandler.QueryInterface(Ci.nsILocalHandlerApp);
>+  do_check_eq(localHandler.executable.path, "/usr/bin/test");

I bet these checks want to use |instanceof Ci.nsILocalHandlerApp| too, modulo the same issues mentioned in my previous comment.
Attachment #272570 - Flags: review?(dmose) → review+
instanceof doesn't require classinfo; it just calls QueryInterface
Attached patch patch v4: fixes review comments (obsolete) — Splinter Review
(In reply to comment #7)
> (From update of attachment 272570 [details] [diff] [review])
> This is great work; it's a huge step forward to having a sane, documented,
> tested, well-defined API to interact with.  Most/all of my comments are just
> nits.  r+ with the following addressed:
> 
> >Index: uriloader/exthandler/nsHandlerService.js
> >
> > [...]
> >
> >+const NC_MIME_TYPES         = NC_NS + "MIME-types";
> 
> Not sure if RDF arc names are treated case-sensitively or not, but
> nsHelperAppRDF.h has a capital T in Types.

I think they are case-sensitive, but the constants in nsHelperAppRDF.h aren't actually used anywhere, and the default mimeTypes.rdf files <http://lxr.mozilla.org/mozilla/find?luckytricky=1&string=mimetypes.rdf> all use the lower-case "types".


> Setting the default seems fine, but it'd be good to document that behavior in
> the doxygen comments for nsIHandlerInfo.store().

Good point, I've added some documentation about that.


> >+  _storePreferredHandler: function HS__storePreferredHandler(aHandlerInfo) {
> >+    try {
> >+      handler.QueryInterface(Ci.nsILocalHandlerApp);
> >+      this._setLiteral(handlerID, NC_PATH, handler.executable.path);
> >+      this._removeValue(handlerID, NC_URI_TEMPLATE);
> >+    }
> >+    catch(ex) {
> >+      handler.QueryInterface(Ci.nsIWebHandlerApp);
> >+      this._setLiteral(handlerID, NC_URI_TEMPLATE, handler.uriTemplate);
> >+      this._removeValue(handlerID, NC_PATH);
> >+    }
> 
> Instead of using QI and try/catch here, I think it'd make the code more
> reasonable to do something like 
> 
> if (handler instanceof Ci.nsILocalHandlerApp)

Yeah, that seems much better.  Ok, doing it this way now.


> >+   * FIXME: the ID should be a property of nsIHandlerInfo.
> 
> This kinda feels like an implementation detail of RDF.  Which makes me
> not-so-sure that it wants to be on nsIHandlerInfo.  On the other hand, I
> suppose it's as good a representation of the (kind, type) tuple as any.

Yeah, you may be right.  I've changed the comment from a FIXME statement to an XXX question.


> >+   * FIXME: this should be a property of nsIHandlerApp.
> >+
> >+   * FIXME: this should be an arbitrary ID, and we should retrieve it from
> >+   * the datastore for a given content type via the NC:ExternalApplication
> >+   * property rather than looking for a specific ID, so a handler doesn't
> >+   * have to change IDs when it goes from being a possible handler to being
> >+   * the preferred one.
> 
> I think the above two FIXMEs probably want to happen at the same time.  I.e. we
> probably don't want to expose this on nsIHandlerApp as long it's merely an RDF
> implementation detail and not used by any other code.

Indeed.  And perhaps the first FIXME is in the same boat as the previous FIXME.  I've turned it into a question as well.


> >+  _getTypeList: function HS__getTypeList(aHandlerInfo) {
> 
> Having a function named get do a bunch of implicit creation seems likely to
> confuse folks reading the calling code.  Maybe change this to be
> _ensureTypeList or _ensureAndGetTypeList?

Good point.  I've changed it to _ensureAndGetTypeList.


> >+  // FIXME: this stuff should all be in a JavaScript module given that I keep
> >+  // copying it from component to component.
> 
> Alternately, some of this stuff might want to live in or be replaced by stuff
> from FUEL.  You might add that to the FIXME.

Yup, added.


> >+  //**************************************************************************//
> >+    // This causes extraneous errors to show up in the log when the directory
> >+    // service asks us first for CurProcD and MozBinD.  I wish there was a way
> >+    // to suppress those errors.
> 
> I presume that actually providing those values would be a bad idea?

I'm not sure.  On principle, it seems beneficial to override the fewest possible locations provided by existing directory providers.  And that's how the Places tests do it, which is where I got the code for this.  So it seems like the right approach, despite the extraneous errors.


> >+  //**************************************************************************//
> >+  // Utilities
> >+
> >+  /**
> >+   * Get the datasource file, registering ourselves as a provider
> >+   * of that directory if necessary.
> >+   */
> 
> Why register dynamically here, rather than in the constructor?

This is how the Places tests do it, and I think the idea here is that we only take on the role of providing a location for the file if there isn't an existing provider for it, so if at some point xpcshell includes such a provider, or if the tests are run in a different context where there's already a provider, then we don't override it.


> >Index: uriloader/exthandler/tests/unit/tail_handlerService.js
> 
> I take it a tail file is required, even if it's empty?

Hmm, no, it's actually not, so removing it.


> >+  // It doesn't matter whether or not this executable exists or is executable,
> >+  // only that it'll QI to nsIFile and has a path attribute, which the service
> >+  // expects.
> >+  var executable = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
> >+  executable.initWithPath("/usr/bin/test");
> 
> If I'm reading nsLocalFileWin.cpp correctly, the above statement is going to
> throw on windows since it's not a windows-style path.

Yup.  Per discussion with bsmedberg on IRC, I'm now using the directory service to get a platform-specific temporary directory.  It turns out that the executable has to exist in order for it to be a preferredApplicationHandler, i.e. I can write it to mimeTypes.rdf, but if I call nsIMIMEService::getFromTypeAndExtension, and the executable path doesn't point to an existing file, then nsIHandlerInfo::preferredApplicationHandler will be null on the returned nsIHandlerInfo.

This seems suboptimal, because you can't tell the difference between "has a configured preferred handler, but that doesn't seem to be around anymore" and "doesn't have a configured preferred handler".  I'll file a bug on that.


> >+  var preferredHandler = handlerInfo.preferredApplicationHandler;
> >+  do_check_eq(typeof preferredHandler, "object");
> > [...]
> >+  var localHandler = preferredHandler.QueryInterface(Ci.nsILocalHandlerApp);
> >+  do_check_eq(localHandler.executable.path, "/usr/bin/test");
> 
> I bet these checks want to use |instanceof Ci.nsILocalHandlerApp| too, modulo
> the same issues mentioned in my previous comment.

Yup, I've added a check using instanceof.

I also added a method to the HandlerServiceTest object, _getDatasourceContents, which serializes and returns the mimeTypes.rdf datasource.  This is useful when trying to figure why a test isn't working, although it isn't used normally.

Carrying forward dmose's review.
Attachment #273055 - Flags: superreview?(cbiesinger)
Attachment #273055 - Flags: review+
Attachment #272570 - Attachment is obsolete: true
Attachment #272570 - Flags: superreview?(cbiesinger)
Over in bug 388388, biesi asked me to incorporate the nsHandlerService.js changes I made there into this patch.

I can't completely do that, since the changes rely on other changes in that patch, but here's a version of this patch that includes all the changes it was possible to integrate in advance.

I'll make the rest of the changes in a new patch for bug 388388.  To make it easy, I've labeled them in this patch.  They're quite simple, actually, just one occurrence of this string:

        // FIXME: remove this extra condition in the fix for bug 388388.
        && aHandlerInfo.QueryInterface(Ci.nsIMIMEInfo).MIMEType)

And four occurrences of this one (or the like):

        // FIXME: change this to aHandlerInfo.type in the fix for bug 388388.
        aHandlerInfo.QueryInterface(Ci.nsIMIMEInfo).MIMEType;

Since I'm making changes to this patch over and above the changes for which dmose's review carries forward, I'm re-requesting review from him.
Attachment #273055 - Attachment is obsolete: true
Attachment #273103 - Flags: superreview?(cbiesinger)
Attachment #273103 - Flags: review?(dmose)
Attachment #273055 - Flags: superreview?(cbiesinger)
Comment on attachment 273103 [details] [diff] [review]
patch v5: incorporates changes to nsHandlerService from patch in bug 388388

Why not add the tests directory from exthandler/Makefile.in instead of from 
uriloader/Makefile.in?

uriloader/exthandler/nsHandlerService.js

Shouldn't you also store the description property of the handler info?

What's NC_HANDLER_INFO? We don't currently seem to have any RDF arcs with
handlerProp in the name... 

+    if (handler instanceof Ci.nsILocalHandlerApp) {
+      handler.QueryInterface(Ci.nsILocalHandlerApp);

You don't need both instanceof and QueryInterface, one of them is enough

+    if (aHandlerInfo instanceof Ci.nsIMIMEInfo
+        // FIXME: remove this extra condition in the fix for bug 388388. 
+        && aHandlerInfo.QueryInterface(Ci.nsIMIMEInfo).MIMEType)

Hm, I usually see the operator at the end of the first line instead of at
the beginning of the second line

+   * property rather than looking for a specific ID, so a handler doesn't 
+   * have to change IDs when it goes from being a possible handler to
being
+   * the preferred one.

We don't have possible handlers on trunk...

uriloader/exthandler/nsIHandlerService.idl
+   * FIXME: also store any changes to the list of possible handlers.

That list doesn't exist on trunk...

+   * @param aHandlerInfo  {nsIHandlerInfo}  the handler info object

Why mention the type of the parameter here again? Ah, you did that for the 
JS comments too... not needed in IDL though :)

What is Firefox-specific about the tests? Shouldn't they run even when
MOZ_PHOENIX isn't defined?
Attachment #273103 - Flags: superreview?(cbiesinger) → superreview+
(In reply to comment #9)
> I think they are case-sensitive, but the constants in nsHelperAppRDF.h aren't
> actually used anywhere, and the default mimeTypes.rdf files
> <http://lxr.mozilla.org/mozilla/find?luckytricky=1&string=mimetypes.rdf> all
> use the lower-case "types".

The constants are used here:
http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperAppService.cpp#555

> The constants are used here:
> http://lxr.mozilla.org/seamonkey/source/uriloader/exthandler/nsExternalHelperAppService.cpp#555

A bunch of the constants in nsHelperAppRDF.h are used there, but not the one dmose was referring to, NC_MIME_TYPES.  That one isn't used anywhere, nor is NC_MIME_TYPE (the other constant in that file which capitalizes "type").
Comment on attachment 273103 [details] [diff] [review]
patch v5: incorporates changes to nsHandlerService from patch in bug 388388

Looks good; r=dmose.
Attachment #273103 - Flags: review?(dmose) → review+
(In reply to comment #11)

> Shouldn't you also store the description property of the handler info?

Hmm, perhaps.  It's not entirely clear, since the description isn't editable in either the Download Actions or the UCTH dialog, and for the latter sdwilsh thinks the current functionality is all he needs.

But perhaps selecting a non-default action provided by the OS in the UCTH dialog should write the description to the RDF file as well.  I'll file another bug on that if it turns out to be the case.

Incidentally, the goal here is to have a base upon which we can build.  I suspect that we'll discover that we want to write a bunch more stuff to the datastore over time (f.e. a list of possible handlers).  Once we have this code in, it'll be easier to add such things.


> What's NC_HANDLER_INFO? We don't currently seem to have any RDF arcs with
> handlerProp in the name... 

NC:handlerProp is the property for arcs between a MIME type resource and its handler resource, f.e.:

  <RDF:Description RDF:about="urn:mimetype:application/x-tar" ...>
    <NC:handlerProp RDF:resource="urn:mimetype:handler:application/x-tar"/>
  </RDF:Description>

It's described in the default mimeTypes.rdf file <http://mxr.mozilla.org/mozilla/source/profile/defaults/mimeTypes.rdf#23> and used in downloadactions.js, overrideHandler.js, and helperApps.js.


> +    if (handler instanceof Ci.nsILocalHandlerApp) {
> +      handler.QueryInterface(Ci.nsILocalHandlerApp);
> 
> You don't need both instanceof and QueryInterface, one of them is enough

Ok, I'll remove the extraneous QueryInterface call before checking in the patch.


> +    if (aHandlerInfo instanceof Ci.nsIMIMEInfo
> +        // FIXME: remove this extra condition in the fix for bug 388388. 
> +        && aHandlerInfo.QueryInterface(Ci.nsIMIMEInfo).MIMEType)
> 
> Hm, I usually see the operator at the end of the first line instead of at
> the beginning of the second line

Hmm, indeed, I'll move it there.


> +   * property rather than looking for a specific ID, so a handler doesn't 
> +   * have to change IDs when it goes from being a possible handler to
> being
> +   * the preferred one.
> 
> We don't have possible handlers on trunk...

Right, but we will soon!  I'll update the comment to reflect the future nature of the possible handlers functionality.


> uriloader/exthandler/nsIHandlerService.idl
> +   * FIXME: also store any changes to the list of possible handlers.
> 
> That list doesn't exist on trunk...

I'll make this comment futuristic too.


> +   * @param aHandlerInfo  {nsIHandlerInfo}  the handler info object
> 
> Why mention the type of the parameter here again? Ah, you did that for the 
> JS comments too... not needed in IDL though :)

Hmm, indeed.  This is the format my IDE suggests, but I'll remove the duplicative comment from the IDL file.


> What is Firefox-specific about the tests? Shouldn't they run even when
> MOZ_PHOENIX isn't defined?

As far as I know, there's nothing Firefox-specific about the tests, so yes, they should indeed run even when MOZ_PHOENIX is not defined.  I'll remove that ifdef.
Here's the version I'm going to check in.  It's just like v5 except that it fixes the nits biesi raised in comment 11, including this one that I forgot to respond to in my responses in comment 12:

> Why not add the tests directory from exthandler/Makefile.in instead of from 
> uriloader/Makefile.in?

Good idea, I've done so in this patch.
Attachment #273103 - Attachment is obsolete: true
(In reply to comment #13)

> A bunch of the constants in nsHelperAppRDF.h are used there, but not the one
> dmose was referring to, NC_MIME_TYPES.  That one isn't used anywhere, nor is
> NC_MIME_TYPE (the other constant in that file which capitalizes "type").

Err, to be precise I should say that the following two constants from nsHelperAppRDF.h, which capitalize the word "type", are not used in any code:

 47 #define NC_RDF_MIMETYPES        NC_NAMESPACE_URI"MIME-Types"
 48 // a mime type has the following properties...
 49 #define NC_RDF_MIMETYPE         NC_NAMESPACE_URI"MIME-Type"

Here's a search that returns the #defines as the only place where these constants appear in our codebase:

http://mxr.mozilla.org/mozilla/search?string=NC_RDF_MIMETYPE

Nor does any code define that constant separately, as far as I can tell.  For example, overrideHandler.js, helperApps.js, and downloadActions.js all assume the urn:mimetypes:root resource exists and access it directly rather than through the NC:MIME-types property of the urn:mimetypes resource.

But in the handler service I decided not to make that assumption (helped by the fact that it actually doesn't exist during testing), so the handler service checks for the presence of the |urn:mimetypes -> NC:MIME-types -> urn:mimetypes:root| arc and recreates it if it doesn't exist.
Patch v6 checked into trunk:

Checking in Makefile.in;
/cvsroot/mozilla/uriloader/exthandler/Makefile.in,v  <--  Makefile.in
new revision: 1.66; previous revision: 1.65
done
Checking in nsHandlerService.js;
/cvsroot/mozilla/uriloader/exthandler/nsHandlerService.js,v  <--  nsHandlerService.js
new revision: 1.4; previous revision: 1.3
done
Checking in nsIHandlerService.idl;
/cvsroot/mozilla/uriloader/exthandler/nsIHandlerService.idl,v  <--  nsIHandlerService.idl
new revision: 1.4; previous revision: 1.3
done
Checking in tests/Makefile.in;
/cvsroot/mozilla/uriloader/exthandler/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.4; previous revision: 1.3
done
Checking in tests/unit/head_handlerService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/head_handlerService.js,v  <--  head_handlerService.js
new revision: 1.4; previous revision: 1.3
done
Checking in tests/unit/test_handlerService.js;
/cvsroot/mozilla/uriloader/exthandler/tests/unit/test_handlerService.js,v  <--  test_handlerService.js
new revision: 1.4; previous revision: 1.3
done
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
fwiw, re storing description, this code is not only used by firefox, and in seamonkey you can edit the description in the "helper applications" preferences.
er, I meant... this code is not firefox-specific, even though it may only be used by ffox initially.
Duplicate of this bug: 190413
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.