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

RESOLVED FIXED in Firefox 61

Status

()

P5
normal
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: johannh, Assigned: trisha, Mentored)

Tracking

({good-first-bug})

60 Branch
Firefox 61
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox60 wontfix, firefox61 fixed)

Details

Attachments

(1 attachment)

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.
(Assignee)

Comment 1

9 months ago
(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
(Assignee)

Comment 3

9 months ago
Created attachment 8958529 [details] [diff] [review]
bug1444868.patch

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

Comment 5

9 months ago
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

Comment 6

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/93714d07f744
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
status-firefox60: affected → wontfix
You need to log in before you can comment on or make changes to this bug.