Closed Bug 1444445 Opened 2 years ago Closed 2 years ago

Remove unsafeSetInnerHTML in browser_l10n_localizeMarkup.js

Categories

(DevTools :: General, enhancement)

60 Branch
enhancement
Not set
normal

Tracking

(firefox60 wontfix, firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: johannh, Assigned: trisha, Mentored)

References

Details

Attachments

(1 file, 5 obsolete files)

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
Can I take this up since I'm already working on 1444441?
Assignee: nobody → guptatrisha97
Attached patch bug1444445.patch (obsolete) — Splinter Review
Attachment #8963556 - Flags: review?(jhofmann)
Is there anything I need to change/add in this patch? Thanks.
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
Attachment #8963556 - Flags: review?(jhofmann) → review-
Attached patch bug1444445.patch (obsolete) — Splinter Review
Thanks for your review. I ran the mochitest after the artifact build and it seemed to be successful. Does this patch look okay now?
Attachment #8963556 - Attachment is obsolete: true
Attachment #8965657 - Flags: review?(jhofmann)
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?
Attachment #8965657 - Flags: review?(jhofmann) → review-
Attached patch bug1444445.patch (obsolete) — Splinter Review
Attachment #8965657 - Attachment is obsolete: true
Attachment #8965775 - Flags: review?(jhofmann)
Can I get TryServer access so as to run the tests before checking this patch in (once it gets review+)? Thanks
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
Attachment #8965775 - Flags: review?(jhofmann) → review-
Attached patch bug1444445.patch (obsolete) — Splinter Review
Attachment #8965775 - Attachment is obsolete: true
Attachment #8966199 - Flags: review?(jhofmann)
Attached patch bug1444445.patch (obsolete) — Splinter Review
Attachment #8966199 - Attachment is obsolete: true
Attachment #8966199 - Flags: review?(jhofmann)
Attachment #8966214 - Flags: review?(jhofmann)
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!
Attachment #8966214 - Flags: review?(jhofmann) → review-
Attached patch bug1444445.patchSplinter Review
Attachment #8966214 - Attachment is obsolete: true
Attachment #8966826 - Flags: review?(jhofmann)
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.
Attachment #8966826 - Flags: review?(jhofmann) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dbca44c0855
Remove unsafeSetInnerHTML in browser_l10n_localizeMarkup.js. r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8dbca44c0855
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.