Closed Bug 1460392 Opened 2 years ago Closed Last year

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

Categories

(Thunderbird :: Preferences, task)

task
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(7 files, 18 obsolete files)

5.65 KB, patch
aceman
: review+
Details | Diff | Splinter Review
4.51 KB, patch
aceman
: review+
Details | Diff | Splinter Review
12.89 KB, patch
aceman
: review+
Details | Diff | Splinter Review
16.08 KB, patch
aceman
: review+
Details | Diff | Splinter Review
11.28 KB, patch
aceman
: review+
Details | Diff | Splinter Review
8.13 KB, patch
aceman
: review+
Details | Diff | Splinter Review
11.09 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
FX does this also for the de-XBL. When we do this now, this should then work also in the future.
Attached patch handler_part1.patch (obsolete) — Splinter Review
Part 1 - Don't persist the last selected handler
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8974496 - Flags: review?(acelists)
Attached patch handler_part2.patch (obsolete) — Splinter Review
Part 2 - Define services using defineLazyServiceGetters

Port of https://hg.mozilla.org/mozilla-central/rev/8834a397305b
Attachment #8974498 - Flags: review?(acelists)
Attached patch handler_part3.patch (obsolete) — Splinter Review
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.
Attachment #8974499 - Flags: review?(acelists)
Attached patch handler_part4.patch (obsolete) — Splinter Review
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?
Attachment #8974501 - Flags: feedback?(acelists)
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.
Attached patch handler_part4.patch (obsolete) — Splinter Review
FX doesn't have the description (for example: (application/pdf: .pdf) ). This is what broke. I managed this now.
Attachment #8974501 - Attachment is obsolete: true
Attachment #8974501 - Flags: feedback?(acelists)
Attachment #8974815 - Flags: review?(acelists)
Attached patch handler_part5.patch (obsolete) — Splinter Review
Part 6 - Move _describePreferredAction to HandlerInfoWrapper.

Port of https://hg.mozilla.org/mozilla-central/rev/3e98dcc160f8
Attachment #8974816 - Flags: review?(acelists)
Attached patch handler_part6.patch (obsolete) — Splinter Review
Part 6 - Move action icon getters to HandlerInfoWrapper.

Port of https://hg.mozilla.org/mozilla-central/rev/b29b502c0fce
Attachment #8974817 - Flags: review?(acelists)
Attached patch handler_part7.patch (obsolete) — Splinter Review
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.
Attachment #8974821 - Flags: feedback?(acelists)
Attached patch handler_part7.patch (obsolete) — Splinter Review
Part 7 fixed after talk over IRC.
Attachment #8974821 - Attachment is obsolete: true
Attachment #8974821 - Flags: feedback?(acelists)
Attachment #8974833 - Flags: review?(acelists)
Blocks: 1468167
Attached patch handler_part4.patch (obsolete) — Splinter Review
Updated to tip.
Attachment #8974815 - Attachment is obsolete: true
Attachment #8974815 - Flags: review?(acelists)
Attachment #8989712 - Flags: review?(acelists)
Attached patch handler_part5.patch (obsolete) — Splinter Review
Updated to tip.
Attachment #8974816 - Attachment is obsolete: true
Attachment #8974816 - Flags: review?(acelists)
Attachment #8989713 - Flags: review?(acelists)
Attached patch handler_part6.patch (obsolete) — Splinter Review
Updated to tip.
Attachment #8974817 - Attachment is obsolete: true
Attachment #8974817 - Flags: review?(acelists)
Attachment #8989714 - Flags: review?(acelists)
Attached patch handler_part7.patch (obsolete) — Splinter Review
Updated to tip.
Attachment #8974833 - Attachment is obsolete: true
Attachment #8974833 - Flags: review?(acelists)
Attachment #8989715 - Flags: review?(acelists)
No longer blocks: tb-war-on-xbl
I had to update all patches because of the linting in this directory.
Attachment #8974496 - Attachment is obsolete: true
Attachment #8974496 - Flags: review?(acelists)
Attachment #9011182 - Flags: review?(acelists)
Attachment #8974498 - Attachment is obsolete: true
Attachment #8974498 - Flags: review?(acelists)
Attachment #9011183 - Flags: review?(acelists)
Attached patch handler_part3.patch (obsolete) — Splinter Review
Attachment #8974499 - Attachment is obsolete: true
Attachment #8974499 - Flags: review?(acelists)
Attachment #9011184 - Flags: review?(acelists)
Attached patch handler_part4.patch (obsolete) — Splinter Review
Attachment #8989712 - Attachment is obsolete: true
Attachment #8989712 - Flags: review?(acelists)
Attachment #9011185 - Flags: review?(acelists)
Attachment #8989713 - Attachment is obsolete: true
Attachment #8989713 - Flags: review?(acelists)
Attachment #9011186 - Flags: review?(acelists)
Attached patch handler_part6.patch (obsolete) — Splinter Review
Attachment #8989714 - Attachment is obsolete: true
Attachment #8989714 - Flags: review?(acelists)
Attachment #9011187 - Flags: review?(acelists)
Attached patch handler_part7.patch (obsolete) — Splinter Review
Attachment #8989715 - Attachment is obsolete: true
Attachment #8989715 - Flags: review?(acelists)
Attachment #9011188 - Flags: review?(acelists)
Aceman, please can you check the linting too?
Attachment #9011182 - Flags: review?(acelists) → review+
Attachment #9011183 - Flags: review?(acelists) → review+
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.
Attachment #9011184 - Flags: review?(acelists) → review+
(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 ?
Attachment #9011186 - Flags: review?(acelists) → review+
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.
Attachment #9011184 - Attachment is obsolete: true
Attachment #9012792 - Flags: review?(acelists)
(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.
Attachment #9011187 - Attachment is obsolete: true
Attachment #9011187 - Flags: review?(acelists)
Attachment #9012794 - Flags: review?(acelists)
Part_7 needed a rebase to apply.
Attachment #9011188 - Attachment is obsolete: true
Attachment #9011188 - Flags: review?(acelists)
Attachment #9012795 - Flags: review?(acelists)
Attached patch handler_part4.patch (obsolete) — Splinter Review
After talk with aceman over IRC he found that applicationManager.js needed some changes to work correctly.
Attachment #9011185 - Attachment is obsolete: true
Attachment #9011185 - Flags: review?(acelists)
Attachment #9012971 - Flags: review?(acelists)
Attachment #9012792 - Flags: review?(acelists) → review+
Comment on attachment 9012971 [details] [diff] [review]
handler_part4.patch

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

Thanks
Attachment #9012971 - Flags: review?(acelists) → review+
Attachment #9012794 - Flags: review?(acelists) → review+
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.
Attachment #9012795 - Flags: review?(acelists) → review+
Keywords: checkin-needed
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.
Attachment #9012971 - Attachment is obsolete: true
Attachment #9012993 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Part 3 wasn't ported, see comment #4.
Target Milestone: --- → Thunderbird 64.0
Regressions: 1576950
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.