From the current investigation, we need to rewrite "Retrieval Methods", "Storage Method" and "DataStore Utils". We also need a preference to decide whether to use mimeType.rdf or mimeType.json.

Set priority to P2 because we're going to implement this feature on Firefox 51. @ Development Schedule: https://docs.google.com/spreadsheets/d/1WznJUt7lLvcZwwry0yvJ_-hw0ZAXTxOOvmYCw74uGKY/edit#gid=0

In this bug, we will add a new implementation of nsIHandlerService for JSON backend(nsHandlerService-json.js). The import function (mimeType.rdf to mimeType.json) is also covered in this bug.

Hi Paolo, In this patch, I write a new implementation of nsHanderService. I've tried the APIs of nsHandlerService with mimeTypes.json(Bug 1287658 attachment 8802855 [details]). Besides,I think we should integrate the import function into this patch. What do you think?

Comment on attachment 8811634 [details] [diff] [review] [1117-WIP] nsHandlerService-json implementation (In reply to Alphan Chen [:alchen] from comment #3) > In this patch, I write a new implementation of nsHanderService. > I've tried the APIs of nsHandlerService with mimeTypes.json(Bug 1287658 > attachment 8802855 [details]). Thanks, this is the approach that we discussed, and it looks good to me. I'm fine with implementing this under a different contractID for now. When we actually migrate to the new back-end, we can use the old contractID for the new implementation. We need xpcshell tests for each method and code path that we support. They need to save the data first, then reload it from the file and verify that we saved what we expected. The "passwordmgr" tests have examples for testing persistence by reloading the data. The tests should all run two times, one for the old contractID and one for the new contractID. The "jsdownloads" tests have an example of how to run a set of xpcshell tests two times with a different flag, and there are other examples in the tree. You can remove duplicate code and improve readability if you create functions that convert the XPCOM objects to the JS objects and the other way around, then just use them inside loops, "Array.map" statements, or Array comprehensions. In this new implementation file, we should upgrade legacy code that uses techniques that are now obsoleted. One example is that we can now use "Services.jsm" or lazy service getters with "XPCOMUtils.jsm", so we can get rid of most of the old boilerplate. We can also use newer syntax, for example ES6 method definitions instead of named functions. In future versions you post for feedback or review, please remove all the code that is commented out, and all debugging like the "dump" statements. Please use MozReview, as it makes it easier to test locally and update to new versions. > Besides,I think we should integrate the import function into this patch. If you are talking about the data migration, it's logically separate from the data access itself, so I think it's better to only implement the data access in this bug, and implement the migration afterwards. If you are talking about the initialization of default handlers, we can just keep it around in the new implementation of nsIHandlerService.

Review commit: https://reviewboard.mozilla.org/r/97134/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/97134/

(In reply to Alphan Chen [:alchen] from comment #6) > Created attachment 8816388 [details] > Bug 1287660 - New implementation of nsIHandlerService for the JSON backend; > > Review commit: https://reviewboard.mozilla.org/r/97134/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/97134/ Please ignore this review. I send the review request again when trying to configure Git to use MozReview. Currently I don't find a way to cancel it.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. Don't worry for the duplicate review request! I look forward to seeing the new version as we discussed. I'm thinking that we may be able to split this big patch in multiple parts. The first can just add the "-json" variant of the service and add the new test that works directly on the two versions of the service. You can leave the modification of the existing tests that call nsIMIMEService.getFromTypeAndExtension to a second patch, so you don't need to deal with nsIComponentRegistrar in the first part. Actually, we may want to just keep these tests as they are, so they work on the active version of the service. We can tweak them when we switch implementations in a later bug. This way, you don't need to write code to change the active service with nsIComponentRegistrar at all.

(In reply to :Paolo Amadini from comment #8) > Comment on attachment 8816011 [details] > > You can leave the modification of the existing tests that call > nsIMIMEService.getFromTypeAndExtension to a second patch, so you don't need > to deal with nsIComponentRegistrar in the first part. > > Actually, we may want to just keep these tests as they are, so they work on > the active version of the service. We can tweak them when we switch > implementations in a later bug. This way, you don't need to write code to > change the active service with nsIComponentRegistrar at all. OK, I prefer to open a new bug for the second patch. Before doing second patch, we could finish migration (bug 1287658) first.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review102134 Thanks for doing this patch which only implements the new service. Because the existing service on which this patch is based is very old, we still need to update the code to match the modern Firefox standards, and simplify it. For testing, we'll need to ensure that both the old and new service behave in the same way, by running the same new tests on both of them. Before writing about the specifics in the review, here are some general notes that apply to the entire patch. - Keep useful code comments, but remove redundant comments, even if they existed in the old file. - Avoid for..in loops, use for..of for arrays, together with Object.keys() for objects. - Use "let" instead of "var", also in the declarations of "for" loops. - Don't define variables that are used only once, unless it improves readability. - Add braces around single-line "if" clauses. - Don't use quotes around property names in object literals, if they don't contain special characters. ::: docshell/build/nsDocShellModule.cpp:195 (Diff revision 2) > { NS_ABOUT_MODULE_CONTRACTID_PREFIX "telemetry", &kNS_ABOUT_REDIRECTOR_MODULE_CID }, > { NS_ABOUT_MODULE_CONTRACTID_PREFIX "webrtc", &kNS_ABOUT_REDIRECTOR_MODULE_CID }, > { NS_URI_LOADER_CONTRACTID, &kNS_URI_LOADER_CID }, > { NS_DOCUMENTLOADER_SERVICE_CONTRACTID, &kNS_DOCUMENTLOADER_SERVICE_CID }, > { NS_HANDLERSERVICE_CONTRACTID, &kNS_CONTENTHANDLERSERVICE_CID, mozilla::Module::CONTENT_PROCESS_ONLY }, > + { NS_HANDLERSERVICEJSON_CONTRACTID, &kNS_CONTENTHANDLERSERVICE_CID, mozilla::Module::CONTENT_PROCESS_ONLY }, This entry registers the C++ implementation of nsIHandlerService for the content process, which simply forwards all the calls to the parent process. We don't need to register a JSON variant here. ::: uriloader/exthandler/nsCExternalHandlerService.idl:27 (Diff revision 2) > +#define NS_HANDLERSERVICEJSON_CONTRACTID \ > +"@mozilla.org/uriloader/handler-service-json;1" > + Consequently, this is also unneeded. ::: uriloader/exthandler/nsHandlerService-json.js:5 (Diff revision 2) > +const Ci = Components.interfaces; > +const Cc = Components.classes; > +const Cu = Components.utils; > +const Cr = Components.results; The new convention for this is: const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components; ::: uriloader/exthandler/nsHandlerService-json.js:10 (Diff revision 2) > +Cu.import("resource://gre/modules/osfile.jsm"); > +Cu.import('resource://gre/modules/XPCOMUtils.jsm'); > +Cu.import("resource://gre/modules/Services.jsm"); > +XPCOMUtils.defineLazyModuleGetter(this, "JSONFile", > + "resource://gre/modules/JSONFile.jsm"); > + > +// Observer Service > +XPCOMUtils.defineLazyServiceGetter(this, "observerSvc", > + "@mozilla.org/observer-service;1", > + "nsIObserverService"); > +// MIME Service > +XPCOMUtils.defineLazyServiceGetter(this, "mimeSvc", > + "@mozilla.org/mime;1", > + "nsIMIMEService"); > +// Protocol Service > +XPCOMUtils.defineLazyServiceGetter(this, "protoSvc", > + "@mozilla.org/uriloader/external-protocol-service;1", > + "nsIExternalProtocolService"); You can use "Services.obs" to get the observer service, you don't need a specific lazy getter. The names for the global variables of the other services should look like gMIMEService and gExternalProtocolService. The redundant comments should be removed. nit: leave an empty line between different categories of getters, and no empty lines between the same type of getter. ::: uriloader/exthandler/nsHandlerService-json.js:33 (Diff revision 2) > +const HandlerServiceFactory = { > + _instance: null, > + createInstance: function (outer, iid) { > + if (this._instance) > + return this._instance; > + > + let processType = Cc["@mozilla.org/xre/runtime;1"]. > + getService(Ci.nsIXULRuntime).processType; > + if (processType != Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) > + return Cr.NS_ERROR_NOT_IMPLEMENTED; > + > + return (this._instance = new HandlerService()); > + } > +}; We already register the component only for the parent process using the manifest, and all callers properly use getService instead of createInstance, so this code and the _xpcom_factory reference are unnecessary. ::: uriloader/exthandler/nsHandlerService-json.js:49 (Diff revision 2) > + //**************************************************************************// > + // XPCOM Plumbing > + > + classID: Components.ID("{220cc253-b60f-41f6-b9cf-fdcb325f970f}"), > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIHandlerService]), Remove the "***" headers in the entire file, they are unneeded. Thanks for keeping the original spacing, it's been useful for comparing the files. Now that I've looked at the differences we can just align to the new conventions. So, just one space after the colons here. ::: uriloader/exthandler/nsHandlerService-json.js:60 (Diff revision 2) > + // Observe profile-before-change so we can switch to the datasource > + // in the new profile when the user changes profiles. > + observerSvc.addObserver(this, "profile-before-change", false); > + > + // Observe xpcom-shutdown so we can remove these observers > + // when the application shuts down. > + observerSvc.addObserver(this, "xpcom-shutdown", false); > + > + // Observe profile-do-change so that non-default profiles get upgraded too > + observerSvc.addObserver(this, "profile-do-change", false); All this code was probably written in ancient times, when we could ideally switch profiles at runtime in the browser. We cannot do this anymore, so all these observers and all the related functions in this file are unneeded. I thought they might be used by regression tests, but after a quick search I don't see any test that use these notifications. We still need to force data reload in our new tests, but we can implement this with a specific observer notification that resets the JSONFile implementation, similarly to what LoginTestUtils.reloadData does. ::: uriloader/exthandler/nsHandlerService-json.js:71 (Diff revision 2) > + let jsonPath = OS.Path.join(OS.Constants.Path.profileDir, > + "mimeTypes.json"); > + > + // LOAD DATA > + this._store = new JSONFile({ > + path: jsonPath, > + dataPostProcessor: this._dataPostProcessor.bind(this), > + }); jsonPath is used once, just inline the call. Also, align the arguments to "join" so they start on the same column. ::: uriloader/exthandler/nsHandlerService-json.js:79 (Diff revision 2) > + // LOAD DATA > + this._store = new JSONFile({ > + path: jsonPath, > + dataPostProcessor: this._dataPostProcessor.bind(this), > + }); > + this._store.load(); As we determined previously, we shouldn't call the asynchronous version of the data loading. You can, hovever, call ensureDataReady here, and remove all the other calls throughout the file. This is because every time we start this service, we have to check immediately whether we need to upgrade the list of default handlers, so we need to read the current version from the JSON file. ::: uriloader/exthandler/nsHandlerService-json.js:80 (Diff revision 2) > + this._store = new JSONFile({ > + path: jsonPath, > + dataPostProcessor: this._dataPostProcessor.bind(this), > + }); > + this._store.load(); > + this._store.saveSoon(); We shouldn't call saveSoon every time we load the data. However, we should do that after we imported the default data. ::: uriloader/exthandler/nsHandlerService-json.js:82 (Diff revision 2) > + // Update DB > + // do any necessary updating of the datastore > + this._updateDB(); This is another example of comments that need to be removed. ::: uriloader/exthandler/nsHandlerService-json.js:87 (Diff revision 2) > + _dataPostProcessor(data) { > + /* we can do import here if there is no mimeTypes.json */ > + if (!data || !data.mimetypes || !data.schemes) { > + data = {"version":{},"mimetypes":{},"schemes":{}}; > + // TODO : Bug 1287658 Do migration from mimeTypes.rdf > + } > + return data; > + }, You can use simpler logic: return data || { version: {}, mimetypes: {}, schemes: {} }; We must assume that nobody will alter the JSON file, because there is always a way to break the code by doing that, even if we did a more complicated check for initialization. Also, we will not do the import from RDF here - any failure in the dataPostProcessor will cause the JSON file to be unavailable for reading and for writing. So, you can remove the comments. ::: uriloader/exthandler/nsHandlerService-json.js:100 (Diff revision 2) > + if (!(this._currentLocale in this._store.data.version)) { > + this._store.data.version[this._currentLocale] = -1; > + } > + var defaultHandlersVersion = this._store.data.version[this._currentLocale]; There's likely a better way of doing this without storing -1 in the data store. ::: uriloader/exthandler/nsHandlerService-json.js:106 (Diff revision 2) > + // set the new version first so that if we recurse we don't > + // call _injectNewDefaults several times > + this._store.data.version[this._currentLocale] = this._prefsDefaultHandlersVersion; > + this._injectNewDefaults(); This is from the original code, but can the constructor actually recurse? I see a comment later in this same file that mentions that some code had to be implemented because XPCOM had issues with re-entering the constructor. So, I suspect the case described here doesn't exist, although it's better to verify. So, you can simply set the version only after a successful default data import. ::: uriloader/exthandler/nsHandlerService-json.js:111 (Diff revision 2) > + } catch (ex) { > + // if injecting the defaults failed, set the version back to the > + // previous value > + this._store.data.version[this._currentLocale] = defaultHandlersVersion; > + } This looks conceptually wrong, because there are cases where we could enter this code with defaultHandlersVersion unset. This would likely be caught by ESLint when using "let" instead of "var". ::: uriloader/exthandler/nsHandlerService-json.js:119 (Diff revision 2) > + // get handler service pref branch > + var handlerSvcBranch = Services.prefs.getBranch("gecko.handlerService."); > + > + // get the version of the preferences for this locale > + return Number(handlerSvcBranch. > + getComplexValue("defaultHandlersVersion", > + Ci.nsIPrefLocalizedString).data); You can inline the variable. Maybe you can add a JSDoc-style comment on the getter that reminds that this value depends on the locale, instead of the comments inside the function. ::: uriloader/exthandler/nsHandlerService-json.js:128 (Diff revision 2) > + get _currentLocale() { > + var chromeRegistry = Cc["@mozilla.org/chrome/chrome-registry;1"]. > + getService(Ci.nsIXULChromeRegistry); > + var currentLocale = chromeRegistry.getSelectedLocale("global"); > + return currentLocale; > + }, This is another example from existing code that can be simplified. At first it seems you may just need to inline currentLocale, but actually this getter is now only used inside _updateDB. So, you can remove this getter completely and define currentLocale at the beginning of _updateDB. ::: uriloader/exthandler/nsHandlerService-json.js:217 (Diff revision 2) > + let handler = enumerator.getNext(); > + handler.QueryInterface(Ci.nsIHandlerApp); Simplifiable to enumerator.getNext().QueryInterface(...), because QueryInterface returns the object. ::: uriloader/exthandler/nsHandlerService-json.js:263 (Diff revision 2) > + if ((type in this._store.data.mimetypes)) { > + overrideObj = this._store.data.mimetypes[type]; > + } else if ((aHandlerInfo.type in this._store.data.schemes)) { > + overrideObj = this._store.data.schemes[type]; This isn't right, you're basing the type of handler on the existence in one of the arrays. The original code correctly used an argument type check like "aHandlerInfo instanceof Ci.nsIMIMEInfo". ::: uriloader/exthandler/nsHandlerService-json.js:268 (Diff revision 2) > + if ((type in this._store.data.mimetypes)) { > + overrideObj = this._store.data.mimetypes[type]; > + } else if ((aHandlerInfo.type in this._store.data.schemes)) { > + overrideObj = this._store.data.schemes[type]; > + } else { > + return Cr.NS_ERROR_NOT_AVAILABLE; This should be a "throw", not a "return". Ensure there is a regression test that checks this failure case properly, using "Assert.throws". You can update this to use "new Components.Exception" for better information and stack traces. ::: uriloader/exthandler/nsHandlerService-json.js:274 (Diff revision 2) > + aHandlerInfo.preferredApplicationHandler = > + this.fromSerializable(overrideObj.preferredHandler); > + if (aHandlerInfo.preferredApplicationHandler != null) > + aHandlerInfo.possibleApplicationHandlers.appendElement(aHandlerInfo.preferredApplicationHandler, false); You can likely simplify this using a variable. ::: uriloader/exthandler/nsHandlerService-json.js:280 (Diff revision 2) > + var possibleHandler = this.fromSerializable(handler); > + aHandlerInfo.possibleApplicationHandlers.appendElement(possibleHandler, false); This should handle the case where fromSerializable returns null. ::: uriloader/exthandler/nsHandlerService-json.js:287 (Diff revision 2) > + if (overrideObj.askBeforeHandle != null) { > + alwaysAsk = overrideObj.askBeforeHandle; This would need to be called askBeforeHandling, we can refine the exact names later though. In cases like this, while "!= null" is correct, I'd rather use an alternative like the "in" operator, because it's not obvious that "false != null". However, one thing to note is that if we always store this value in the JSON file, then this check is unneeded. This alternate path likely existed to allow using RDF files created by older versions of Firefox. ::: uriloader/exthandler/nsHandlerService-json.js:318 (Diff revision 2) > + /** > + * @param aHandler > + * A nsIHandlerApp handler app > + * @returns Serializable representation of a handler app object. > + */ > + toSerializable(aHandler) { These need to be called handlerAppToSerializable and handlerAppFromSerializable, because they are methods of a service, and not of a specific type of object. ::: uriloader/exthandler/nsHandlerService-json.js:320 (Diff revision 2) > + var HandlerType = { > + LocalHandlerApp: 0, > + WebHandlerApp: 1, > + DBusHandlerApp: 2, > + }; In general, constants should be defined once, while here they are defined two times in each of the functions. However, I think that instead of using an explicit "type" property, you can infer the type in the serializable representation based on the existence of the "path", "uriTemplate", or "service" properties. ::: uriloader/exthandler/nsHandlerService-json.js:330 (Diff revision 2) > + > + var serializable; > + if (aHandler instanceof Ci.nsILocalHandlerApp) { > + serializable = { > + "name": aHandler.name, > + "description": aHandler.detailedDescription, We don't store the detailedDescription property in the data store currently, and it seems unneeded. In fact, we don't read it again. ::: uriloader/exthandler/nsHandlerService-json.js:387 (Diff revision 2) > + handlerApp = Cc["@mozilla.org/uriloader/dbus-handler-app;1"]. > + createInstance(Ci.nsIDBusHandlerApp); > + handlerApp.service = aHandlerObj.service; > + handlerApp.method = aHandlerObj.method; > + handlerApp.objectPath = aHandlerObj.objpath; > + handlerApp.dBusInterface = aHandlerObj.iface; These names don't match what we save in the JSON file, so this code won't work. We need a regression test that saves and restores each type of handler application. ::: uriloader/exthandler/nsHandlerService-json.js:406 (Diff revision 2) > + var handlerObj = { > + "description": description, > + "action": action, > + "askBeforeHandle": askBeforeHandle, > + "fileExtensions": [], > + "preferredHandler": {}, > + "possibleHandlers": [], > + }; In general, I'd prefer if we only stored properties that are different from the default value, but we can also do this after solving the other issues with the patch. ::: uriloader/exthandler/nsHandlerService-json.js:420 (Diff revision 2) > + var handlers = aHandlerInfo.possibleApplicationHandlers; > + var apps = handlers.enumerate(); > + while (apps.hasMoreElements()) { > + var handler = apps.getNext(); > + handler.QueryInterface(Ci.nsIHandlerApp); > + var serializable = toSerializable(handler); > + if (serializable != null) { > + handlerObj.possibleHandlers.push(serializable); > + } > + } The old service made sure that the set of possible handlers included the preferred handler, if that was found in the data store. We should have a test that checks this is still the case. ::: uriloader/exthandler/nsHandlerService-json.js:448 (Diff revision 2) > + if ((aHandlerInfo.type in this._store.data.mimetypes)) > + return true; > + if ((aHandlerInfo.type in this._store.data.schemes)) > + return true; Same as above about the need to check the type of the argument. ::: uriloader/exthandler/nsHandlerService-json.js:457 (Diff revision 2) > + if ((aHandlerInfo.type in this._store.data.mimetypes)) { > + delete this._store.data.mimetypes[aHandlerInfo.type]; > + } else if ((aHandlerInfo.type in this._store.data.schemes)) { > + delete this._store.data.schemes[aHandlerInfo.type]; > + } Same. ::: uriloader/exthandler/nsHandlerService-json.js:500 (Diff revision 2) > + catch(ex) { > + // Note: for historical reasons, we don't actually check to see > + // if the exception is NS_ERROR_FILE_UNRECOGNIZED_PATH, which is what > + // nsILocalFile::initWithPath throws when a path is relative. > + > + file = Service.dirSvc.get("XCurProcD", Ci.nsIFile); There is a typo here, this means that this fallback code would never work. This also means this part of the code is not tested. On the other hand, it seems that this fallback is another legacy feature that we don't need. On my Windows system, "XCurProcD" point to the "browser" subfolder of the installation directory, which seems strange as a location to base relative paths for helper applications, even if they are added by external installation programs. Most likely, we should require all paths to be absolute, or ignore them. To do this, you can simply use the "new FileUtils.File()" constructor inside a try-catch block, and remove the _getFileWithPath function. ::: uriloader/exthandler/nsHandlerService-json.js:524 (Diff revision 2) > + if (aClass.localeCompare("mimetypes") == 0) { > + handler = mimeSvc.getFromTypeAndExtension(type, null); > + } else if (aClass.localeCompare("schemes") == 0){ > + handler = protoSvc.getProtocolHandlerInfo(type); > + } else { > + continue; > + } There is no reason to use localeCompare here, aClass can only have two values specified eralier. You can also assume there isn't a third case to be handled. Actually, this loop is now so simple that you don't need the _appendHandlers function, just implement the loop two times in "enumerate". ::: uriloader/exthandler/tests/unit/test_handlerService-json.js:20 (Diff revision 2) > +XPCOMUtils.defineLazyServiceGetter(this, "handlerJsonSvc", > + "@mozilla.org/uriloader/handler-service-json;1", > + "nsIHandlerService"); You need to run the tests two times. In the first, use "@mozilla.org/uriloader/handler-service-json;1" here. In the second, use "@mozilla.org/uriloader/handler-service;1". To do this, rename this file to "common_test_handlerService.js", then invoke it from "test_handlerService_rdf.js" and "test_handlerService_json.js", using the same technique as "toolkit/components/jsdownloads/test/unit/common_test_Download.js". ::: uriloader/exthandler/tests/unit/test_handlerService-json.js:59 (Diff revision 2) > + do_check_neq(currentHandlerTypes.indexOf(handlerInfo.type), -1); > + } > +}); > + > +add_task(function* testExists() { > + yield prepareImportDB(); This does nothing, because the service has already loaded the file. As mentioned above, you should force the reload of the data from disk. In the function that does the reload, you'll need to ensure that the contents of the JSON file are saved to disk before the new JSONFile object is initialized. You'll need to add the code for reloading the data to the old RDF-based service, so we can run the same tests on it. This path likely needs more tests, as I've indicated above during the review. I haven't reviewed the remaining tests, but I've run them, and one check fails locally for me. Please ensure that all tests pass before the next review round, even if not all of them are implemented.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review102134 > You need to run the tests two times. In the first, use "@mozilla.org/uriloader/handler-service-json;1" here. In the second, use "@mozilla.org/uriloader/handler-service;1". > > To do this, rename this file to "common_test_handlerService.js", then invoke it from "test_handlerService_rdf.js" and "test_handlerService_json.js", using the same technique as "toolkit/components/jsdownloads/test/unit/common_test_Download.js". Since this test is mainly about testing the import functionality(two services don't load the same file) and new API implementation, I am wondering does the original service(RDF-based) need this test as well? In my opinion, the original test ("uriloader/exthandler/tests/unit/test_handlerService.js") is the one we need to run twice to make sure two services have the same behavior. What do you think? > This does nothing, because the service has already loaded the file. As mentioned above, you should force the reload of the data from disk. > > In the function that does the reload, you'll need to ensure that the contents of the JSON file are saved to disk before the new JSONFile object is initialized. > > You'll need to add the code for reloading the data to the old RDF-based service, so we can run the same tests on it. > > This path likely needs more tests, as I've indicated above during the review. > > I haven't reviewed the remaining tests, but I've run them, and one check fails locally for me. Please ensure that all tests pass before the next review round, even if not all of them are implemented. Yes, it does nothing if we run all the tests. However, if we only run one test we need to prepare the DB before checking. I will add reload functionality in the next patch. For the failed case, I will run the tests on three platforms(Linux, Mac, Windows) before uploading next time. Sorry about that.

(In reply to Alphan Chen [:alchen] from comment #12) > > Since this test is mainly about testing the import functionality(two > services don't load the same file) and new API implementation, I am > wondering does the original service(RDF-based) need this test as well? > > In my opinion, the original test > ("uriloader/exthandler/tests/unit/test_handlerService.js") is the one we > need to run twice to make sure two services have the same behavior. > What do you think? > In this test, two services begin with the same condition(no DB existing). I think it is a better test to do it twice.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review102134 > You can use simpler logic: > > return data || { version: {}, mimetypes: {}, schemes: {} }; > > We must assume that nobody will alter the JSON file, because there is always a way to break the code by doing that, even if we did a more complicated check for initialization. > > Also, we will not do the import from RDF here - any failure in the dataPostProcessor will cause the JSON file to be unavailable for reading and for writing. So, you can remove the comments. Since the data will be {} (not undefined or null) when there is no DB, I will check "data.schemes" here.

(In reply to Alphan Chen [:alchen] from comment #13) > (In reply to Alphan Chen [:alchen] from comment #12) > > > > Since this test is mainly about testing the import functionality(two > > services don't load the same file) and new API implementation, I am > > wondering does the original service(RDF-based) need this test as well? > > > > In my opinion, the original test > > ("uriloader/exthandler/tests/unit/test_handlerService.js") is the one we > > need to run twice to make sure two services have the same behavior. > > What do you think? > > > In this test, two services begin with the same condition(no DB existing). > I think it is a better test to do it twice. You're right that running test_handlerService.js twice is the best way to check that the API behavior is the same as the old service. The only difficulty is that test_handlerService.js actually tests the combination of nsIMIMEService and nsIHandlerService, so you actually need to mock nsIHandlerService using nsIComponentRegistrar so that the nsIMIMEService.getFromTypeAndExtension call operates on the JSON version. This is unfortunately also required for any test involving the nsIHandlerService.enumerate method, because it calls into nsIMIMEService. This is a design issue with the current API. Note that while test_handlerService.js claims to test round-trip data integrity, it does not actually test what is written to disk. This may be fine for the RDF back-end because the service does not keep any other state outside of the RDF data source, and the RDF data source guarantees correct serialization. For the JSON back-end, we definitely need the new tests you've written for the serialization to disk. I agree that if we run test_handlerService.js twice, we don't need to run the JSON back-end tests twice. However, most test cases for the JSON serialization will be very similar to those in test_handlerService.js, with the addition of the data reload, so we'll end up with some duplication of test cases anyways. With regard to the fact that test_handlerService.js starts from an empty data source, that is true, but we also need JSON serialization tests that start from an empty data source, in addition to those that start from a predetermined file. Again, the Logins tests are a good reference and you can see that they test both situations.

(In reply to Alphan Chen [:alchen] from comment #12) > Yes, it does nothing if we run all the tests. > However, if we only run one test we need to prepare the DB before checking. > I will add reload functionality in the next patch. I see what you mean, but by doing this you're assuming that all test cases will end with the service in a state that matches the predefined JSON file. This happens to be true because all tests are read-only except the last one, but this does not make the tests atomic, because if you add any test case that writes data in-between, you may break the the following tests. The goal should be that all the test cases initialize the database in the way they need it, either empty or from the predefined data, and clean up by leaving an empty database after they've finished. > For the failed case, I will run the tests on three platforms(Linux, Mac, > Windows) before uploading next time. > Sorry about that. No worries!

(In reply to Alphan Chen [:alchen] from comment #14) > Since the data will be {} (not undefined or null) when there is no DB, I > will check "data.schemes" here. Ah, my bad, checking one property sounds good to me.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96782/#review104522 ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:195 (Diff revision 3) > + throw Cr.NS_ERROR_NO_INTERFACE; > + return this; > + } > +}; > + > +add_task(function* testStoreForDBusHandler() { I can pass the test for dBusHandler on Ubuntu. However, there is undefined error on mac and windows. So I comment the test first. 0:00.58 PROCESS_OUTPUT: Thread-1 (pid:88706) "JavaScript strict warning: file:///Users/.../gecko-dev/obj-x86_64-apple-darwin16.3.0/dist/Nightly.app/Contents/Resources/components/nsHandlerService.js, line 594: ReferenceError: reference to undefined property "@mozilla.org/uriloader/dbus-handler-app;1”"

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review102134 > In general, I'd prefer if we only stored properties that are different from the default value, but we can also do this after solving the other issues with the patch. Do you mean that we need to compare all the properties of aHandler with this._store.data.mimetypes[type] or (schemes[type]) before storing?

(In reply to Alphan Chen [:alchen] from comment #19) > I can pass the test for dBusHandler on Ubuntu. > However, there is undefined error on mac and windows. > So I comment the test first. For tests like this one, that are platform-specific, we usually add code at the beginning that checks if the test is applicable or not. In this case we can use feature detection instead of just checking the platform, because the ContractID in Components.classes does not exist on Mac and Windows: add_task(function* testStoreForDBusHandler() { if (!("@mozilla.org/uriloader/dbus-handler-app;1" in Cc)) { do_print("Skipping test because it does not apply to this platform."); return; } // ...actual test code... });

(In reply to Alphan Chen [:alchen] from comment #20) > > In general, I'd prefer if we only stored properties that are different from the default value, but we can also do this after solving the other issues with the patch. > > Do you mean that we need to compare all the properties of aHandler with > this._store.data.mimetypes[type] or (schemes[type]) before storing? I mean that we should check if strings are empty, arrays have no elements, or objects have no keys, and in that case avoid including the property in the serializable representation. For example, you wouldn't see "description":"", "fileExtensions":[], or "preferredHandler":{} in the JSON file, they would just be skipped. For example, we do this for downloads here: https://dxr.mozilla.org/mozilla-central/rev/8eaf154b385bbe0ff06155294ccf7962aa2d3324/toolkit/components/jsdownloads/src/DownloadCore.jsm#1297-1303 Note that this is more applicable to the nsIHandlerInfo than the nsIHandlerApp. I think that today none of the fields that we serialize for the nsIHandlerApp can be empty.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review105688 Thanks for the update, sorry if it took so long before I could get to this review. The changes to the structure of the tests and the additions to nsHandlerService.js look good. However, there are still various issues with the main implementation that I've detailed below. I've not reviewed all the tests, but I've checked test coverage by commenting some lines of the production code and seeing if the tests actually failed. It seems that some code paths need additional tests, I've commented where that's necessary. ::: uriloader/exthandler/nsHandlerService-json.js:12 (Diff revision 3) > +XPCOMUtils.defineLazyModuleGetter(this, "JSONFile", > + "resource://gre/modules/JSONFile.jsm"); > +XPCOMUtils.defineLazyServiceGetter(this, "gMimeSvc", > + "@mozilla.org/mime;1", > + "nsIMIMEService"); > +XPCOMUtils.defineLazyServiceGetter(this, "gExternalProtocolService", > + "@mozilla.org/uriloader/external-protocol-service;1", > + "nsIExternalProtocolService"); > +XPCOMUtils.defineLazyModuleGetter(this, "FileUtils", > + "resource://gre/modules/FileUtils.jsm"); nit: put all the module getters first, then an empty line, then the service getters. ::: uriloader/exthandler/nsHandlerService-json.js:14 (Diff revision 3) > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/Task.jsm"); > + > +XPCOMUtils.defineLazyModuleGetter(this, "JSONFile", > + "resource://gre/modules/JSONFile.jsm"); > +XPCOMUtils.defineLazyServiceGetter(this, "gMimeSvc", Call this gMIMEService, and not gMIMESvc, because the interface is called nsIMIMEService. ::: uriloader/exthandler/nsHandlerService-json.js:29 (Diff revision 3) > + this._init(); > +} > + > +HandlerService.prototype = { > + > + // XPCOM Plumbing Sorry, when I said to remove the "***" headers I also meant that the unneeded comments in them should be removed too. For example in this case "XPCOM Plumbing" doesn't add any information, if someone is not already familiar with QueryInterface and XPCOMUtils they would need to look them up anyways. ::: uriloader/exthandler/nsHandlerService-json.js:34 (Diff revision 3) > + // Initialization & Destruction > + _init() { > + // Observe handlersvc-json-replace so we can switch to the datasource > + Services.obs.addObserver(this, "handlersvc-json-replace", false); > + > + // Observe xpcom-shutdown so we can remove these observers > + // when the application shuts down. > + Services.obs.addObserver(this, "xpcom-shutdown", false); > + > + // LOAD DATA > + this._store = new JSONFile({ > + path: OS.Path.join(OS.Constants.Path.profileDir, > + "mimeTypes.json"), > + dataPostProcessor: this._dataPostProcessor.bind(this), > + }); > + this._store.ensureDataReady(); > + > + // Update DB > + // do any necessary updating of the datastore > + this._updateDB(); > + }, I made the example of "// Update DB" in my previous review, but pretty much every comment in this function is redundant. Please remove them. You can likely add a comment for "handlersvc-json-replace" in the "observe" method instead. ::: uriloader/exthandler/nsHandlerService-json.js:36 (Diff revision 3) > + // Observe handlersvc-json-replace so we can switch to the datasource > + Services.obs.addObserver(this, "handlersvc-json-replace", false); > + > + // Observe xpcom-shutdown so we can remove these observers > + // when the application shuts down. > + Services.obs.addObserver(this, "xpcom-shutdown", false); There's a slightly better technique you can use here. If you add Ci.nsISupportsWeakReference to the generateQI array above, and pass "true" as the third argument here, you don't need to call removeObserver, so you don't need to register for "xpcom-shutdown". (You don't need to change how the old module works, since we're going to remove it soon. The way you added the new observer to the old module is good.) ::: uriloader/exthandler/nsHandlerService-json.js:44 (Diff revision 3) > + this._store = new JSONFile({ > + path: OS.Path.join(OS.Constants.Path.profileDir, > + "mimeTypes.json"), > + dataPostProcessor: this._dataPostProcessor.bind(this), > + }); > + this._store.ensureDataReady(); You are repeating most of the code that initializes the JSON file in the _onDBChange function below. To avoid the repetition, I recommend making "this._store" a lazy getter backed by a "this.__store" member, just like nsHandlerService does with "this.__ds". The observer notification for "handlersvc-json-replace" would then nullify "this.__store". ::: uriloader/exthandler/nsHandlerService-json.js:58 (Diff revision 3) > + this._updateDB(); > + }, > + > + > + _dataPostProcessor(data) { > + return (data.schemes)? data:{ version: {}, mimetypes: {}, schemes: {} }; Adjust spacing and unneeded parentheses: return data.schemes ? data : { version: {}, mimetypes: {}, schemes: {} }; (On second thought, checking "data.version" instead of "data.schemes" seems more logical, but "schemes" is also fine.) ::: uriloader/exthandler/nsHandlerService-json.js:64 (Diff revision 3) > + }, > + > + > + _updateDB() { > + > + this._store.ensureDataReady(); As I mentioned in the previous review comment, all the calls to ensureDataReady except the one after the file is loaded are redundant and can be removed. ::: uriloader/exthandler/nsHandlerService-json.js:68 (Diff revision 3) > + const locale = Cc["@mozilla.org/chrome/chrome-registry;1"] > + .getService(Ci.nsIXULChromeRegistry) > + .getSelectedLocale("global"); nit: while "const" is correct, "let" is what we normally use. ::: uriloader/exthandler/nsHandlerService-json.js:73 (Diff revision 3) > + let prefsDefaultHandlersVersion = Number(handlerSvcBranch. > + getComplexValue("defaultHandlersVersion", > + Ci.nsIPrefLocalizedString).data); nit: improve indentation, avoid splitting after the dot if possible. This line goes over 80 characters anyways, but if you're wrapping it's better to stay withing the 80 character limit if possible. ::: uriloader/exthandler/nsHandlerService-json.js:83 (Diff revision 3) > + if (locale in this._store.data.version) { > + defaultHandlersVersion = this._store.data.version[locale]; > + } > + > + if (defaultHandlersVersion < prefsDefaultHandlersVersion ) { > + this._injectNewDefaults(); The entire import procedure is not tested. This is because you're actually testing a template database that looks similar to the one initialized by the preferences, instead of the actual import procedure. While the existing "test_handlerService.js" also checks the import, I think it's worth writing a simple test that just checks that the import is happening and the protocols are present when starting from an empty database, without going into too much detail. ::: uriloader/exthandler/nsHandlerService-json.js:86 (Diff revision 3) > + } catch (ex) { > + // if injecting the defaults failed, we do nothing on original DB. > + } Add a "Cu.reportError(ex);" call. nit: double space to be removed ::: uriloader/exthandler/nsHandlerService-json.js:93 (Diff revision 3) > + } > + }, > + > + > + _injectNewDefaults() { > + // get handler service pref branch unneeded comment ::: uriloader/exthandler/nsHandlerService-json.js:150 (Diff revision 3) > + createInstance(Ci.nsIWebHandlerApp); > + > + handlerApp.uriTemplate = handlerPrefs.uriTemplate; > + handlerApp.name = handlerPrefs.name; > + > + if (!this._isInHandlerArray(possibleHandlers, handlerApp)) { nit: you can move _isInHandlerArray just below this function, since it's used only here. ::: uriloader/exthandler/nsHandlerService-json.js:169 (Diff revision 3) > + this._store = null; > + this._store = new JSONFile({ The JSONFile constructor with correct arguments is infallible and has no side effects, so there's no need to nullify the variable right before you assign it. Something missing from this implementation is that you should finalize the existing JSONFile, to ensure that the data is written to disk. To do this, you can call into the internal function "this._store._saver.finalize()" and wait on the returned Promise. This has the unwanted effect that all the previous JSONFile objects will keep their shutdown blockers registered, but this is acceptable because this code is test-only. Please add a comment to explain this in the code. ::: uriloader/exthandler/nsHandlerService-json.js:176 (Diff revision 3) > + path: OS.Path.join(OS.Constants.Path.profileDir, > + "mimeTypes.json"), > + dataPostProcessor: this._dataPostProcessor.bind(this), > + }); > + > + yield this._store.load(); There is no particular need to use asynchronous loading here, even if we can. This code path is test-only anyways and performance isn't an issue. In fact, most of the code in this function will be present already in the lazy getter for "this._store", where you can call ensureDataReady. ::: uriloader/exthandler/nsHandlerService-json.js:178 (Diff revision 3) > + Services.obs.notifyObservers(null, > + "handlersvc-json-replace-complete", null); For clarity, move the notifyObservers call directly in the implementation of "this.observe", using a "Promise.then" function. ::: uriloader/exthandler/nsHandlerService-json.js:210 (Diff revision 3) > + // nsIHandlerService > + enumerate() { Include the "// nsIHandlerService" interface indicator before every method that implements the interface. This is the new style that replaces the single header. ::: uriloader/exthandler/nsHandlerService-json.js:212 (Diff revision 3) > + }, > + > + > + // nsIHandlerService > + enumerate() { > + var handlers = Cc["@mozilla.org/array;1"]. let instead of var ::: uriloader/exthandler/nsHandlerService-json.js:226 (Diff revision 3) > + handlers.appendElement(handler, false); > + } > + return handlers.enumerate(); > + }, > + > + fillHandlerInfo(aHandlerInfo, aOverrideType) { In the "observe(subject, topic, data)" method you are using the most recent convention, which is to avoid the "a" prefix for arguments. Since this is a new file, I think we should do the same and remove the "a" prefix from all arguments, and they should start with a lowercase letter. ::: uriloader/exthandler/nsHandlerService-json.js:233 (Diff revision 3) > + if ((aHandlerInfo instanceof Ci.nsIMIMEInfo)) { > + if (type in this._store.data.mimetypes) { > + overrideObj = this._store.data.mimetypes[type]; > + } else { > + throw new Components.Exception("handlerSvc fillHandlerInfo: don't know this type", > + Cr.NS_ERROR_NOT_AVAILABLE); > + } > + } else { > + if (type in this._store.data.schemes) { > + overrideObj = this._store.data.schemes[type]; > + } else { > + throw new Components.Exception("handlerSvc fillHandlerInfo: don't know this type", > + Cr.NS_ERROR_NOT_AVAILABLE); > + } > + } You can greatly simplify this function and the other ones using a call like: let handlerList = getHandlerListByHandlerInfoType(handlerInfo); This would return a reference to the "mimetypes" or "schemes" object based on which type of handlerInfo is provided. Add a JSDoc-style comment to the helper function to explain this. ::: uriloader/exthandler/nsHandlerService-json.js:261 (Diff revision 3) > + } > + > + for (let handler of overrideObj.possibleHandlers) { > + let possibleHandler = this.handlerAppFromSerializable(handler); > + if (possibleHandler != null) { > + aHandlerInfo.possibleApplicationHandlers.appendElement(possibleHandler, false); Not tested. ::: uriloader/exthandler/nsHandlerService-json.js:270 (Diff revision 3) > + for (let extension of overrideObj.fileExtensions) > + aHandlerInfo.appendExtension(extension); Just like single-line "if" clauses, the single-line "for" clauses should have braces too. ::: uriloader/exthandler/nsHandlerService-json.js:282 (Diff revision 3) > + * @param aHandler > + * A nsIHandlerApp handler app > + * @returns Serializable representation of a handler app object. > + */ > + handlerAppToSerializable(aHandler) { > + var serializable; let instead of var ::: uriloader/exthandler/nsHandlerService-json.js:313 (Diff revision 3) > + * @param aHandlerObj > + * Serializable representation of a handler object. > + * @returns {nsIHandlerApp} the handler app, if any; otherwise null > + */ > + handlerAppFromSerializable(aHandlerObj) { > + var handlerApp; let instead of var ::: uriloader/exthandler/nsHandlerService-json.js:316 (Diff revision 3) > + */ > + handlerAppFromSerializable(aHandlerObj) { > + var handlerApp; > + if ("path" in aHandlerObj) { > + if (aHandlerObj.path) { > + var file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile); You're not using this nsILocalFile instance anymore. ::: uriloader/exthandler/nsHandlerService-json.js:319 (Diff revision 3) > + if (!file.exists()) { > + return null; > + } This is not tested, we should assert that non-existent executables are not saved. ::: uriloader/exthandler/nsHandlerService-json.js:351 (Diff revision 3) > + handlerApp.name = aHandlerObj.name; > + return handlerApp; > + }, > + > + > + store(aHandlerInfo) { nit: move the "store" method before the "fillHandlerInfo" method. ::: uriloader/exthandler/nsHandlerService-json.js:353 (Diff revision 3) > + let description = aHandlerInfo.description; > + let action = aHandlerInfo.preferredAction; > + let askBeforeHandling = aHandlerInfo.alwaysAskBeforeHandling; > + > + let handlerObj = { > + "description": description, > + "action": action, > + "askBeforeHandling": askBeforeHandling, > + "fileExtensions": [], > + "preferredHandler": {}, > + "possibleHandlers": [], > + }; You are using these variables only once, and the quotes are not needed. You can simplify the object initializer to look something like: { action: handlerInfo.preferredAction, } ::: uriloader/exthandler/nsHandlerService-json.js:360 (Diff revision 3) > + let askBeforeHandling = aHandlerInfo.alwaysAskBeforeHandling; > + > + let handlerObj = { > + "description": description, > + "action": action, > + "askBeforeHandling": askBeforeHandling, This isn't fully tested. If I change this to "askBeforeHandling: false", the tests still pass. ::: uriloader/exthandler/nsHandlerService-json.js:367 (Diff revision 3) > + "preferredHandler": {}, > + "possibleHandlers": [], > + }; > + > + let preferredHandler = aHandlerInfo.preferredApplicationHandler; > + if (preferredHandler != null) { Just "if (preferredHandler)", and the same in the rest of the file. This is the way we normally use to check if an object isn't null. ::: uriloader/exthandler/nsHandlerService-json.js:374 (Diff revision 3) > + let handler = apps.getNext(); > + handler.QueryInterface(Ci.nsIHandlerApp); This can be simplified in the same way you did for the other occurrence of QueryInterface. Sometimes I suggest an improvement only once, and it's up to you to fix the same issue throughout the entire patch. ::: uriloader/exthandler/nsHandlerService-json.js:378 (Diff revision 3) > + while (apps.hasMoreElements()) { > + let handler = apps.getNext(); > + handler.QueryInterface(Ci.nsIHandlerApp); > + let serializable = this.handlerAppToSerializable(handler); > + if (serializable != null) { > + handlerObj.possibleHandlers.push(serializable); Not tested. Also, I think the code in the old service that made sure that the set of possible handlers included the preferred handler still hasn't been ported. We need to do add a test for this, this should pass already with the old service, and also with the new service once we've ported the code. ::: uriloader/exthandler/nsHandlerService-json.js:407 (Diff revision 3) > + if (aHandlerInfo.type in this._store.data.mimetypes) { > + return true; > + } > + } else { > + if (aHandlerInfo.type in this._store.data.schemes) { > + return true; Not tested for protocols. ::: uriloader/exthandler/nsHandlerService-json.js:422 (Diff revision 3) > + if (aHandlerInfo.type in this._store.data.mimetypes) { > + delete this._store.data.mimetypes[aHandlerInfo.type]; > + } > + } else { > + if (aHandlerInfo.type in this._store.data.schemes) { > + delete this._store.data.schemes[aHandlerInfo.type]; Not tested for protocols. ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:47 (Diff revision 3) > + yield removeImportDB(); > + yield reloadData(); Most of these tests will need to do things in this order: 1. Unload the data (using the observer notification) 2. Delete the store, or prepare the template store If necessary: 3. Call the write methods (these will reload the data) 4. Unload the data (using the observer notification) 5. Call the read methods (these will reload the data) 6. Verify that the data was read correctly The unload in step 1 must be done before step 2, to ensure that the JSON or RDF file is not locked. Your removeImportDB and prepareImportDB methods can already do steps 1 and 2 together, so you don't need to call "reloadData" every time. You still need a separate "reloadData" call in the middle of most tests. ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:225 (Diff revision 3) > + try { > + let handlerInfo = gMimeSvc.getFromTypeAndExtension("nonexistent/type", null); > + gHandlerSvc.fillHandlerInfo(handlerInfo, "nonexistent/type2"); > + do_throw("Shouldn't be able to do fillHandlerInfo with non-existing type"); > + do_check_false(true); > + } catch (ex) { > + do_check_eq(ex.result, Cr.NS_ERROR_NOT_AVAILABLE); > + } You can likely use "Assert.throws": Assert.throws(() => gHandlerSvc.fillHandlerInfo(handlerInfo, "nonexistent/type2"), ex => ex.result == Cr.NS_ERROR_NOT_AVAILABLE); ::: uriloader/exthandler/tests/unit/test_handlerService_json.js:41 (Diff revision 3) > + let src = "mimeTypes.json"; > + let dst = OS.Path.join(OS.Constants.Path.profileDir, src); > + > + yield OS.File.copy(src, dst); This normally won't work because you're using a relative path as the source. The current directory is the right one only when you run the test individually. Just use "do_get_file" to get the full path to the template file, like you do above for the script. ::: uriloader/exthandler/tests/unit/test_handlerService_json.js:59 (Diff revision 3) > + Services.obs.notifyObservers(null, "handlersvc-json-replace", null); > + yield TestUtils.topicObserved("handlersvc-json-replace-complete"); You need to use this pattern to avoid possible race conditions: let promise = yield TestUtils.topicObserved(...); Services.obs.notifyObservers(...); yield promise; ::: uriloader/exthandler/tests/unit/test_handlerService_rdf.js:25 (Diff revision 3) > +const pdfHandlerInfo = gMimeSvc.getFromTypeAndExtension("application/pdf", null); > +const gzipHandlerInfo = gMimeSvc.getFromTypeAndExtension("application/x-gzip", null); > + > + > +function run_test() { > + do_get_profile(); > + run_next_test(); > +} I'd actually move all the identical parts to the shared file, including all the module and service getters except the one for gHandlerService. ::: uriloader/exthandler/tests/unit/xpcshell.ini:19 (Diff revision 3) > + > +[test_handlerService_json.js] > +support-files = mimeTypes.json > + > +[test_handlerService_rdf.js] nit: move these under the existing [test_handlerService.js] entry. While these entries are not strictly in alphabetical order, it's good to keep them together. Also, remove unneeded blank lines.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review105688 > The entire import procedure is not tested. This is because you're actually testing a template database that looks similar to the one initialized by the preferences, instead of the actual import procedure. > > While the existing "test_handlerService.js" also checks the import, I think it's worth writing a simple test that just checks that the import is happening and the protocols are present when starting from an empty database, without going into too much detail. All the thing needed to be imported in "_injectNewDefault" are from preference. However, we cannot read anything from preference in our xpcshell test. So I think we don't need this test. > There is no particular need to use asynchronous loading here, even if we can. This code path is test-only anyways and performance isn't an issue. > > In fact, most of the code in this function will be present already in the lazy getter for "this._store", where you can call ensureDataReady. I use "ensureDataReady" at first. However, I met some problems at that time. Will double check the case again.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review105688 > All the thing needed to be imported in "_injectNewDefault" are from preference. > However, we cannot read anything from preference in our xpcshell test. > So I think we don't need this test. Also, we include the test of starting from an empty database in "testImportAndReload()" currently(revision 3). In the second part, the test begins without DB.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review105688 > The JSONFile constructor with correct arguments is infallible and has no side effects, so there's no need to nullify the variable right before you assign it. > > Something missing from this implementation is that you should finalize the existing JSONFile, to ensure that the data is written to disk. > > To do this, you can call into the internal function "this._store._saver.finalize()" and wait on the returned Promise. > > This has the unwanted effect that all the previous JSONFile objects will keep their shutdown blockers registered, but this is acceptable because this code is test-only. Please add a comment to explain this in the code. On second thought, since this is a test-only code, we don't care if the data is written to disk or not. We just want to read a new one.

(In reply to Alphan Chen [:alchen] from comment #26) > > Something missing from this implementation is that you should finalize the existing JSONFile, to ensure that the data is written to disk. > > > > To do this, you can call into the internal function "this._store._saver.finalize()" and wait on the returned Promise. > > On second thought, since this is a test-only code, we don't care if the data > is written to disk or not. We just want to read a new one. Writing the changed data is not the only problem here, although there *are* tests where you want to reload the data you have written to disk, instead of starting with a new file. Even if you want to start with a new file, there is a real issue that can happen. In the JSONFile implementation, the "saveSoon" method starts a timer that atomically writes the file to disk after some time. If you don't stop that timer, it can fire after you overwrite or delete the data store, resulting in loading the wrong data. > > This has the unwanted effect that all the previous JSONFile objects will keep their shutdown blockers registered, but this is acceptable because this code is test-only. Please add a comment to explain this in the code. To elaborate on this previous statement, the small issue is that you can only call the "finalize" method of DeferredTask once: https://dxr.mozilla.org/mozilla-central/rev/6a23526fe5168087d7e4132c0705aefcaed5f571/toolkit/modules/DeferredTask.jsm#240-242 Because shutdown blockers for old JSONFile instances are still registered, but you already called "finalize" manually, this will result in harmless console errors when the test ends. Replacing a JSONFile instance in this way in production code can result in race conditions and should not be done. Tests on the same profile run sequentially instead, so this is acceptable.

(In reply to Alphan Chen [:alchen] from comment #25) > > All the thing needed to be imported in "_injectNewDefault" are from preference. > > However, we cannot read anything from preference in our xpcshell test. > > So I think we don't need this test. > > Also, we include the test of starting from an empty database in > "testImportAndReload()" currently(revision 3). In the second part, the test > begins without DB. Ah, I see what you mean. The xpcshell tests currently run without the Firefox preferences by default, there is some discussion about this in bug 1168178. To test the import code path, you should add "firefox-appdir = browser" to the [DEFAULT] section of the "xpcshell.ini" file. This gives you and the tested code access to the Firefox preferences, which is the most important configuration we should really test. The fact it wasn't tested was an existing issue. The current "test_handlerService.js" works in both cases because it tests whether the preferences for the import are present or absent: https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/tests/unit/test_handlerService.js#124 In the new test we don't need the conditional, because "firefox-appdir = browser" is the only relevant configuration we should test in order to land this bug.

(In reply to :Paolo Amadini from comment #27) > Even if you want to start with a new file, there is a real issue that can > happen. In the JSONFile implementation, the "saveSoon" method starts a timer > that atomically writes the file to disk after some time. If you don't stop > that timer, it can fire after you overwrite or delete the data store, > resulting in loading the wrong data. Oh, I got your point. :) > > > > This has the unwanted effect that all the previous JSONFile objects will keep their shutdown blockers registered, but this is acceptable because this code is test-only. Please add a comment to explain this in the code. > > To elaborate on this previous statement, the small issue is that you can > only call the "finalize" method of DeferredTask once: > > https://dxr.mozilla.org/mozilla-central/rev/ > 6a23526fe5168087d7e4132c0705aefcaed5f571/toolkit/modules/DeferredTask. > jsm#240-242 > > Because shutdown blockers for old JSONFile instances are still registered, > but you already called "finalize" manually, this will result in harmless > console errors when the test ends. > Since xpcshell test will do "assertNoUncaughtRejections" in the end, I met the test failed due to the rejection. Look like I should remove the blocker after calling "finalize" manually.(Maybe add a test only method in JSONFile.jsm?!) Any other ideas? 0:00.72 TEST_STATUS: Thread-1 testFillHandlerInfoWithError FAIL [testFillHandlerInfoWithError : 221] A promise chain failed to handle a rejection: Error: The object has been already finalized. - rejection date: Thu Jan 19 2017 10:37:33 GMT+0800 (CST) - stack: finalize@resource://gre/modules/DeferredTask.jsm:241:13 JSONFile/<@resource://gre/modules/JSONFile.jsm:108:56 trigger@resource://gre/modules/AsyncShutdown.jsm:719:23 _wait@resource://gre/modules/AsyncShutdown.jsm:866:7 wait@resource://gre/modules/AsyncShutdown.jsm:850:28 observe@resource://gre/modules/AsyncShutdown.jsm:533:17 _execute_test@/home/alphan/My_GitHub/gecko-cinnabar/testing/xpcshell/head.js:636:5 @-e:1:1 - false == true resource://testing-common/PromiseTestUtils.jsm:assertNoUncaughtRejections:221 /home/alphan/My_GitHub/gecko-cinnabar/testing/xpcshell/head.js:_execute_test:644

(In reply to Alphan Chen [:alchen] from comment #29) > Since xpcshell test will do "assertNoUncaughtRejections" in the end, I met > the test failed due to the rejection. Look like I should remove the blocker > after calling "finalize" manually.(Maybe add a test only method in > JSONFile.jsm?!) Any other ideas? Ah, looks like someone in bug 1332024 found exactly the same issue in another test and wrote a patch to add a "JSONFile.finalize()" method :-) Good timing! I was a bit undecided if we needed this as a non-test-only method, but it looks like it's better to have it, so I'll review the patch in the other bug. You can already import the work in progress patch there if you want, even if it requires a revision.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review106908 ::: uriloader/exthandler/nsHandlerService-json.js:378 (Diff revision 3) > + while (apps.hasMoreElements()) { > + let handler = apps.getNext(); > + handler.QueryInterface(Ci.nsIHandlerApp); > + let serializable = this.handlerAppToSerializable(handler); > + if (serializable != null) { > + handlerObj.possibleHandlers.push(serializable); Yes, I missed the implementation of "the set of possible handlers included the preferred handler" in store(). Will fix it in the later patch. However, I cannot test the code due to enumerate() get the list by "gMIMEService.getFromTypeAndExtension(type, null);". In the current design, MIMEService still uses the RDF backend to get the handler info. We can only get the handlerInfo from JSON DS by "fillHandlerInfo". However, there is the same mechanism of "the set of possible handlers included the preferred handle" in it. We already have the test for this mechanism of "fillHandlerInfo".

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review105688 > You can likely use "Assert.throws": > > Assert.throws(() => gHandlerSvc.fillHandlerInfo(handlerInfo, "nonexistent/type2"), > ex => ex.result == Cr.NS_ERROR_NOT_AVAILABLE); I will meet exception when using the code you said. If I expand the arrow function. Then it is good. 0:00.70 LOG: Thread-1 ERROR Unexpected exception TypeError: 'prototype' property of ex => ex.result == Cr.NS_ERROR_NOT_AVAILABLE is not an object at resource://testing-common/Assert.jsm:317 317 } else if (actual instanceof expected) { "expected" is ex => ex.result == Cr.NS_ERROR_NOT_AVAILABLE. It looks like we cannot put arrow function on the right-hand side of instantof.

(In reply to Alphan Chen [:alchen] from comment #31) > Yes, I missed the implementation of "the set of possible handlers included > the preferred handler" in store(). Will fix it in the later patch. So, probably my mistake here, I looked at the old implementation again and it does nothing special when storing the data, it just stores what is in the object. However, when the old implementation processes the data on retrieval, it ensures it doesn't add the preferred handler to the possible handlers if it's already in the list. As a start, your implementation of "fillHandlerInfo" should do the same check as well. First you should write a test that fails with the current code. For example, store an nsIHandlerInfo with the same preferred handler and possible handler, then retrieve it and check that there is only one and not two possible handlers. Then you can update the code to match the test. > However, I cannot test the code due to enumerate() get the list by > "gMIMEService.getFromTypeAndExtension(type, null);". In the current design, > MIMEService still uses the RDF backend to get the handler info. Yeah, as we noticed previously we cannot test "enumerate" properly, but I don't think it's needed for this test, right? > We can only get the handlerInfo from JSON DS by "fillHandlerInfo". However, > there is the same mechanism of "the set of possible handlers included the > preferred handle" in it. We already have the test for this mechanism of > "fillHandlerInfo". It's correct that we should only test the observable behavior. For example, since we're building our datastore from scratch and we don't have to import legacy versions of the JSON format, it's indifferent whether our new implementation does the processing when storing or when retrieving. We don't need to implement both. Cases like the one above, however, *are* observable and can reveal implementation bugs, so we should test them. (This bug would result in nsIHandlerApp duplication every time the nsIHandlerInfo is stored and reloaded.) It also looks like the preferred handler is always moved to the top of the possible handlers after a reload. We should have a separate test case for that as well. (When we have all these regression tests in place, we can likely rethink and optimize the format of the data in the JSON file to avoid duplication. The tests will guarantee that the API is fully backwards-compatible with the old service.) Slightly unrelated, while I didn't review all the test cases yet, I've looked at one related to this code and I've seen it does: do_check_neq(handlerInfo.possibleApplicationHandlers.length, 0); This should look like: do_check_eq(handlerInfo.possibleApplicationHandlers.length, 1); And likely should also test the actual application handler contents. Try to be specific in the expected results. In general, "do_check_neq" should be used only in rare cases.

(In reply to Alphan Chen [:alchen] from comment #32) > It looks like we cannot put arrow function on the right-hand side of > instantof. Ah, I found bug 1237961 for that, I'll write a patch.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96782/#review108486 ::: uriloader/exthandler/nsHandlerService-json.js:171 (Diff revision 4) > + }, > + > + > + _onDBChange() { > + return Task.spawn(function* () { > + yield this.__store.finalize(); This patch is based on the fix of bug 1332024.

Thanks for the update, we're getting closer to a version of the patch where the code is adequately compact, and close to the quality of other browser code. As noted before, this is more difficult than usual because we've started from existing older code that didn't match the modern browser code. In fact, using correct spacing and formatting is made more difficult here by the fact that ESLint is not enabled for this folder. I put together the following script to run ESLint on the main module temporarily: sed -i .orig '/uriloader/d' .eslintignore cp browser/.eslintrc.js uriloader/ ./mach eslint uriloader/exthandler/nsHandlerService-json.js rm uriloader/.eslintrc.js hg revert .eslintignore rm .eslintignore.orig After configuring "mach elsint", please use it before posting new versions, and fix the reported errors. I ran this on the current version and the result included some issues I had already seen and others I had missed.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review108620 This iteration is taking time, I'm sending a few comments already although I haven't completed the review yet. Anyways, feel free to post an updated version that starts to address these comments. ::: uriloader/exthandler/nsHandlerService-json.js:33 (Diff revision 4) > + QueryInterface: XPCOMUtils.generateQI([Ci.nsISupportsWeakReference, > + Ci.nsIHandlerService > + ]), Use one of the following two styles, but don't mix them: QueryInterface: XPCOMUtils.generateQI([ Ci.nsIHandlerService, Ci.nsISupportsWeakReference, ]), QueryInterface: XPCOMUtils.generateQI([Ci.nsIHandlerService, Ci.nsISupportsWeakReference]), ::: uriloader/exthandler/nsHandlerService-json.js:38 (Diff revision 4) > + _init() { > + Services.obs.addObserver(this, "handlersvc-json-replace", true); > + }, Just move this call to the constructor. ::: uriloader/exthandler/nsHandlerService-json.js:48 (Diff revision 4) > + __store: null, > + get _store() { > + if (!this.__store) { > + this.__store = new JSONFile({ > + path: OS.Path.join(OS.Constants.Path.profileDir, > + "mimeTypes.json"), Let's rename the file to "handlers.json". It contains both MIME types and schemes, and is controlled by nsIHandlerService. I will gradually ask for more renames in the data and the code on new versions of the patch. Doing these renames incrementally will make it easier and faster for me to review the other changes at each iteration. ::: uriloader/exthandler/nsHandlerService-json.js:75 (Diff revision 4) > + let defaultHandlersVersion = -1; > + if (locale in this._store.data.version) { > + defaultHandlersVersion = this._store.data.version[locale]; > + } I did a search across all locales for this preference: https://dxr.mozilla.org/l10n-central/search?q=defaultHandlersVersion It turns out that the lowest possible value is 1. Since we don't have to keep zero into account, this code can be simplified to: let defaultHandlersVersion = this._store.data.version[locale] || 0; ::: uriloader/exthandler/nsHandlerService-json.js:85 (Diff revision 4) > + if (defaultHandlersVersion < prefsDefaultHandlersVersion ) { > + this._injectNewDefaults(); > + this._store.data.version[locale] = prefsDefaultHandlersVersion; > + } > + } catch (ex) { > + Cu.reportError("handlerSvc _updateDB failed: "+ ex); "Cu.reportError(ex);" is sufficient and even better, because we'll get the stack trace parsed by the Browser Console. ::: uriloader/exthandler/nsHandlerService-json.js:171 (Diff revision 4) > + yield this.__store.finalize(); > + this.__store = null; if (!this.__store) { return; } This allows the reinitialization to work even if this was never initialized. ::: uriloader/exthandler/nsHandlerService-json.js:264 (Diff revision 4) > + let overrideObj; > + if ((handlerInfo instanceof Ci.nsIMIMEInfo)) { > + if (type in this._store.data.mimetypes) { > + overrideObj = this._store.data.mimetypes[type]; > + } else { > + throw new Components.Exception("handlerSvc fillHandlerInfo: don't know this type", > + Cr.NS_ERROR_NOT_AVAILABLE); > + } > + } else { > + if (type in this._store.data.schemes) { > + overrideObj = this._store.data.schemes[type]; > + } else { > + throw new Components.Exception("handlerSvc fillHandlerInfo: don't know this type", > + Cr.NS_ERROR_NOT_AVAILABLE); > + } > + } You can definitely use _getHandlerListByHandlerInfoType to simplify this. ::: uriloader/exthandler/nsHandlerService-json.js:405 (Diff revision 4) > + let handlerList = this._getHandlerListByHandlerInfoType(handlerInfo); > + if (handlerList && handlerInfo.type in handlerList) { > + return true; > + } else { > + return false; > + } This can become a one-liner: return handlerInfo.type in this._getHandlerListByHandlerInfoType(handlerInfo); ::: uriloader/exthandler/nsHandlerService-json.js:416 (Diff revision 4) > + let handlerList = this._getHandlerListByHandlerInfoType(handlerInfo); > + if (handlerList && handlerInfo.type in handlerList) { > + delete handlerList[handlerInfo.type]; > + } This can also be simplified: delete this._getHandlerListByHandlerInfoType(handlerInfo)[handlerInfo.type]; ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:68 (Diff revision 4) > + run_next_test(); > +} > + > + > +add_task(function* testImportAndReload() { > + // test reload without DB We have to call removeImportDB to ensure the database is reset if we run this together with other tests. If you run this test alone, it will time out because the observer is not initialized yet. Just adding "gHandlerSvc;" at the beginning of reloadData() to force the initialization should fix the issue. When doing this, add a comment to explain why this call is needed.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review108918 Unless I've missed them, the protocol handler code paths for "store" and "fillHandlerInfo" still don't have tests, except for the default ones that are imported. I'm referring to everything stored in the "schemes" object, not the web handler applications stored in the "mimetypes" object. Since the test code can be made more readable with helper functions, I've not reviewed all the test cases thoroughly yet. ::: uriloader/exthandler/nsHandlerService-json.js:70 (Diff revision 4) > + let handlerSvcBranch = Services.prefs.getBranch("gecko.handlerService."); > + let prefsDefaultHandlersVersion = > + Number(handlerSvcBranch.getComplexValue("defaultHandlersVersion", > + Ci.nsIPrefLocalizedString).data); I've just checked that Services.prefs.getComplexValue also exists, so this can be simplified to: let prefsDefaultHandlersVersion = Number(Services.prefs.getComplexValue( "gecko.handlerService.defaultHandlersVersion", Ci.nsIPrefLocalizedString).data); ::: uriloader/exthandler/nsHandlerService-json.js:119 (Diff revision 4) > + // This clause is essentially a reimplementation of > + // nsIExternalProtocolHandlerService.getProtocolHandlerInfo(). > + // Necessary because calling that from here would make XPConnect barf > + // when getService tried to re-enter the constructor for this > + // service. Now that this code isn't called from the constructor anymore, this comment does not indicate the actual reason for which we're still reimplementing this. Necessary because we want to use this instance of the service, but nsIExternalProtocolHandlerService would call the RDF-based based version until we complete the conversion. ::: uriloader/exthandler/nsHandlerService-json.js:139 (Diff revision 4) > + let handlerPrefs = schemes[scheme][handlerNumber]; > + let handlerApp = Cc["@mozilla.org/uriloader/web-handler-app;1"]. > + createInstance(Ci.nsIWebHandlerApp); > + > + handlerApp.uriTemplate = handlerPrefs.uriTemplate; > + handlerApp.name = handlerPrefs.name; Conveniently, you can now use handlerAppFromSerializable here. ::: uriloader/exthandler/nsHandlerService-json.js:156 (Diff revision 4) > + _isInHandlerArray(array, handler) { > + let enumerator = array.enumerate(); > + while (enumerator.hasMoreElements()) { > + let handlerApp = enumerator.getNext().QueryInterface(Ci.nsIHandlerApp); > + if (handlerApp.equals(handler)) { > + return true; > + } > + } > + > + return false; > + }, This isn't currently tested, this function could always return false and the tests won't fail. I believe it can be tested simply by repeating the import on an existing database, for example increasing the version in the preferences or decreasing the version in the data. ::: uriloader/exthandler/nsHandlerService-json.js:177 (Diff revision 4) > + this.__store = null; > + }.bind(this)); > + }, > + > + > + observe(subject, topic, data) { // nsIObserver The comment should be kept to indicate that this method implements an interface. Thinking about it, this should also be added to the list of the interfaces in generateQI. The code likely works without it because of some automatic casting logic of XPCOM, but it's more correct to list it explicitly. ::: uriloader/exthandler/nsHandlerService-json.js:184 (Diff revision 4) > + // Observe handlersvc-json-replace so we can switch to the datasource > + case "handlersvc-json-replace": > + let promise = this._onDBChange(); > + promise.then(() => { > + Services.obs.notifyObservers(null, "handlersvc-json-replace-complete", null); > + }); Promise chains should be terminated with ".catch(Cu.reportError);". Now that we have only one possible observer notification, we can simplify the code by removing the "switch" statement and moving the Task.spawn call directly in here. You can still return early if the topic is not "handlersvc-json-replace", as a form of documentation. ::: uriloader/exthandler/nsHandlerService-json.js:192 (Diff revision 4) > + let handlers = Cc["@mozilla.org/array;1"]. > + createInstance(Ci.nsIMutableArray); Here and in other places, the style should be consistent. We usually align the dot under the square bracket: let handlers = Cc["@mozilla.org/array;1"] .createInstance(Ci.nsIMutableArray); ::: uriloader/exthandler/nsHandlerService-json.js:218 (Diff revision 4) > + if (handlerInfo.description) { > + handlerObj.description = handlerInfo.description; > + } > + > + let preferredHandler = handlerInfo.preferredApplicationHandler; > + let possibleHandlers = []; Move the declaration of possibleHandlers to the block of code below, that actually uses the variable. ::: uriloader/exthandler/nsHandlerService-json.js:226 (Diff revision 4) > + let handlers = handlerInfo.possibleApplicationHandlers; > + let apps = handlers.enumerate(); Unneeded intermediate variable. ::: uriloader/exthandler/nsHandlerService-json.js:240 (Diff revision 4) > + if (possibleHandlers.length) { > + handlerObj.possibleHandlers = possibleHandlers; > + } > + > + let type = handlerInfo.type; > + if(handlerInfo instanceof Ci.nsIMIMEInfo) { It's clearer to keep the instanceof check only for the block that sets the file extensions. Then you can assign directly to _getHandlerListByHandlerInfoType(handlerInfo)[handlerInfo.type]. ::: uriloader/exthandler/nsHandlerService-json.js:245 (Diff revision 4) > + if(handlerInfo instanceof Ci.nsIMIMEInfo) { > + let extEnumerator = handlerInfo.getFileExtensions(); > + let extensions = []; > + while (extEnumerator.hasMore()) { > + let extension = extEnumerator.getNext(); > + if (extensions.indexOf(extension) == -1) { This can become "!extensions.includes(extension)". You can also rewrite this using a Set, it only saves one line of code though. ::: uriloader/exthandler/nsHandlerService-json.js:264 (Diff revision 4) > + > + // nsIHandlerService > + fillHandlerInfo(handlerInfo, overrideType) { > + let type = overrideType || handlerInfo.type; > + > + let overrideObj; Call this something like "storedHandlerInfo", or another name that is clearer than "overrideObj". ::: uriloader/exthandler/nsHandlerService-json.js:286 (Diff revision 4) > + handlerInfo.description = overrideObj.description; > + handlerInfo.preferredAction = overrideObj.action; > + > + let preferHandler = null; > + if (overrideObj.preferredHandler) { > + preferHandler = this.handlerAppFromSerializable(overrideObj.preferredHandler); nit: extra space to be removed ::: uriloader/exthandler/nsHandlerService-json.js:325 (Diff revision 4) > + */ > + handlerAppToSerializable(handler) { > + let serializable; > + if (handler instanceof Ci.nsILocalHandlerApp) { > + serializable = { > + name: handler.name, Assign the name just once, like you do in handlerAppFromSerializable. Alternatively, just "return" directly from within each "if" statement. ::: uriloader/exthandler/nsHandlerService-json.js:355 (Diff revision 4) > + * @returns {nsIHandlerApp} the handler app, if any; otherwise null > + */ > + handlerAppFromSerializable(handlerObj) { > + let handlerApp; > + if ("path" in handlerObj) { > + if (handlerObj.path) { Just noticed this, it's likely a leftover from a previous version. Because we always store the path, you don't have to check whether it's empty here. ::: uriloader/exthandler/nsHandlerService-json.js:364 (Diff revision 4) > + } > + catch(ex) { nit: on the same line, like the style you used before. ::: uriloader/exthandler/nsHandlerService-json.js:376 (Diff revision 4) > + handlerApp.service = handlerObj.service; > + handlerApp.method = handlerObj.method; > + handlerApp.objectPath = handlerObj.objectPath; > + handlerApp.dBusInterface = handlerObj.dBusInterface; nit: remove extra spaces. ::: uriloader/exthandler/nsHandlerService-json.js:429 (Diff revision 4) > + if (mimeTypes[type].fileExtensions) { > + if (mimeTypes[type].fileExtensions.includes(extension)) { Can simplify to: if (mimeTypes[type].fileExtensions && mimeTypes[type].fileExtensions.includes(extension)) ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:70 (Diff revision 4) > + let defaultHandlerTypes = ["webcal", "ircs", "mailto", "irc"]; > + while (handlerInfos.hasMoreElements()) { > + let handlerInfo = handlerInfos.getNext().QueryInterface(Ci.nsIHandlerInfo); > + do_check_neq(defaultHandlerTypes.indexOf(handlerInfo.type), -1); > + defaultHandlerTypes.splice(defaultHandlerTypes.indexOf(handlerInfo.type), 1); > + } > + do_check_eq(defaultHandlerTypes.length, 0); Now that we have Assert.deepEqual, you can rework this test to build an array of types, then check them using: Assert.deepEqual(types.sort(), ["irc", "ircs", "mailto", "webcal"]); Since you're enumerating the handlers a lot in these tests, you should write a helper function that returns an array with the data you need to check. ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:212 (Diff revision 4) > + handlerInfo2.preferredApplicationHandler = webHandler; > + handlerInfo2.possibleApplicationHandlers.appendElement(webHandler, false); There should be an explanation of what this is designed to test. At present, I don't believe that someone reading this could figure out that this is the important part. In general, there should be a comment explaining the test before each add_task call, and the test function name should be more descriptive, similar to the last test you added in the file. ::: uriloader/exthandler/tests/unit/mimeTypes.json:1 (Diff revision 4) > +{"version":{"en-US":4},"mimetypes":{"application/pdf":{"description":"PDF document","action":3,"askBeforeHandling":false,"fileExtensions":["pdf"]}},"schemes":{"webcal":{"action":1,"askBeforeHandling":true,"possibleHandlers":[{"name":"30 Boxes","uriTemplate":"https://30boxes.com/external/widget?refer=ff&url=%s"}]},"ircs":{"action":1,"askBeforeHandling":true,"fileExtensions":[],"possibleHandlers":[{"name":"Mibbit","uriTemplate":"https://www.mibbit.com/?url=%s"}]},"mailto":{"action":4,"askBeforeHandling":false,"possibleHandlers":[{"name":"Yahoo! Mail","uriTemplate":"https://compose.mail.yahoo.com/?To=%s"},{"name":"Gmail","uriTemplate":"https://mail.google.com/mail/?extsrc=mailto&url=%s"}]},"irc":{"action":1,"askBeforeHandling":true,"possibleHandlers":[{"name":"Mibbit","uriTemplate":"https://www.mibbit.com/?url=%s"}]}}} In the template files for the tests, you should set the version to some arbitrarily high number. Otherwise, a simple version change to the default handlers for the English locale would invalidate the tests that are supposed to start with just the data from the template. ::: uriloader/exthandler/tests/unit/xpcshell.ini:4 (Diff revision 4) > [DEFAULT] > head = head_handlerService.js > run-sequentially = Bug 912235 - Intermittent failures > nit: remove the extra line between run-sequentially and firefox-appdir.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review109790 ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:113 (Diff revision 4) > + do_check_false(gHandlerSvc.exists(gzipHandlerInfo)); > + > + // test "exists()" with store() and remove() > + let protocolSvc = Cc["@mozilla.org/uriloader/external-protocol-service;1"]. > + getService(Ci.nsIExternalProtocolService); > + let handler = protocolSvc.getProtocolHandlerInfo("ircs"); Here I do the test for protocol handler with store(), exist() and remove(). Will add the test for protocol with fillHandlerInfo.

(In reply to Alphan Chen [:alchen] from comment #40) > Here I do the test for protocol handler with store(), exist() and remove(). This looks more like a test for the exists() method, and the test name in fact is "testExists". Also, this test works from the imported data and not from an empty database, which means we're not really testing the normal use case, which is to use store() to register a handler for a new protocol. > Will add the test for protocol with fillHandlerInfo. In particular, we need to test that all the properties and handlers are saved correctly.

Looks much better, thanks! The test coverage has improved, and I'll start the review of the test cases next. Please be patient as this may take some time, I'm also working on other bugs. In the meantime, you can look for opportunities for adding helper functions and loops to reduce duplication in the test code. Feel free to post new versions as you have them, even if I've not finished the full review. For example, it's immediately apparent that you can use a loop for the last test, something like: for (let preferredAction of [ Ci.nsIHandlerInfo.saveToDisk, Ci.nsIHandlerInfo.alwaysAsk, ]) { // ... } There might be other cases I didn't see yet.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review111494 ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:273 (Diff revision 6) > +// Test the functionality of _IsInHandlerArray() by injecting default handler again > +add_task(function* testIsInHandlerArray() { > + yield removeImportDB(); > + > + let osDefaultHandlerFound = {}; > + let protoInfo = gExternalProtocolService.getProtocolHandlerInfoFromOS("nonexistent", > + osDefaultHandlerFound); > + do_check_eq(protoInfo.possibleApplicationHandlers.length, 0); > + do_check_false(gHandlerSvc.exists(protoInfo)); > + gHandlerSvc.fillHandlerInfo(protoInfo, "ircs"); > + do_check_eq(protoInfo.possibleApplicationHandlers.length, 1); > + > + yield updateDB(); > + yield reloadData(); > + protoInfo = gExternalProtocolService.getProtocolHandlerInfoFromOS("nonexistent", > + osDefaultHandlerFound); > + do_check_false(gHandlerSvc.exists(protoInfo)); > + gHandlerSvc.fillHandlerInfo(protoInfo, "ircs"); > + do_check_eq(protoInfo.possibleApplicationHandlers.length, 1); > +}); It's better to test the re-import by increasing the version in the preferences. The advantages are: - You don't need a special observer in the production code. This makes future refactoring easier, and you test the actual code rather than a dedicated testing path. - You're testing the versioning logic at the same time. - The small amount of additional code in this test case is compensated by having less code in production and not needing a separate updateDB helper in the testing code. Since the version is a special type of localized preference, I'm not sure whether pushPrefEnv supports it. If it doesn't, the documentation says it can be written to using Services.prefs methods directly: https://developer.mozilla.org/en-US/Add-ons/Code_snippets/Preferences#nsIPrefLocalizedString Make sure to reset the preference after the test, manually or with popPrefEnv. Before reading the documentation I was not sure if these preferences could be written to, so I also thought that an alternative solution could be to decrease the version in the data directly. This would need different code for the JSON and RDF versions, so the preference approach seems better. ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:281 (Diff revision 6) > + > + let osDefaultHandlerFound = {}; > + let protoInfo = gExternalProtocolService.getProtocolHandlerInfoFromOS("nonexistent", > + osDefaultHandlerFound); > + do_check_eq(protoInfo.possibleApplicationHandlers.length, 0); > + do_check_false(gHandlerSvc.exists(protoInfo)); There's no particular reason to test that the "nonexistent" protocol doesn't exist. Rather, remove a different protocol, like "irc", before triggering the re-import. After the re-import, check that it exists again. This way, you can be sure that the re-import actually happened, so when you later test that "ircs" has only one handler, you also know the _IsInHandlerArray code was invoked.

(In reply to :Paolo Amadini from comment #45) > I'm not sure whether pushPrefEnv supports it. I did some testing, and nsISupportsString is stored in the same way as normal character preferences in the preferences file. So, you may be able to just use pushPrefEnv with the string "1" as the value. This would make things really simple.

(In reply to :Paolo Amadini from comment #46) > with the string "1" as the value This should be a high value, of course, for example "100". I confused the direction when I wrote the comment.

(In reply to :Paolo Amadini from comment #47) > (In reply to :Paolo Amadini from comment #46) > > with the string "1" as the value > > This should be a high value, of course, for example "100". I confused the > direction when I wrote the comment. No problem, I understand that you just mean we can use string as the value. However, pushPrefEnv is a SpecialPower API which is available in mochitest. So I still use the traditional way. Will put a latest patch soon. Looking forward to seeing your review.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review114208 Thanks for the update, I believe the tests now cover 90% of the code. After the fix noted below, only a few calls remain to be tested, like fillHandlerInfo with an empty overrideType. Since the remaining 10% of the work on the test files is mostly smaller cleanup and removal of duplication, I'll post an updated changeset with these small changes. ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:302 (Diff revisions 6 - 7) > + string.data = "999"; > + Services.prefs.setComplexValue("gecko.handlerService.defaultHandlersVersion", > + Ci.nsIPrefLocalizedString, string); > + > + // remove original database and do reloading > + yield removeImportDB(); This should be reloadData instead of removeImportDB, otherwise this will test a clean re-import of the defaults instead of an update.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review115408 I've finished reviewing the tests, and with the change noted in comment 50 and the additional changes below, we can land this version. In the coming weeks, I'll likely post some smaller updates to the test coverage and the file format as mentioned before. However, these are changes that don't block the implementation of the migration between the two services in bug 1287658. ::: uriloader/exthandler/nsHandlerService-json.js:11 (Diff revision 7) > + > + nit: please avoid using two or more consecutive empty lines, only one is sufficient, throughout the whole patch. ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:17 (Diff revision 7) > +"use strict" > + > +Cu.import("resource://gre/modules/osfile.jsm"); > +Cu.import("resource://gre/modules/Task.jsm"); > + > +XPCOMUtils.defineLazyServiceGetter(this, "gMimeSvc", Global rename to gMIMEService. ::: uriloader/exthandler/tests/unit/mimeTypes.rdf:19 (Diff revision 7) > + NC:alwaysAsk="false"> > + <NC:possibleApplication RDF:resource="urn:handler:web:https://compose.mail.yahoo.com/?To=%s"/> > + <NC:possibleApplication RDF:resource="urn:handler:web:https://mail.google.com/mail/?extsrc=mailto&amp;url=%s"/> > + </RDF:Description> > + <RDF:Description RDF:about="urn:root" > + NC:en-US_defaultHandlersVersion="4" /> This should be "999" too. ::: uriloader/exthandler/tests/unit/test_handlerService_json.js:16 (Diff revision 7) > + > + > +Cu.import("resource://gre/modules/XPCOMUtils.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/NetUtil.jsm"); > +Cu.import("resource://testing-common/TestUtils.jsm"); nit: you can move the TestUtils import to the common file. There isn't a particular reason for it to be here, as you're using other modules like Task that are imported there anyways. ::: uriloader/exthandler/tests/unit/test_handlerService_json.js:19 (Diff revision 7) > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/NetUtil.jsm"); > +Cu.import("resource://testing-common/TestUtils.jsm"); > + > + > +XPCOMUtils.defineLazyServiceGetter(this, "gHandlerSvc", Global rename to gHandlerService.

(In reply to :Paolo Amadini from comment #51) > Comment on attachment 8816011 [details] > Bug 1287660 - New implementation of nsIHandlerService for the JSON backend; > > > In the coming weeks, I'll likely post some smaller updates to the test > coverage and the file format as mentioned before. > Where should I put these updates? Land the main patch first and do a revised patch after the main patch?!

(In reply to Alphan Chen [:alchen] from comment #52) > Where should I put these updates? For the changes in comment 50 and 51, just post a new version on MozReview, that will be attached to this bug. The r+ should be carried over automatically to the new version. > Land the main patch first and do a revised patch after the main patch?! Once you have posted the new version we can land it in this bug, because the interface is stable and 90% tested already, even if there are a small number of tests missing. We can do the optimizations of the file format and tests in a separate follow-up bug, but before doing that you can start to implement the migration in bug 1287658.

https://treeherder.mozilla.org/logviewer.html#?job_id=79570237&repo=try&lineNumber=2978 Look like injectNewDefaults() doesn't work well on Android even with the service of RDF backend. @test_handlerService_rdf.js [task 2017-02-23T07:54:15.411307Z] 07:54:15 WARNING - TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/unit/test_handlerService_rdf.js | testImportAndReload - [testImportAndReload : 86] [] deepEqual ["irc","ircs","mailto","webcal"] It shows that "@mozilla.org/uriloader/handler-service-json;1" is undefined on Android build. @test_handlerService_json.js [task 2017-02-23T07:54:08.054700Z] 07:54:08 WARNING - TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/unit/test_handlerService_json.js | xpcshell return code: 0 [task 2017-02-23T07:54:08.129956Z] 07:54:08 INFO - "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "@mozilla.org/uriloader/handler-service-json;1"" {file: "resource://gre/modules/XPCOMUtils.jsm" line: 230}]" [task 2017-02-23T07:54:08.130069Z] 07:54:08 INFO - <<<<<<< However, the existing tests didn't run on Android. [test_handlerService.js] support-files = mailcap # Bug 676997: test consistently fails on Android fail-if = os == "android" [test_punycodeURIs.js] # Bug 676997: test consistently fails on Android fail-if = os == "android"

(In reply to Alphan Chen [:alchen] from comment #55) > It shows that "@mozilla.org/uriloader/handler-service-json;1" is undefined > on Android build. This should be able to fix by adding nsHandlerService-json.js into mobile/android/installer/package-manifest.in.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review116668 ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:217 (Diff revision 8) > +// Verify the handling of app handler: web handler > +add_task(function* testStoreForWebHandler() { > + yield removeImportDB(); > + > + let handlerInfo = gMIMEService.getFromTypeAndExtension("nonexistent/type", null); > + do_check_eq(handlerInfo.preferredAction, Ci.nsIHandlerInfo.saveToDisk); The behavior on Android for this is also different. https://treeherder.mozilla.org/#/jobs?repo=try&revision=922b17d06c75dcf0a0e4bb743b2968f0f7fda454&selectedJob=79895351 TEST-UNEXPECTED-FAIL | uriloader/exthandler/tests/unit/test_handlerService_json.js | testStoreAndFillHandlerInfo - [testStoreAndFillHandlerInfo : 156] 2 == 0

(In reply to Alphan Chen [:alchen] from comment #55) > Look like injectNewDefaults() doesn't work well on Android even with the > service of RDF backend. > > However, the existing tests didn't run on Android. To improve our test coverage, maybe we can skip the test cases for _injectNewDefaults() on Android from code, like we do for the Linux one, and still run the other test cases? (In reply to Alphan Chen [:alchen] from comment #57) > The behavior on Android for this is also different. > > TEST-UNEXPECTED-FAIL | > uriloader/exthandler/tests/unit/test_handlerService_json.js | > testStoreAndFillHandlerInfo - [testStoreAndFillHandlerInfo : 156] 2 == 0 This value is probably not relevant, since we're actually just constructing a blank nsIHandlerInfo to use with fillHandlerInfo. Actually, I was thinking we can factor this out to a helper function: /** * Get a handler info for a MIME type that neither the application nor the OS * knows about and make sure its properties are set to the proper default * values. */ function getBlankHandlerInfo(type) { let handlerInfo = gMIMEService.getFromTypeAndExtension(type, null); do_check_eq(handlerInfo.preferredAction, Ci.nsIHandlerInfo.saveToDisk); do_check_eq(handlerInfo.possibleApplicationHandlers.length, 0); do_check_true(handlerInfo.alwaysAskBeforeHandling); return handlerInfo; } We can make the first do_check_eq conditional on the platform, or also remove it completely. You can then use getBlankHandlerInfo in all the other test cases as well. Please request review or needinfo again on the revised version, thanks!

Hi Paolo, I found a different design on Android. http://searchfox.org/mozilla-central/source/uriloader/exthandler/android/nsOSHelperAppService.cpp#54 On Android, we use mimeInfo to handle both mimeTypes and protocol. Some codes need to be refined due to this symptom.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7138d1205c5 The try server looks fine.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review120026 ::: uriloader/exthandler/nsHandlerService-json.js:157 (Diff revisions 8 - 9) > + } else if (!this.__store) { > + Services.obs.notifyObservers(null, "handlersvc-json-replace-complete", null); > + return; Seems like something to check inside _onDBChange instead. ::: uriloader/exthandler/nsHandlerService-json.js:306 (Diff revisions 8 - 9) > method: handler.method, > objectPath: handler.objectPath, > dBusInterface: handler.dBusInterface, > }; > } > return null; Add a comment here to say that we return null for the Android default application handler. ::: uriloader/exthandler/nsHandlerService.js:714 (Diff revisions 8 - 9) > + // The handler(SystemChooser) will be set in nsMIMEInfoAndroid constructor. > + // We can ignore this handler. > + if (handler.name == "Android chooser") { > + return; > + } I see that the nsHandlerService on Android would currently throw "unknown handler type" without this fix. Probably we never call "store" on Android in production code. So, this check should be done by a helper function that uses "instanceof" to check whether "handler" is none of the known sub-types. Otherwise, you could exclude the handler on Desktop if the name of the application is "Android chooser". ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:29 (Diff revisions 8 - 9) > > const pdfHandlerInfo = gMIMEService.getFromTypeAndExtension("application/pdf", null); > const gzipHandlerInfo = gMIMEService.getFromTypeAndExtension("application/x-gzip", null); > +const ircHandlerInfo = gExternalProtocolService.getProtocolHandlerInfo("irc"); > > +let onAndroid = false; You can check if Services.appinfo.widgetToolkit == "android" instead of using this variable. Otherwise, you should initialize the variable using feature detection, and not do it as a side effect of calling getBlankHandlerInfo. ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:137 (Diff revisions 8 - 9) > + // test some default protocol info properties > + try { > + // If we have a defaultHandlersVersion pref, then assume that we're in the > + // firefox tree and that we'll also have default handlers. > + let rootPrefBranch = Services.prefs.getBranch(""); > + if (rootPrefBranch.getCharPref("gecko.handlerService.defaultHandlersVersion")) { You can check Services.prefs.getPrefType("gecko.handlerService.defaultHandlersVersion") so you don't need the try-catch. You also don't need to get an empty preference branch. Also, you should move the "test reload without DB" case to a different task and add that exclusion at the beginning of the task. ::: uriloader/exthandler/tests/unit/common_test_handlerService.js:260 (Diff revisions 8 - 9) > > gHandlerService.fillHandlerInfo(handlerInfo, "nonexistent/type2"); > - let apps = handlerInfo.possibleApplicationHandlers.enumerate(); > - let app = apps.getNext().nsIHandlerApp; > + let apps = handlerInfo.possibleApplicationHandlers.enumerate(); > + let app; > + if (onAndroid) { > + app = apps.getNext().nsIHandlerApp; ".QueryInterface(Ci.nsIHandlerApp)" or ".QueryInterface(Ci.nsIWebHandlerApp)" should be there instead of ".nsIHandlerApp". I wasn't familiar with the latter syntax anyways. ::: uriloader/exthandler/tests/unit/xpcshell.ini:17 (Diff revisions 8 - 9) > # Bug 676997: test consistently fails on Android > fail-if = os == "android" > [test_handlerService_json.js] > support-files = handlers.json > [test_handlerService_rdf.js] > -support-files = mimeTypes.rdf > +support-files = handlers.rdf I think "mimeTypes.rdf" was clearer since it resembled the name we use in actual profiles.

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review120026 > I think "mimeTypes.rdf" was clearer since it resembled the name we use in actual profiles. If we use mimeTypes.rdf, it will be removed when running the test on Android by "function HandlerServiceTest_init()". So we should use another name to avoid the delete. (maybe TestMimeType.rdf)

(In reply to Alphan Chen [:alchen] from comment #63) > > I think "mimeTypes.rdf" was clearer since it resembled the name we use in actual profiles. > > If we use mimeTypes.rdf, it will be removed when running the test on Android > by "function HandlerServiceTest_init()". This function uses this._dirSvc.get("UMimTyp", Ci.nsIFile) which returns an absolute path to "mimeTypes.rdf" in the *profile* folder, which should be different from the absolute path of the file in the *test* folder... unless for some reason the Android test infrastructure installs test files in the profile folder or initializes the profile in the test folder. Are you saying that the do_get_file("mimeTypes.rdf") call returns the same absolute path as the directory service? If this is the case, you should file a bug for the Android test infrastructure, because it can create problems for other things as well, and most notably for "handlers.json" in our case. Out of curiosity, what is the actual absolute path returned by these functions in your Android test runs? If you need to use a different file name, then call the two files "mimeTypes_v1.rdf" and "handlers_v1.json". It's in line with what we're doing for "places.sqlite": https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/migration

Comment on attachment 8816011 [details] Bug 1287660 - New implementation of nsIHandlerService for the JSON backend. https://reviewboard.mozilla.org/r/96784/#review120448 Looks good after the naming issue in comment 65 is sorted out. Thanks for the fast update!

(In reply to :Paolo Amadini from comment #65) > (In reply to Alphan Chen [:alchen] from comment #63) > > > I think "mimeTypes.rdf" was clearer since it resembled the name we use in actual profiles. > > > > If we use mimeTypes.rdf, it will be removed when running the test on Android > > by "function HandlerServiceTest_init()". > > This function uses this._dirSvc.get("UMimTyp", Ci.nsIFile) which returns an > absolute path to "mimeTypes.rdf" in the *profile* folder, which should be Actually not, head_handlerService.js mocks up the function and return Current working directory. http://searchfox.org/mozilla-central/source/uriloader/exthandler/tests/unit/head_handlerService.js#89 > > Are you saying that the do_get_file("mimeTypes.rdf") call returns the same > absolute path as the directory service? If this is the case, you should file > a bug for the Android test infrastructure, because it can create problems > for other things as well, and most notably for "handlers.json" in our case. > > Out of curiosity, what is the actual absolute path returned by these > functions in your Android test runs? > /storage/sdcard/tests/xpc/uriloader/exthandler/tests/unit/mimeTypes.rdf

(In reply to Alphan Chen [:alchen] from comment #67) > (In reply to :Paolo Amadini from comment #65) > > > > > Are you saying that the do_get_file("mimeTypes.rdf") call returns the same > > absolute path as the directory service? If this is the case, you should file > > a bug for the Android test infrastructure, because it can create problems > > for other things as well, and most notably for "handlers.json" in our case. > > > > Out of curiosity, what is the actual absolute path returned by these > > functions in your Android test runs? > > > /storage/sdcard/tests/xpc/uriloader/exthandler/tests/unit/mimeTypes.rdf They are in different folder in other cases. For example : on Ubuntu (**test* folder) obj-x86_64-pc-linux-gnu/_tests/xpcshell/uriloader/exthandler/tests/unit/TestMimeType.rdf (Current working directory) obj-x86_64-pc-linux-gnu/dist/bin/browser/mimeTypes.rdf

(In reply to Alphan Chen [:alchen] from comment #67) > (In reply to :Paolo Amadini from comment #65) > > This function uses this._dirSvc.get("UMimTyp", Ci.nsIFile) which returns an > > absolute path to "mimeTypes.rdf" in the *profile* folder, which should be > Actually not, head_handlerService.js mocks up the function and return > Current working directory. > http://searchfox.org/mozilla-central/source/uriloader/exthandler/tests/unit/ > head_handlerService.js#89 > I think we can replace the folder("CurProcD") to "ProfD". Then we can still use mimeTypes.rdf. What do you think? The following are the results. (Ubuntu) (**test* folder) obj-x86_64-pc-linux-gnu/_tests/xpcshell/uriloader/exthandler/tests/mimeTypes.rdf (The profile directory) /tmp/firefox/xpcshellprofile/mimeTypes.rdf (Android) (**test* folder) /storage/sdcard/tests/xpc/uriloader/exthandler/tests/unit/mimeTypes.rdf (The profile directory) /storage/sdcard/tests/xpc/p/mimeTypes.rdf

(In reply to Alphan Chen [:alchen] from comment #69) > > I think we can replace the folder("CurProcD") to "ProfD". > Then we can still use mimeTypes.rdf. > What do you think? > There should be no side-effect to do this change. I've already tried it on several platforms. Will put the latest patch and trigger a try.

(In reply to Alphan Chen [:alchen] from comment #69) > I think we can replace the folder("CurProcD") to "ProfD". Ah, that explains what's going on, thanks! Yes, that sounds good, the original test shouldn't have used the current directory. I think the mock directory might have been implemented in ancient times, before xpcshell had the "do_get_profile" capability. The entire code doing the mock directory may be unnecessary now, if we call "do_get_profile" in the head file for all tests in the folder. You can try this now, or file follow-up bug for it. Thanks!

(In reply to :Paolo Amadini from comment #72) > > I think the mock directory might have been implemented in ancient times, > before xpcshell had the "do_get_profile" capability. The entire code doing > the mock directory may be unnecessary now, if we call "do_get_profile" in > the head file for all tests in the folder. You can try this now, or file > follow-up bug for it. Thanks! Follow-up bug (Bug 1346716) is opened.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58b5599956140a61f5895150c8ed821f0c7d3580 The result of try server looks fine.

One more thing, we need to modify our test case if we remove the possible handler(30boxes) of webcal in bug 1252831.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/4548e84389d1 New implementation of nsIHandlerService for the JSON backend. r=Paolo