Also We should do the conversion automatically for the smooth transition(from mimeType.rdf to mimeType.json).

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

Since helperApp.js is the only consumer to access mimeType.rdf directly, we will investigate it in bug 1287661. Based on the result of bug 1287661, we will revise helperApp.js and(/or) enhance nsIHandlerService API to let consumer use the same way to access the database.

According to the investigation result on bug 1287661, removing HelperApps.js from nsHelperAppDlg.js by using nsIHandlerService::Store(). And here is the try result https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd6e011fc980

Hi Eden, I'm still looking at the patch since there's a lot of code removed and it takes time to go through it to see if there is something lost in the conversion. In the meantime, can you describe the existing test coverage for the code we are touching, in other words which tests would break if you just remove the call to "store" in updateHelperAppPref? If there is no test or the existing test does not check everything we need, we'll need to write a new test or update the existing one.

Comment on attachment 8796728 [details] [diff] [review] Bug1287664_Replace_HelperApps.js_by_nsIHandlerService. r?Paolo Clearing the review request while waiting for the testing situation to be sorted out. I couldn't find any obvious test that checks that the dialog saves changes, but maybe they're located somewhere non-obvious. You can easily verify by removing the code and running all the test suites.

(In reply to :Paolo Amadini from comment #7) > Comment on attachment 8796728 [details] [diff] [review] > Bug1287664_Replace_HelperApps.js_by_nsIHandlerService. r?Paolo > > Clearing the review request while waiting for the testing situation to be > sorted out. I couldn't find any obvious test that checks that the dialog > saves changes, but maybe they're located somewhere non-obvious. You can > easily verify by removing the code and running all the test suites. Sorry for late response. I thought the testcase "uriloder/exthandler/tests/mochitest/browser_download_always_ask_preferred_app.js" is to check the related functions. But I found that it isn't, it just checks the the value on the dialog. I will write a test for this case.

Remove useless module HelperApps.js by using nsIHandlerService. I create a new test browser_remember_download_option.js under uriloader/exthandler/tests/mochitest/ to make sure the code path are tested.

That's great. Removing unneeded code is always good! I look forward to getting to this review next week.

Comment on attachment 8800394 [details] [diff] [review] Bug1287664_Replace_HelperApps.js_by_using_nsIHandlerService. r?Paolo Hi Eden, sorry for the delay on this review, I'm also busy with other front-end reviews and development. I've been looking at this during the past few days and the preliminary testing I did shows that everything works correctly. For the regression test, I'll provide a few specific code suggestions later. With regard to its structure, you have one of three options to remove code duplication, most preferred first: 1. Keep two test files, but move the shared mock objects to a "head.js" file that you can use from both tests. 2. Keep two totally separate test files like now, in which case you have to remove all unnecessary code from the mock objects, and you don't need to test again the same things as the other test. 3. Just add checks to the existing test file, and rename it accordingly. In all cases, please use "hg copy" when most of the code comes from an existing file.

Comment on attachment 8800394 [details] [diff] [review] Bug1287664_Replace_HelperApps.js_by_using_nsIHandlerService. r?Paolo Review of attachment 8800394 [details] [diff] [review]: ----------------------------------------------------------------- Here are a few coding notes. I'll take another look after the test is rewritten to reduce code duplication using one of the methods above. ::: uriloader/exthandler/tests/mochitest/browser_remember_download_option.js @@ +14,5 @@ > + mockedExecutable.initWithPath("/bin/cat"); > + } else { > + mockedExecutable.initWithPath( > + "C:\\ProgramFiles\\Windows Journal\\Journal.exe"); > + } Hard-coding file paths is an anti-pattern. Since I don't think it matters that the application really exists, you can just get a reference to a non-existing temporary file using this function from "FileUtils.jsm": FileUtils.getFile("TmpD", ["executable"]); Alternatively you can do something similar to what setupFakeHandler() does in "browser/components/preferences/in-content/tests/browser_change_app_handler.js". @@ +23,5 @@ > + mockedHandlerApp.executable = mockedExecutable; > + mockedHandlerApp.detailedDescription = "Mocked handler app."; > + > + // Mock the mime info > + let mockedMIME = gMimeSvc.getFromTypeAndExtension("text/x-test-handler", null); You should use registerCleanupFunction so this type is removed at the end of the test, similarly to what the test I mentioned does. @@ +89,5 @@ > + ok(!doc.getElementById("rememberChoice").checked, > + "Remember choice checkbox should be not checked."); > + doc.getElementById("rememberChoice").checked = true; > + ok(doc.getElementById("rememberChoice").checked, > + "Remember choice checkbox Should be checked."); There's no need to double-check doc.getElementById("rememberChoice").checked, you just set it and we can assume that test code works. @@ +94,5 @@ > + > + ok(!gHandlerSvc.exists(mockedMIME), "Should not be in nsIHandlerService."); > + > + // Make the ok button be enabled. > + doc.documentElement.getButton("accept").disabled = false; Add a comment on why this is needed, alternatively use BrowserTestUtils.waitForCondition() until the button is enabled. Also, you probably still need to wait for the window to be closed. @@ +100,5 @@ > + > + ok(gHandlerSvc.exists(mockedMIME), "Should be in nsIHandlerService."); > + > + // Check the saved handler information > + // Maybe use nsIRDFService to access mimeTypes.rdf for checking. I think it's fine, this test can just do what it does now, checking that the data is stored using the handler service and the information on the handler is changed.

Hello Paolo For the regression test structure, I take the first option 1. Keep two test files, but move the shared mock objects to a "head.js" file that you can use from both tests. I create head.js and move mock objects into head.js, such that both tests file can use it. And the attached patch is modified according to your suggestion in comment 12. Thanks.

The try result is of the patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cc4b60453ba&selectedJob=29730134

Eden, sorry for the delay, I've now finished the review of the code in the removed file and I have a few questions. The main difference you identified in bug 1287661 comment 2 is that nsIHandlerService does not save the list of associated file extensions. In bug 1287661 comment 3 you said that you tested manually and there is no difference in mimeTypes.rdf between using HelperApps.js and nsIHandlerService::Store, but my code review shows that surely there is a difference, because the extensions are not saved. Are you saying that the list of extensions in mimeTypes.rdf is not important at all, and whether we save them or not makes no difference? There are methods in nsIHandlerService that access the file extensions for reading. Does the browser behave in the exact same way if we don't store any extension in the mimeTypes.rdf file? A similar difference is that HelperApps.js does not touch the list of possible handler applications that are not the default handler. nsIHandlerService, instead, replaces the list using the information in the object, and deletes any handler that is not in the object. Is this something we should worry about, possibly deleting handlers that the user has previously set? Also, nsIHandlerService does not store the "editable" property. I think that property is irrelevant and is ignored everywhere, is this correct? There is also another minor difference in how some RDF assertions are set to point to "false" by HelperApps.js instead of being removed, but when reading they are treated in the same way, so we can probably ignore this difference. I'll review the test code next.

(In reply to :Paolo Amadini from comment #16) > Eden, sorry for the delay, I've now finished the review of the code in the > removed file and I have a few questions. > > The main difference you identified in bug 1287661 comment 2 is that > nsIHandlerService does not save the list of associated file extensions. In > bug 1287661 comment 3 you said that you tested manually and there is no > difference in mimeTypes.rdf between using HelperApps.js and > nsIHandlerService::Store, but my code review shows that surely there is a > difference, because the extensions are not saved. > > Are you saying that the list of extensions in mimeTypes.rdf is not important > at all, and whether we save them or not makes no difference? > > There are methods in nsIHandlerService that access the file extensions for > reading. Does the browser behave in the exact same way if we don't store any > extension in the mimeTypes.rdf file? Yes, I think the browser behaviors in the exact same way in general. extension attribute is used in nsIHandlerService::getTypeFromExtension() and nsIHandlerService::FillHandlerInfo(), and Both they are used in nsExternalHelperAppService::GetFromTypeAndExtension() https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2613 According to the flow, GetTypeFromExtension() and FillHandlerInfo() are called while the extension maps to different types in mimeTypes.rdf and the one browser gets from others(https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2729). However, it means one extension maps two different mime types, and it would makes browser confuse which type should be used for content handling when next time browser handle the same file extension link, and it makes user feel strange. Please see bug 306471. Why the situation happens, because website provides wrong content-type and extension mapping, and we saved it through "HelperApps.js". Patches for bug 306471 is avoiding loading type from mimeTypes.rdf by extension, but the correct way should be don't save extension in mimeTypes.rdf, and HelperApps.js is the only one module saving extension in mimeTypes.rdf. > A similar difference is that HelperApps.js does not touch the list of > possible handler applications that are not the default handler. > nsIHandlerService, instead, replaces the list using the information in the > object, and deletes any handler that is not in the object. Is this something > we should worry about, possibly deleting handlers that the user has > previously set? I think this should not be an issue. According to the loading flow, the modified nsIHandlerInfo(or nsIMIMEInfo) should be the same with the one browser getting from nsIHandlerService(or nsIMIMEService). So the value of possibleApplications should the same with the value in mimeTypes.rdf. HelperApps.js works without bugs because it is only used when the attribute preferredApplication is modified. So, although nsIHandlerService::store() saves the attribute possibleApplications, it just saves the same list back to mimeTypes.rdf. So it shouldn't be an issue. > > Also, nsIHandlerService does not store the "editable" property. I think that > property is irrelevant and is ignored everywhere, is this correct? > Yes, the attribute is irrelevant and ignored everywhere. > There is also another minor difference in how some RDF assertions are set to > point to "false" by HelperApps.js instead of being removed, but when reading > they are treated in the same way, so we can probably ignore this difference. > Yes, the value of attributes are the same with the default reading behavior. > I'll review the test code next. Thanks.

Thanks for the pointers. I've read through the code you pointed to, and I think the changes we're making here mainly affect the following logic, which is still in effect even after bug 306471 landed: https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2642-2674 In particular, the first time we encounter a MIME type and file extension combination that neither us nor the operating system know about, we ask the user what to do, and we save that extension-to-type association in mimeTypes.rdf, together with the preferences like "always ask". Later, if we encounter a different MIME type associated to the same extension, and again the operating system doesn't know about them, we look up the file extension that we saved previously in mimeTypes.rdf, and we retrieve the preferred application and the preferences from before. Then we save a duplicate mimeTypes.rdf entry associated to the new MIME type. In doing this, you're right that we duplicate the extension list, so if we find a third MIME type associated to the same extension, we'll choose one of the two existing entries arbitrarily and copy it again. Also, we don't propagate the possible handlers during the copy, but it's likely that there weren't any to begin with. If we stop saving the extensions in mimeTypes.rdf at all, we break this mechanism. Arguably, we shouldn't create a duplicate mimeTypes.rdf entry in the first place, and just edit the original entry, so this logic isn't perfect to begin with. Disposing of this old mechanism might be an improvement rather than a regression, but we need to think about the consequences explicitly. Does it make sense?

Gijs, do you have any comment or previous experiences about the situation described in comment 18? We're trying to understand whether we should continue to save file extensions in mimeTypes.rdf, because the patch in this bug currently gets rid of this functionality as a side-effect of removing handlerApps.js.

(In reply to :Paolo Amadini from comment #19) > Gijs, do you have any comment or previous experiences about the situation > described in comment 18? > > We're trying to understand whether we should continue to save file > extensions in mimeTypes.rdf, because the patch in this bug currently gets > rid of this functionality as a side-effect of removing handlerApps.js. It's not clear to me from comment #18 which functionality goes away. When we encounter the second extension that maps to the same mime-type for which we have a handler configured, after the patch is applied, what happens? Do we use the user's choice for extension A for extension B as well assuming they map to the same mime type, and just not store an entry for extension B? Or do we prompt the user? The first would be a behaviour change that would need to be matched up with the spec we're trying to implement. If we made a decision to get rid of the functionality, I guess that's what we should do (but didn't we say we would put this behind a pref?), and otherwise I guess we should update the patch so that it reflects the spec.

(In reply to :Gijs Kruitbosch from comment #20) > It's not clear to me from comment #18 which functionality goes away. When we > encounter the second extension that maps to the same mime-type for which we > have a handler configured, after the patch is applied, what happens? Do we > use the user's choice for extension A for extension B as well assuming they > map to the same mime type, and just not store an entry for extension B? Or > do we prompt the user? Did you reverse MIME type and extension in this example, or are you talking about a different case? > The first would be a behaviour change that would need to be matched up with > the spec we're trying to implement. If we made a decision to get rid of the > functionality, I guess that's what we should do (but didn't we say we would > put this behind a pref?), and otherwise I guess we should update the patch > so that it reflects the spec. This issue came up independently from the specification, similarly to the work we did in bug 306471. There is no mention of the extension-to-type mapping logic in the specification.

Talking to Paolo on IRC, it feels like we should ensure that we keep the mimetype-to-extension mapping for now, to avoid regressing the case where servers are inconsistent about mimetypes for an extension, and for the user to be confused why their choice about what to do about e.g. pdf/doc/txt/csv files is not remembered. If we're removing helperApps.js that boils down to having to reimplement that aspect of it elsewhere and ensuring we have tests that the behaviour is correct.

Comment on attachment 8803709 [details] [diff] [review] Bug1287664_Replacing_useless_module_HelperApps.js_by_using_nsIHandlerService. r?Paolo Clearing review because for now we need to keep saving the extensions. Eden, in order to emulate the case from comment 18 we would need a more complex test that runs two downloads and interacts with the dialog, we cannot just use the mock objects. This may be particularly complicated to write, thus I suggest you file a separate bug for it. In this bug, we can start from the two unit tests we already have, and simply verify that we saved the extensions too. I've taken a closer look at the test code, and I've noticed that there is actually a lot of unused code in the original test, for example _saveCount and other properties. We should get rid of all the unused code and write a test that is as short as possible. Do we really need two objects for internalMockedMIME and mockedMIME? Cannot the dialog just work with the original nsIMIMEInfo we have in internalMockedMIME? Do we need to create an empty executable file? Is there code that fails if we don't create the file? Do we need two different values for suggestedFileName and other properties like the file extensions between the two tests? Can we just set the same default values in head.js and avoid changing them? There is still some similar code between the two tests for opening the dialog and waiting, it can likely be moved to helper functions in head.js. You can use "Task.async" or "function*" for asynchronous helpers. In general I recommend writing helper functions rather than active code in head.js, so tests that don't need the initialization do not have any overhead. For example, browser_web_protocol_handlers.js can work without the nsIMIMEInfo.getFromTypeAndExtension call in head.js.

(In reply to :Paolo Amadini from comment #23) > Comment on attachment 8803709 [details] [diff] [review] > Bug1287664_Replacing_useless_module_HelperApps.js_by_using_nsIHandlerService. > r?Paolo > > Clearing review because for now we need to keep saving the extensions. > > Eden, in order to emulate the case from comment 18 we would need a more > complex test that runs two downloads and interacts with the dialog, we > cannot just use the mock objects. This may be particularly complicated to > write, thus I suggest you file a separate bug for it. In this bug, we can > start from the two unit tests we already have, and simply verify that we > saved the extensions too. For now, I will keep saving the extension in nsIHandlerService::Store(). And for the case in comment 18, I will create a bug for it. I will try to create a test to simulate the case, and we can discuss the solution on it. > > I've taken a closer look at the test code, and I've noticed that there is > actually a lot of unused code in the original test, for example _saveCount > and other properties. We should get rid of all the unused code and write a > test that is as short as possible. > > Do we really need two objects for internalMockedMIME and mockedMIME? Cannot > the dialog just work with the original nsIMIMEInfo we have in > internalMockedMIME? > I did this because the original test needs make sure the property "hasDefaultHandler" to be true, otherwise it fails on the check ok(!dlg.document.getElementById("openHandler").selectedItem.hidden, "Should not have selected a hidden item."); because the property "hasDefaultHandler" is a read only attribute, I can not modify it directly, so I use mockedMIME to wrap the "internalMockedMIME". > Do we need to create an empty executable file? Is there code that fails if > we don't create the file? Creating file makes sure the file really exists, because nsIHanderService::Store() checks the file existence. If the file doesn't exist, it makes the nsIHandlerService::Store() fail. I create an empty file and delete it while the test finish, because I don't want to hard code the file path for different platform. > > Do we need two different values for suggestedFileName and other properties > like the file extensions between the two tests? Can we just set the same > default values in head.js and avoid changing them? > I think we can make these properties be the same in two tests. I modify these properties because I want to make sure the original test has the same behavior. > There is still some similar code between the two tests for opening the > dialog and waiting, it can likely be moved to helper functions in head.js. > You can use "Task.async" or "function*" for asynchronous helpers. > > In general I recommend writing helper functions rather than active code in > head.js, so tests that don't need the initialization do not have any > overhead. For example, browser_web_protocol_handlers.js can work without the > nsIMIMEInfo.getFromTypeAndExtension call in head.js. That's a good advice, thanks. I will write helper functions in the next patch.

(In reply to Eden Chuang[:edenchuang] from comment #24) > > Do we really need two objects for internalMockedMIME and mockedMIME? Cannot > > the dialog just work with the original nsIMIMEInfo we have in > > internalMockedMIME? > > > > I did this because the original test needs make sure the property > "hasDefaultHandler" to be true, otherwise it fails on the check > > ok(!dlg.document.getElementById("openHandler").selectedItem.hidden, "Should > not have selected a hidden item."); > > because the property "hasDefaultHandler" is a read only attribute, I can not > modify it directly, so I use mockedMIME to wrap the "internalMockedMIME". Ok, thanks! I wonder if a Proxy could work here? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy#Value_correction_and_an_extra_property There may be complications related to XPCOM, and you may need to implement QueryInterface. If a Proxy doesn't work then the current solution is fine, even if a little verbose. > Creating file makes sure the file really exists, because > nsIHanderService::Store() checks the file existence. If the file doesn't > exist, it makes the nsIHandlerService::Store() fail. Ah, this might be a separate bug by itself if the result is that user preferences are not saved when the executable does not exist, but we don't need to fix it here.

Hello Paolo I tried to use the proxy you suggested, it still can not modify the read only attribute of nsIHandlerInfo. Of course, we can fake the attribute in the proxy, but I think it is too complicate for achieve this small improvement. So I don't use the proxy in this patch. If you think it is necessary, I will write a proxy for it.

(In reply to Eden Chuang[:edenchuang] from comment #26) > Of course, we can fake the attribute in the proxy That's exactly how the proxy should be used. Setting properties on the Proxy object would just forward them to the setter on the target and wouldn't work. I've now tested this, and it works without any XPCOM issues: let mockedMIME = new Proxy(internalMockedMIME, { get: function (target, property) { switch (property) { case "hasDefaultHandler": return true; case "defaultDescription": return "Default description"; default: return target[property]; } }, });

Comment on attachment 8808029 [details] [diff] [review] Bug1287664_Replacing_useless_module_HelperApps.js_by_using_nsIHandlerService. r?Paolo Review of attachment 8808029 [details] [diff] [review]: ----------------------------------------------------------------- The production code looks good! Just like HelperApps.js does, we only add associated extensions, and never remove them once added. We'll change this behavior later for the JSON back-end. The test code needs the following change, that I've found while testing the Proxy. ::: uriloader/exthandler/tests/mochitest/head.js @@ +22,5 @@ > + internalMockedMIME.description = mockedHandlerApp.detailedDescription; > + internalMockedMIME.alwaysAskBeforeHandling = true; > + internalMockedMIME.preferredAction = Ci.nsIHandlerInfo.useHelperApp; > + internalMockedMIME.possibleApplicationHandlers.appendElement(mockedHandlerApp, false); > + internalMockedMIME.preferredApplicationHandler = mockedHandlerApp; Both possibleApplicationHandlers and preferredApplicationHandler should only be set in browser_remember_download_option.js, and not in browser_download_always_ask_preferred_app.js, so the latter continues to test what it's supposed to. You can actually move the setup for mockedHandlerApp to a different helper function, so the executable is only created for browser_remember_download_option.js. @@ +88,5 @@ > + > + return mockedLauncher; > +} > + > +var dlg = {}; This shouldn't be a global. You can return the reference to the dialog from the openHelperAppDialog async function. @@ +114,5 @@ > + // remove the mocked mime info from database. > + let mockHandlerInfo = gMimeSvc.getFromTypeAndExtension("text/x-test-handler", null); > + if (gHandlerSvc.exists(mockHandlerInfo)) { > + gHandlerSvc.remove(mockHandlerInfo); > + } You can call registerCleanupFunction from within helper functions. In this way, the code that does the cleanup is near to the code that does the setup, and is only called if the setup was really done for that test.

Hello Paolo According to your advices in comment 28, I attached the patch for review. Thanks.

Comment on attachment 8808446 [details] [diff] [review] Bug1287664_Replacing_useless_module_HelperApps.js_by_using_nsIHandlerService. r?Paolo Review of attachment 8808446 [details] [diff] [review]: ----------------------------------------------------------------- Congratulations for getting the first bug of the RDF replacement project done, and it's not a simple one either! ::: uriloader/exthandler/tests/mochitest/head.js @@ +8,5 @@ > + // Mock the executable > + let mockedExecutable = FileUtils.getFile("TmpD", ["mockedExecutable"]); > + if (!mockedExecutable.exists()) { > + mockedExecutable.create(Ci.nsIFile.NORMAL_FILE_TYPE, 0o755); > + } As a last note, you can call registerCleanupFunction multiple times in the same test file, so it makes sense to do the cleanup for the executable separately here. You don't even need to define mockedExecutable again inside the cleanup function, you can just use the value you already have.

Attaching the improved patch according to Paolo's advices in comment 29. Thanks. I will set checkinneeded once the patch passes the try server run and no side-effect.

[Feature/regressing bug #]: Bug 1287664 [User impact if declined]: No impact. [Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b3a89b70ad8352862d662daa28e3fbe728d931d&selectedJob=30712313 [Risks and why]: No risk, just removing the useless module HelperApps.js. [String/UUID change made/needed]: No

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb7b525fe7e7 Replacing useless module HelperApps.js by using nsIHandlerService. r=Paolo