1431428 - Use DOM manipulations to insert add-on / toolkit icons in "controlling extension" message in the preferences

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P5)

Description

[:Gijs (out for now; he/him)]

We currently use innerHTML and it triggers an eslint warning. We can tidy this up a little.

Comment 1

[:Gijs (out for now; he/him)]

Err, specifically, this code:

https://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/preferences.js#543-547

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Permalink for posterity, https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/browser/components/preferences/in-content/preferences.js#543-547

Comment 3

[:Gijs (out for now; he/him)]

Attachment: Bug 1431428 - use DOM instead of innerHTML for extension messaging in prefs,

Review commit: https://reviewboard.mozilla.org/r/214662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214662/

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8944468 [details]
Bug 1431428 - use DOM instead of innerHTML for extension messaging in prefs,

https://reviewboard.mozilla.org/r/214662/#review220288


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/in-content/preferences.js:546
(Diff revision 1)
> -  let icon = url => `<image src="${url}" class="extension-controlled-icon"/>`;
> +  let icon = url => {
> +    let img = document.createElement("image");
> +    img.src = url;
> +    img.className = "extension-controlled-icon";
> +    return img;
> +  }

Error: Missing semicolon. [eslint: semi]

Comment 5

[:Gijs (out for now; he/him)]

Comment on attachment 8944468 [details]
Bug 1431428 - use DOM instead of innerHTML for extension messaging in prefs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214662/diff/1-2/

Comment 6

[:Gijs (out for now; he/him)]

(In reply to Code Review Bot [:reviewbot] from comment #4)
> Error: Missing semicolon. [eslint: semi]

Oops, fixed in the update.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8944468 [details]
Bug 1431428 - use DOM instead of innerHTML for extension messaging in prefs,

https://reviewboard.mozilla.org/r/214662/#review220386

::: toolkit/modules/BrowserUtils.jsm:748
(Diff revision 2)
> +  getLocalizedFragment(doc, msg, ...nodesOrStrings) {
> +    // Ensure replacement points are indexed:
> +    for (let i = 1; i <= nodesOrStrings.length; i++) {
> +      if (!msg.includes("%" + i + "$S")) {
> +        msg = msg.replace(/%S/, "%" + i + "$S");
> +      }
> +    }

Can we log an error if `nodesOrStrings.length` is not equal to the number of insertion points in `msg`? Or throw an exception? I'll defer to your choice here but I think we should do something.

Comment 8

[:Gijs (out for now; he/him)]

Comment on attachment 8944468 [details]
Bug 1431428 - use DOM instead of innerHTML for extension messaging in prefs,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214662/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2018c9901076
use DOM instead of innerHTML for extension messaging in prefs, r=jaws

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/2018c9901076