1444901 - setText() in the security section can be simplified

Status: RESOLVED FIXED

(Firefox :: Page Info Window, enhancement, P5)

Description

[Johann Hofmann [:johannh]]

https://searchfox.org/mozilla-central/rev/a70da6775d5341a9cca86bf1572a5e3534909153/browser/base/content/pageinfo/security.js#297

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

[Kanika Saini]

Hi! I'd like to work on this. :)

Comment 2

[Johann Hofmann [:johannh]]

Thanks!

Comment 3

[Kanika Saini]

Hi!

This is the output of hg diff after I edited the file.

diff --git a/browser/base/content/pageinfo/security.js b/browser/base/content/pageinfo/security.js
--- a/browser/base/content/pageinfo/security.js
+++ b/browser/base/content/pageinfo/security.js
@@ -294,10 +294,7 @@ function setText(id, value) {
   if (element.localName == "textbox" || element.localName == "label")
     element.value = value;
   else {
-    if (element.hasChildNodes())
-      element.firstChild.remove();
-    var textNode = document.createTextNode(value);
-    element.appendChild(textNode);
+    element.firstChild.textContent(value);
   }
 }
 

I was trying to "hg push review" it but I committed the merge from Mozilla-central and now it doesn't allow me to push the merge commits along with the actual one (as said in the documentation). I was looking for a way that resets the local repo (similar to git reset --hard) but couldn't find anything that's safe to do. Can you please suggest a way out?
Thank you!

Comment 4

[Johann Hofmann [:johannh]]

(In reply to kanika16047 from comment #3)
> +    element.firstChild.textContent(value);

Note that textContent is not a method, but a settable attribute (https://developer.mozilla.org/en-US/docs/Web/API/Node/textContent) and that we don't want to set the textContent of the firstChild but of the element itself :)

> 
> I was trying to "hg push review" it but I committed the merge from
> Mozilla-central and now it doesn't allow me to push the merge commits along
> with the actual one (as said in the documentation). I was looking for a way
> that resets the local repo (similar to git reset --hard) but couldn't find
> anything that's safe to do. Can you please suggest a way out?
> Thank you!

That sounds like an issue we should probably solve over IRC :)

Thanks!

Comment 5

[Kanika Saini]

Attachment: Bug 1444901 - Simplify setText() function in security.js.

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

Comment 6

[:prathiksha]

Comment on attachment 8959855 [details]
Bug 1444901 - Simplify setText() function in security.js.

https://reviewboard.mozilla.org/r/228620/#review234624

Looks good to me, thanks!

Please amend your commit message to make it sound imperative and to include the '-'. Something like:

"Bug 1444901 - Simplify setText() function in security.js. r?prathiksha"


We should probably also do a try run to make sure that the patch does not break anything(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 and you have amended the commit message, you may set the checkin-needed flag to get your patch checked in.

Comment 7

[Kanika Saini]

Comment on attachment 8959855 [details]
Bug 1444901 - Simplify setText() function in security.js.

https://reviewboard.mozilla.org/r/228620/#review234624

Hey!
Thank you for the review.
I'm trying to amend the commit message by doing the following.

hg rollback
hg commit -m "Bug 1444901 - Simplify setText() function in security.js. r?prathiksha"

Do I need to run hg push review again?

Comment 8

[:prathiksha]

I would recommend using the amend option with hg commit to amend an existing changeset. Something like:

hg commit --amend -m"Bug 1444901 - Simplify setText() function in security.js. r?prathiksha"

Using 'hg rollback' followed by 'hg commit' does not sound like good idea as you have to recreate a new changeset every time you have to change your patch. It fails when you have to make a small change to a huge patch with many different file changes.

And yes, once you amend your patch, you have 'hg push review' to submit that patch.

Comment 9

[Kanika Saini]

Comment on attachment 8959855 [details]
Bug 1444901 - Simplify setText() function in security.js.

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

Comment 10

[:prathiksha]

Looks like the try run was successful:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=717b81a4a1f52df17d3ae2daf6e53fedb591acb8

You can set the checkin-needed flag now. :)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33563f005b67
Simplify setText() function in security.js. r=prathiksha

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/33563f005b67