Closed Bug 1444868 Opened 2 years ago Closed 2 years ago

Setting permissionsText in the exceptions dialog (permissions.js) can be simplified

Categories

(Firefox :: Preferences, enhancement, P5)

60 Branch
enhancement

Tracking

()

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

People

(Reporter: johannh, Assigned: trisha, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file)

https://searchfox.org/mozilla-central/rev/a70da6775d5341a9cca86bf1572a5e3534909153/browser/components/preferences/permissions.js#219

Instead of iterating over child nodes and deleting them, then making a new textNode and inserting it, we can just use .textContent to override the intro text directly.
(In reply to Johann Hofmann [:johannh] from comment #0)
> https://searchfox.org/mozilla-central/rev/
> a70da6775d5341a9cca86bf1572a5e3534909153/browser/components/preferences/
> permissions.js#219
> 
> Instead of iterating over child nodes and deleting them, then making a new
> textNode and inserting it, we can just use .textContent to override the
> intro text directly.
Do you mean it overwrites it?

Is this what it should do somewhat?

 var permissionsText = document.getElementById("permissionsText");
 permissionsText.textContent=aParams.introText;

Can you tell me more about aParams.introText please? Where does it come from? Can't find it anywhere in Searchfox.
(In reply to Trisha from comment #1)
> (In reply to Johann Hofmann [:johannh] from comment #0)
> > https://searchfox.org/mozilla-central/rev/
> > a70da6775d5341a9cca86bf1572a5e3534909153/browser/components/preferences/
> > permissions.js#219
> > 
> > Instead of iterating over child nodes and deleting them, then making a new
> > textNode and inserting it, we can just use .textContent to override the
> > intro text directly.
> Do you mean it overwrites it?

Yeah, overwrite :)

> Is this what it should do somewhat?
> 
>  var permissionsText = document.getElementById("permissionsText");
>  permissionsText.textContent=aParams.introText;

That looks reasonable to me, you could use let instead of var and be sure to run ./mach eslint :)

> Can you tell me more about aParams.introText please? Where does it come
> from? Can't find it anywhere in Searchfox.

introText is passed by the code opening this dialog to specify the description that should show at the top of the dialog.

An example is here: https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/browser/components/preferences/in-content/privacy.js#931

Which can be seen when going to about:preferences -> Privacy & Security -> Cookies & Site Data -> Exceptions

You should verify that after your change the dialog still shows this text:

https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/browser/locales/en-US/chrome/browser/preferences/preferences.properties#25

Thanks!
Assignee: nobody → guptatrisha97
Status: NEW → ASSIGNED
Attached patch bug1444868.patchSplinter Review
I ran the test and it gave no errors. Please review. Thanks!
Attachment #8958529 - Flags: review?(jhofmann)
Comment on attachment 8958529 [details] [diff] [review]
bug1444868.patch

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

This looks good to me, thank you!
Attachment #8958529 - Flags: review?(jhofmann) → review+
Keywords: checkin-needed
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/93714d07f744
use .textContent to override the intro text directly in permissions.js. r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/93714d07f744
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.