1460392 - Port bug 1457027 to TB: Create a HandlerListItem class for richlistbox#handlersView

Status: RESOLVED FIXED

(Thunderbird :: Preferences, task)

Description

[Richard Marti (:Paenglab)]

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

Comment 1

[Richard Marti (:Paenglab)]

Attachment: handler_part1.patch

Part 1 - Don't persist the last selected handler

Comment 2

[Richard Marti (:Paenglab)]

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

Comment 3

[Richard Marti (:Paenglab)]

Attachment: handler_part2.patch

Part 2 - Define services using defineLazyServiceGetters

Port of https://hg.mozilla.org/mozilla-central/rev/8834a397305b

Comment 4

[Richard Marti (:Paenglab)]

Attachment: handler_part3.patch

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.

Comment 5

[Richard Marti (:Paenglab)]

Attachment: handler_part4.patch

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?

Comment 6

[Richard Marti (:Paenglab)]

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

Comment 7

[:aceman]

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.

Comment 8

[Richard Marti (:Paenglab)]

Attachment: handler_part4.patch

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

Comment 9

[Richard Marti (:Paenglab)]

Attachment: handler_part5.patch

Part 6 - Move _describePreferredAction to HandlerInfoWrapper.

Port of https://hg.mozilla.org/mozilla-central/rev/3e98dcc160f8

Comment 10

[Richard Marti (:Paenglab)]

Attachment: handler_part6.patch

Part 6 - Move action icon getters to HandlerInfoWrapper.

Port of https://hg.mozilla.org/mozilla-central/rev/b29b502c0fce

Comment 11

[Richard Marti (:Paenglab)]

Attachment: handler_part7.patch

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.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: handler_part7.patch

Part 7 fixed after talk over IRC.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: handler_part4.patch

Updated to tip.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: handler_part5.patch

Updated to tip.

Comment 15

[Richard Marti (:Paenglab)]

Attachment: handler_part6.patch

Updated to tip.

Comment 16

[Richard Marti (:Paenglab)]

Attachment: handler_part7.patch

Updated to tip.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: handler_part1.patch

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

Comment 18

[Richard Marti (:Paenglab)]

Attachment: handler_part2.patch

Comment 19

[Richard Marti (:Paenglab)]

Attachment: handler_part3.patch

Comment 20

[Richard Marti (:Paenglab)]

Attachment: handler_part4.patch

Comment 21

[Richard Marti (:Paenglab)]

Attachment: handler_part5.patch

Comment 22

[Richard Marti (:Paenglab)]

Attachment: handler_part6.patch

Comment 23

[Richard Marti (:Paenglab)]

Attachment: handler_part7.patch

Comment 24

[Richard Marti (:Paenglab)]

Aceman, please can you check the linting too?

Comment 25

[:aceman]

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.

Comment 26

[:aceman]

(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 27

[:aceman]

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 28

[:aceman]

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.

Comment 29

[Richard Marti (:Paenglab)]

Attachment: handler_part3.patch

(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.

Comment 30

[Richard Marti (:Paenglab)]

(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.

Comment 31

[Richard Marti (:Paenglab)]

Attachment: handler_part6.patch

(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.

Comment 32

[Richard Marti (:Paenglab)]

Attachment: handler_part7.patch

Part_7 needed a rebase to apply.

Comment 33

[Richard Marti (:Paenglab)]

Attachment: handler_part4.patch

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

Comment 34

[:aceman]

Comment on attachment 9012971 [details] [diff] [review]
handler_part4.patch

Review of attachment 9012971 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks

Comment 35

[:aceman]

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 36

[Jorg K (CEST = GMT+2)]

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?

Comment 37

[Richard Marti (:Paenglab)]

(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 38

[Jorg K (CEST = GMT+2)]

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.

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: handler_part4.patch

Comment 40

[Pulsebot]

Pushed by [e-mail]:
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

Comment 41

[Jorg K (CEST = GMT+2)]

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