setText() in the security section can be simplified

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P5
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: johannh, Assigned: kanika16047, Mentored)

Tracking

({good-first-bug})

60 Branch
Firefox 61
Points:
---

Firefox Tracking Flags

(firefox60 wontfix, firefox61 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

Last year
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.
Assignee

Comment 1

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

Comment 2

Last year
Thanks!
Assignee: nobody → kanika16047
Status: NEW → ASSIGNED
Assignee

Comment 3

Last year
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!
Reporter

Comment 4

Last year
(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 hidden (mozreview-request)

Comment 6

Last year
mozreview-review
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.
Attachment #8959855 - Flags: review?(prathikshaprasadsuman) → review+
Assignee

Comment 7

Last year
mozreview-review-reply
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

Last year
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 hidden (mozreview-request)

Comment 10

Last year
Looks like the try run was successful:

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

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

Updated

Last year
Keywords: checkin-needed

Comment 11

Last year
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/33563f005b67
Simplify setText() function in security.js. r=prathiksha
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/33563f005b67
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.