[de-xbl] convert charsetpicker binding to customized built-in based on menulist
Categories
(Thunderbird :: General, task)
Tracking
(Not tracked)
People
(Reporter: mkmelin, Assigned: mkmelin)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1524497 +++
charsetpicker should likely be converted to <menulist is="charset-picker">
https://searchfox.org/comm-central/source/mailnews/base/content/charsetList.xml#13
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Make it a custom element. The initial selection doesn't show (but opening it you can see it's correct). This is an other bug also seen in the calendar dialogs for selecting calendars.
I fixed some boilerplates since they were not formatted correctly for whatever reason.
Off to try - https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=847783718185e02b912719de84bbfb82e10fc722
Assignee | ||
Updated•6 years ago
|
Comment 3•6 years ago
|
||
Comment on attachment 9051226 [details] [diff] [review] bug1524508_charsetpicker.patch Review of attachment 9051226 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I had a few suggestions, so r+ from me with those addressed. I haven't yet applied the patch and manually tested it, but wanted to go ahead and post my code review. (Is manual testing needed if the try run was good? Just clarifying expectations for reviewers / reviewing.) ::: mailnews/base/content/menulist-charsetpicker.js @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ > + > +customElements.whenDefined("menulist").then(() => { > + var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm"); This could be const or if not const then let instead of var. @@ +20,5 @@ > > + connectedCallback() { > + super.connectedCallback(); > + if (this.delayConnectedCallback()) { > + return; Do we need the delayConnectedCallback check in this case? @@ +56,5 @@ > + let strCharset = charsetBundle.GetStringFromName( > + item.toLowerCase() + ".title"); > + > + menuLabels.push({ label: strCharset, value: item }); > + }); We could use map instead of forEach here: let menuLabels = charsetValues.map((item) => { ... return { label: strCharset, value: item }; }); @@ +100,5 @@ > +}); > + > +// The menulist CE is defined lazily. Create one now to get menulist defined, > +// allowing us to inherit from it. > +delete document.createElement("menulist"); Hmm, what about wrapping this in a check to see if it is already defined?
Assignee | ||
Comment 4•6 years ago
|
||
Re reviewing and testing things out: judgement call - if it's easy to test, usually worth doing.
Thx, regarding delayConnectedCallback(): probably not need, but still preferable. See bug 1495946 comment 22.
Assignee | ||
Comment 5•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/86cdbc8a3cef
[de-xbl] convert charsetpicker binding to customized built-in based on menulist. r=pmorris
Updated•6 years ago
|
Updated•5 years ago
|
Description
•