FX does this also for the de-XBL. When we do this now, this should then work also in the future.

Part 1 - Don't persist the last selected handler

Part 1 is a port of https://hg.mozilla.org/mozilla-central/rev/33e9a37ae5b4

Part 2 - Define services using defineLazyServiceGetters Port of https://hg.mozilla.org/mozilla-central/rev/8834a397305b

Part 3 - Use class syntax for the HandlerInfoWrapper hierarchy Port of https://hg.mozilla.org/mozilla-central/rev/a2dd752c009e The FX part 3 is stringbundle changes we don't need to port.

Part 4 - Move _describeType to HandlerInfoWrapper. Port of https://hg.mozilla.org/mozilla-central/rev/93bda045ca0b Aceman, I get TypeError: this._visibleTypeDescriptionCount is undefined applications.js:1119. What do I need to do to fix this?

I have the other patches ready but first part 4 needs to be correct before I can work on the following patches.

The part 4 patch removes ._visibleTypeDescriptionCount, but did you remove the reference at https://dxr.mozilla.org/comm-central/rev/28fb09316488db06ca1f606429a75c29d0bd856d/mail/components/preferences/applications.js#1138 ? It is not seen in the patch.

FX doesn't have the description (for example: (application/pdf: .pdf) ). This is what broke. I managed this now.

Part 6 - Move _describePreferredAction to HandlerInfoWrapper. Port of https://hg.mozilla.org/mozilla-central/rev/3e98dcc160f8

Part 6 - Move action icon getters to HandlerInfoWrapper. Port of https://hg.mozilla.org/mozilla-central/rev/b29b502c0fce

Part 7 - Add a HandlerListItem class Port of https://hg.mozilla.org/mozilla-central/rev/e9d78f354b26 This one doesn't work because of this lines: + this.node.setAttribute("shortTypeDetails", + this._typeDetails(visibleType)); This is to add the description FX doesn't have. Aceman, please can you help me to move _typeDetails() into the handlerInfoWrapper? I don't know how this should be done.

Part 7 fixed after talk over IRC.

Updated to tip.

Updated to tip.

Updated to tip.

Updated to tip.

I had to update all patches because of the linting in this directory.

Aceman, please can you check the linting too?

Comment on attachment 9011184 [details] [diff] [review] handler_part3.patch Review of attachment 9011184 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/preferences/applications.js @@ +98,5 @@ > > getNext() { > return this._contents[this._index++]; > }, > +} Why this change? I think eslint adds some of these trailing commas and semicolons. So they should be OK. Also, it seems this ArrayEnumerator is unused and can be removed. @@ +140,5 @@ > + // and user-configured records, stop using this boolean flag and simply > + // check for the presence of a user-configured record to determine whether > + // or not this type is only handled by a plugin. Filed as bug 395142. > + this.handledOnlyByPlugin = false; > + } So in a class commas are not needed here? Yes it works and adding a comma throws a syntax error. Weird. @@ +178,2 @@ > return; > /* eslint-enable mozilla/use-includes-instead-of-indexOf */ This comment may need removal when you removed the starting pair of it. @@ +191,5 @@ > } > > + var handlers = this.possibleApplicationHandlers; > + for (var i = 0; i < handlers.length; ++i) { > + var handler = handlers.queryElementAt(i, Ci.nsIHandlerApp); You could use 'let's here. But if all this code is copied from m-c then deviations may be unwanted and you do not need to do them.

