1444445 - Remove unsafeSetInnerHTML in browser_l10n_localizeMarkup.js

Status: RESOLVED FIXED

(DevTools :: General, enhancement)

Description

[Johann Hofmann [:johannh]]

We're using unsafeSetInnerHTML to inject HTML in the browser_l10n_localizeMarkup.js test file. While that in itself is not dangerous, we're aiming to remove unsafeSetInnerHTML as soon as possible, and so we'll need to rewrite the test.

https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/devtools/shared/tests/browser/browser_l10n_localizeMarkup.js#21

Comment 1

[Trisha Gupta [:trisha]]

Can I take this up since I'm already working on 1444441?

Comment 2

[Johann Hofmann [:johannh]]

Sure!

Comment 3

[Trisha Gupta [:trisha]]

Attachment: bug1444445.patch

Comment 4

[Trisha Gupta [:trisha]]

Is there anything I need to change/add in this patch? Thanks.

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8963556 [details] [diff] [review]
bug1444445.patch

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

Thanks for working on this. A couple of comments below...

Did you run the test? Is it succeeding locally for you?

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js
@@ +22,5 @@
> +  let d0 = document.createElement("div");
> +  d0.setAttribute("id", "d0");
> +  d0.setAttribute("data-localization", "content=inspector.someInvalidKey");
> +  div.appendChild(d0);
> +  div.append("\n     ");

Please remove this and all other instances of adding whitespace.

@@ +35,5 @@
> +  d2.setAttribute("data-localization", "content=inspector.label;title=inspector.accesskey");
> +  div.appendChild(d2);
> +  d2.append("\n     ");
> +  div.append("\n     ");
> +  let comment = document.createComment("keep the following data-localization on two separate lines");

You don't need to create comments.

@@ +63,1 @@
>    

nit: you can remove the trailing whitespace while you're touching this file

Comment 6

[Trisha Gupta [:trisha]]

Attachment: bug1444445.patch

Thanks for your review. I ran the mochitest after the artifact build and it seemed to be successful. Does this patch look okay now?

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8965657 [details] [diff] [review]
bug1444445.patch

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

This looks good, but let's replace the variables we're getting here:

https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/devtools/shared/tests/browser/browser_l10n_localizeMarkup.js#40-44

in favor of d1, d2, etc. that are already defined?

Please also fix the two nits below :)

Thanks!

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js
@@ +43,5 @@
> +  d4.setAttribute("id", "d4");
> +  d4.setAttribute("data-localization", "aria-label=inspector.label");
> +  div.appendChild(d4);
> +  d4.append("Some content");
> +  let div0 = document.createElement("div");

nit: can we call this something a little less random than div0? Maybe toolboxDiv or something?

@@ +50,5 @@
> +  div.appendChild(div0);
> +  let d5 = document.createElement("div");
> +  d5.setAttribute("id", "d5");
> +  d5.setAttribute("data-localization", "content=toolbox.defaultTitle");
> +  div0.appendChild(d5);

nit: can you add a newline below this line please?

Comment 8

[Trisha Gupta [:trisha]]

Attachment: bug1444445.patch

Comment 9

[Trisha Gupta [:trisha]]

Can I get TryServer access so as to run the tests before checking this patch in (once it gets review+)? Thanks

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8965775 [details] [diff] [review]
bug1444445.patch

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

You still need to remove these variables: https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/devtools/shared/tests/browser/browser_l10n_localizeMarkup.js#40-44

The idea is that since we already have data1, data2, data3, etc. we don't need to get the same elements again as d1, d2, d3, etc. Just use the same variables everywhere instead.

Let me know if you need more help with that :)

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js
@@ +31,5 @@
> +  data1.append("Text will disappear");
> +  let data2 = document.createElement("div");
> +  data2.setAttribute("id", "d2");
> +  data2.setAttribute("data-localization",
> +                  "content=inspector.label;title=inspector.accesskey");

nit: please align this line with "data-...." from above (so that the two arguments are vertically aligned)

@@ +36,5 @@
> +  div.appendChild(data2);
> +  let data3 = document.createElement("div");
> +  data3.setAttribute("id", "d3");
> +  data3.setAttribute("data-localization",
> +                  "content=inspector.label;title=inspector.accesskey");

nit: please align the arguments

@@ +45,5 @@
> +  div.appendChild(data4);
> +  data4.append("Some content");
> +  let toolboxDiv = document.createElement("div");
> +  toolboxDiv.setAttribute("data-localization-bundle",
> +                    "devtools/client/locales/toolbox.properties");

nit: please align the arguments

@@ +51,5 @@
> +  let data5 = document.createElement("div");
> +  data5.setAttribute("id", "d5");
> +  data5.setAttribute("data-localization", "content=toolbox.defaultTitle");
> +  toolboxDiv.appendChild(data5);
> +         

nit: please remove the trailing whitespace here

Comment 11

[Trisha Gupta [:trisha]]

Attachment: bug1444445.patch

Comment 12

[Trisha Gupta [:trisha]]

Attachment: bug1444445.patch

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 8966214 [details] [diff] [review]
bug1444445.patch

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

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js
@@ +59,5 @@
> +  div1 = div.querySelector("#d1");
> +  div2 = div.querySelector("#d2");
> +  div3 = div.querySelector("#d3");
> +  div4 = div.querySelector("#d4");
> +  div5 = div.querySelector("#d5");

You can remove these 5 lines :)

::: npm-shrinkwrap.json
@@ +1003,5 @@
> +      "integrity": "sha512-4AH6Z5fzNNBcH+6XDMfA/BTt87skxqJlO0lAh3Dker5zThcAxG6mKz+iGu308UKoPPQ8Dcqx/4JhujzltRa+hQ==",
> +      "requires": {
> +        "safe-buffer": "5.1.1"
> +      }
> +    },

Please revert the change to this file. It was done accidentally by a mach script.

hg revert -r central npm-shrinkwrap.json

should do the trick.

Thanks!

Comment 14

[Trisha Gupta [:trisha]]

Attachment: bug1444445.patch

Comment 15

[Johann Hofmann [:johannh]]

Comment on attachment 8966826 [details] [diff] [review]
bug1444445.patch

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

That looks good, thank you! To get the patch checked in, please set the checkin-needed keyword.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dbca44c0855
Remove unsafeSetInnerHTML in browser_l10n_localizeMarkup.js. r=johannh

Comment 17

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8dbca44c0855