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

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P5
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: johannh, Assigned: trisha, Mentored)

Tracking

({good-first-bug})

60 Branch
Firefox 61
Points:
---

Firefox Tracking Flags

(firefox60 wontfix, firefox61 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

a year 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.
(Reporter)

Comment 2

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

a year ago
I ran the test and it gave no errors. Please review. Thanks!
Attachment #8958529 - Flags: review?(jhofmann)
(Reporter)

Comment 4

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

Updated

a year ago
Keywords: checkin-needed

Comment 5

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

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