(In reply to Richard Marti (:Paenglab) from comment #24) > Aceman, please can you check the linting too? Linter produced some errors: TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/preferences/applications.js:104:2 | Missing semicolon. (semi) TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/preferences/applications.js:241:29 | 'InternalHandlerInfoWrapper' is not defined. (no-undef) TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/preferences/applications.js:282:13 | 'isFeedType' is not defined. (no-undef) TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/preferences/applications.js:284:36 | 'InternalHandlerInfoWrapper' is not defined. (no-undef) TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/preferences/applications.js:542:2 | Unnecessary semicolon. (no-extra-semi) TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/comm/mail/components/preferences/applications.js:1130:50 | 'mimeType' is not defined. (no-undef) (https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=19b0a6b0e58871a9cad0c2377513572d72e987ec) Seems the semicolon on ArrayEnumerator is required.

Comment on attachment 9011185 [details] [diff] [review] handler_part4.patch Review of attachment 9011185 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/preferences/applicationManager.js @@ +13,5 @@ > > var bundle = document.getElementById("appManagerBundle"); > gApplicationsPane._prefsBundle = document.getElementById("bundlePreferences"); > > + var description = gApplicationsPane.handlerInfo.typeDescription; Where is this 'handlerInfo' defined in gApplicationsPane ?

Comment on attachment 9011187 [details] [diff] [review] handler_part6.patch Review of attachment 9011187 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/preferences/applications.js @@ +238,5 @@ > + case Ci.nsIHandlerInfo.saveToDisk: > + return "save"; > + > + case Ci.nsIHandlerInfo.handleInternally: > + if (isFeedType(this.type)) { isFeedType is flagged as undefined by eslint. Do we import it from mozilla/browser/components/preferences/in-content/main.js in any way? Seamonkey has its own copy of it.

(In reply to :aceman from comment #25) > Comment on attachment 9011184 [details] [diff] [review] > handler_part3.patch > > Review of attachment 9011184 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mail/components/preferences/applications.js > @@ +98,5 @@ > > > > getNext() { > > return this._contents[this._index++]; > > }, > > +} > > Why this change? I think eslint adds some of these trailing commas and > semicolons. So they should be OK. Also, it seems this ArrayEnumerator is > unused and can be removed. I removed the ArrayEnumerator. And eslint is happy now. :-) > @@ +140,5 @@ > > + // and user-configured records, stop using this boolean flag and simply > > + // check for the presence of a user-configured record to determine whether > > + // or not this type is only handled by a plugin. Filed as bug 395142. > > + this.handledOnlyByPlugin = false; > > + } > > So in a class commas are not needed here? Yes it works and adding a comma > throws a syntax error. Weird. eslint does not show a error. I leave it as it is. > @@ +178,2 @@ > > return; > > /* eslint-enable mozilla/use-includes-instead-of-indexOf */ > > This comment may need removal when you removed the starting pair of it. Removed. > @@ +191,5 @@ > > } > > > > + var handlers = this.possibleApplicationHandlers; > > + for (var i = 0; i < handlers.length; ++i) { > > + var handler = handlers.queryElementAt(i, Ci.nsIHandlerApp); > > You could use 'let's here. But if all this code is copied from m-c then > deviations may be unwanted and you do not need to do them. Copied from m-c. I'll leave them.

(In reply to :aceman from comment #27) > Comment on attachment 9011185 [details] [diff] [review] > handler_part4.patch > > Review of attachment 9011185 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mail/components/preferences/applicationManager.js > @@ +13,5 @@ > > > > var bundle = document.getElementById("appManagerBundle"); > > gApplicationsPane._prefsBundle = document.getElementById("bundlePreferences"); > > > > + var description = gApplicationsPane.handlerInfo.typeDescription; > > Where is this 'handlerInfo' defined in gApplicationsPane ? It's in applications.js line 989.

(In reply to :aceman from comment #28) > Comment on attachment 9011187 [details] [diff] [review] > handler_part6.patch > > Review of attachment 9011187 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mail/components/preferences/applications.js > @@ +238,5 @@ > > + case Ci.nsIHandlerInfo.saveToDisk: > > + return "save"; > > + > > + case Ci.nsIHandlerInfo.handleInternally: > > + if (isFeedType(this.type)) { > > isFeedType is flagged as undefined by eslint. Do we import it from > mozilla/browser/components/preferences/in-content/main.js in any way? > Seamonkey has its own copy of it. This slipped in when copied from m-c. Removed. I also added the missing InternalHandlerInfoWrapper class. Eslint is happy now.

Part_7 needed a rebase to apply.

After talk with aceman over IRC he found that applicationManager.js needed some changes to work correctly.

Comment on attachment 9012971 [details] [diff] [review] handler_part4.patch Review of attachment 9012971 [details] [diff] [review]: ----------------------------------------------------------------- Thanks

Comment on attachment 9012795 [details] [diff] [review] handler_part7.patch Review of attachment 9012795 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the series. Hopefully it future-proofs the code, as it contains a ton of new JS constructs that are not much used in c-c yet.

Comment on attachment 9011183 [details] [diff] [review] handler_part2.patch Review of attachment 9011183 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/preferences/applications.js @@ +966,5 @@ > if (type in this._handledTypes) > handlerInfoWrapper = this._handledTypes[type]; > else { > let wrappedHandlerInfo = > + gMIMEService.getFromTypeAndExtension(mimeType.type, null); where does mimeType come from?

(In reply to Jorg K (GMT+2) from comment #36) > Comment on attachment 9011183 [details] [diff] [review] > handler_part2.patch > > Review of attachment 9011183 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: mail/components/preferences/applications.js > @@ +966,5 @@ > > if (type in this._handledTypes) > > handlerInfoWrapper = this._handledTypes[type]; > > else { > > let wrappedHandlerInfo = > > + gMIMEService.getFromTypeAndExtension(mimeType.type, null); > > where does mimeType come from? It's in _loadPluginHandlers() in part3. So part2 isn't 100% correct itself but with all parts applied all is defined. See also https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fdf3be2a311263cc557a8757ed680cc8431f9805 without the applicationManager.js change in part4.

Comment on attachment 9012971 [details] [diff] [review] handler_part4.patch Review of attachment 9012971 [details] [diff] [review]: ----------------------------------------------------------------- I can see whether I can update part 4 without the rest unravelling. ::: mail/components/preferences/applicationManager.js @@ +14,5 @@ > var bundle = document.getElementById("appManagerBundle"); > gApplicationsPane._prefsBundle = document.getElementById("bundlePreferences"); > > + this.handlerInfo = window.arguments[0]; > + var bundle = document.getElementById("appManagerBundle"); This gives a linting error since |var bundle| is already there a few lines above.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/1dfd8879d65b Port bug 1457027 to TB: Part 1 - Don't persist the last selected handler. r=aceman https://hg.mozilla.org/comm-central/rev/e490dff436dd Port bug 1457027 to TB: Part 2 - Define services using defineLazyServiceGetters. r=aceman https://hg.mozilla.org/comm-central/rev/be4843f65ece Port bug 1457027 to TB: Part 4 - Use class syntax for the HandlerInfoWrapper hierarchy. r=aceman https://hg.mozilla.org/comm-central/rev/f686a4e62dc8 Port bug 1457027 to TB: Part 5 - Move _describeType to HandlerInfoWrapper. r=aceman https://hg.mozilla.org/comm-central/rev/cc895db45e99 Port bug 1457027 to TB: Part 6 - Move _describePreferredAction to HandlerInfoWrapper. r=aceman https://hg.mozilla.org/comm-central/rev/eca38904a853 Port bug 1457027 to TB: Part 7 - Move action icon getters to HandlerInfoWrapper. r=aceman https://hg.mozilla.org/comm-central/rev/2131ecd0173c Port bug 1457027 to TB: Part 8 - Add a HandlerListItem class. r=aceman

Part 3 wasn't ported, see comment #4.