Paul Morris [:pmorris]
	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?