Remove unsafeSetInnerHTML in browser_l10n_localizeMarkup.js

RESOLVED FIXED in Firefox 61

Status

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: johannh, Assigned: trisha, Mentored)

Tracking

60 Branch
Firefox 61
Dependency tree / graph

Firefox Tracking Flags

(firefox60 wontfix, firefox61 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Reporter

Description

a year ago
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
Assignee

Comment 1

a year ago
Can I take this up since I'm already working on 1444441?
Reporter

Comment 2

a year ago
Sure!
Mentor: jhofmann
Assignee

Updated

a year ago
Assignee: nobody → guptatrisha97
Assignee

Comment 3

a year ago
Posted patch bug1444445.patch (obsolete) — Splinter Review
Attachment #8963556 - Flags: review?(jhofmann)
Reporter

Updated

a year ago
Status: NEW → ASSIGNED
Assignee

Comment 4

a year ago
Is there anything I need to change/add in this patch? Thanks.
Reporter

Comment 5

a year ago
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-
Assignee

Comment 6

a year ago
Posted 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)
Reporter

Comment 7

a year ago
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-
Assignee

Comment 8

a year ago
Posted patch bug1444445.patch (obsolete) — Splinter Review
Attachment #8965657 - Attachment is obsolete: true
Attachment #8965775 - Flags: review?(jhofmann)
Assignee

Comment 9

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

Comment 10

a year ago
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-
Assignee

Comment 11

a year ago
Posted patch bug1444445.patch (obsolete) — Splinter Review
Attachment #8965775 - Attachment is obsolete: true
Attachment #8966199 - Flags: review?(jhofmann)
Assignee

Comment 12

a year ago
Posted patch bug1444445.patch (obsolete) — Splinter Review
Attachment #8966199 - Attachment is obsolete: true
Attachment #8966199 - Flags: review?(jhofmann)
Attachment #8966214 - Flags: review?(jhofmann)
Reporter

Comment 13

a year ago
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-
Assignee

Comment 14

a year ago
Attachment #8966214 - Attachment is obsolete: true
Attachment #8966826 - Flags: review?(jhofmann)
Reporter

Comment 15

a year ago
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+
Assignee

Updated

a year ago
Keywords: checkin-needed

Comment 16

a year ago
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
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.