1444853 - Setting permissionsText in the site permissions dialog can be simplified

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P5)

Description

[Johann Hofmann [:johannh]]

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

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.

Comment 1

[Aakanksha [:accakks]]

Would like to work on this!

Comment 2

[Johann Hofmann [:johannh]]

Thanks!

Comment 3

[Aakanksha [:accakks]]

Attachment: Bug 1444853 - Setting permissionsText in the site permissions dialog can be simplified

Review commit: https://reviewboard.mozilla.org/r/227726/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/227726/

Comment 4

[Aakanksha [:accakks]]

Comment on attachment 8958841 [details]
Bug 1444853 - Setting permissionsText in the site permissions dialog can be simplified

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227726/diff/1-2/

Comment 5

[Aakanksha [:accakks]]

Comment on attachment 8958841 [details]
Bug 1444853 - Setting permissionsText in the site permissions dialog can be simplified

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227726/diff/2-3/

Comment 6

[:prathiksha]

Comment on attachment 8958841 [details]
Bug 1444853 - Setting permissionsText in the site permissions dialog can be simplified

https://reviewboard.mozilla.org/r/227726/#review233770

Please make these changes and request review again.

It looks like you're making a few linting errors(extra whitespaces, extra new lines, etc). That can be handled by running the './mach eslint <path_to_file>' command. Before submmiting your changeset for review, it's always a good idea to run eslint and correct all the linting errors you have made. :)

::: browser/components/preferences/sitePermissions.js:54
(Diff revision 3)
> -    let permissionsText = document.getElementById("permissionsText");
> -    while (permissionsText.hasChildNodes())
> -      permissionsText.firstChild.remove();
> -    permissionsText.appendChild(document.createTextNode(params.introText));
>  
> +    let permissionsText = document.getElementById("permissionsText").textContent; 

Please remove the whitespace at the end of this line

::: browser/components/preferences/sitePermissions.js:54
(Diff revision 3)
> -    let permissionsText = document.getElementById("permissionsText");
> -    while (permissionsText.hasChildNodes())
> -      permissionsText.firstChild.remove();
> -    permissionsText.appendChild(document.createTextNode(params.introText));
>  
> +    let permissionsText = document.getElementById("permissionsText").textContent; 

This line should be left as it is:

let permissionsText = document.getElementById("permissionsText");

When you do this, you're bringing the DOM element with "permissionsText" as id and putting it in the permissionsText variable for later use.

.textContent is just a property that you can use on some XUL elements like label, description, etc to set text.

::: browser/components/preferences/sitePermissions.js:55
(Diff revision 3)
> -    while (permissionsText.hasChildNodes())
> -      permissionsText.firstChild.remove();
> -    permissionsText.appendChild(document.createTextNode(params.introText));
>  
> +    let permissionsText = document.getElementById("permissionsText").textContent; 
> +    document.getElementById("permissionsText").textContent=params.introText;

Here you can just do:

permissionText.textContent = params.introText;

Since you have already fetched the DOM element and stored it in the permissionsText variable in the above line, you don't have to fetch it again to set introText.

::: browser/components/preferences/sitePermissions.js:56
(Diff revision 3)
> -      permissionsText.firstChild.remove();
> -    permissionsText.appendChild(document.createTextNode(params.introText));
>  
> +    let permissionsText = document.getElementById("permissionsText").textContent; 
> +    document.getElementById("permissionsText").textContent=params.introText;
> +    

Please remove all the extra new lines you have added here

Comment 7

[Aakanksha [:accakks]]

Comment on attachment 8958841 [details]
Bug 1444853 - Setting permissionsText in the site permissions dialog can be simplified

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/227726/diff/3-4/

Comment 8

[:prathiksha]

Comment on attachment 8958841 [details]
Bug 1444853 - Setting permissionsText in the site permissions dialog can be simplified

https://reviewboard.mozilla.org/r/227726/#review233916

Looks great, thanks!

Now this needs a run on our try server, to check if tests are still green after this change
(https://wiki.mozilla.org/ReleaseEngineering/TryServer). I'll do that for you this time,
but in the future you might want to get your own try access.

Once we have a successful try run, you may set the checkin-needed flag to get your patch checked in.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6465338139d2
Setting permissionsText in the site permissions dialog can be simplified r=prathiksha

Comment 10

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/6465338139d2