Closed Bug 1431428 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Settings UI, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(1 file)

We currently use innerHTML and it triggers an eslint warning. We can tidy this up a little.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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]
(In reply to Code Review Bot [:reviewbot] from comment #4)
> Error: Missing semicolon. [eslint: semi]

Oops, fixed in the update.
Priority: -- → P5
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.
Attachment #8944468 - Flags: review?(jaws) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2018c9901076
use DOM instead of innerHTML for extension messaging in prefs, r=jaws
https://hg.mozilla.org/mozilla-central/rev/2018c9901076
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1432555
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: