Remove workarounds for bug 449811

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: mak, Assigned: Aman Dwivedi, Mentored)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 months ago
We have various workarounds for bug 449811 in the codebase, now that it's fixed they can be removed.

See
http://searchfox.org/mozilla-central/search?q=bug+449811&case=false&regexp=false&path=

We should not need anymore to create a local scoped var for each iteration.+

Note: still waiting an answer about https://bugzilla.mozilla.org/show_bug.cgi?id=449811#c41
(Assignee)

Comment 1

7 months ago
Hi Marco!
I'm new to the community. I would like to take this up. Please assign it to me. As in the bug, do you mean like removing the declaration of local variable in the scope?
Flags: needinfo?(mak77)
Component: General → JavaScript Engine
Product: Firefox → Core
(Reporter)

Comment 2

7 months ago
(In reply to Aman Dwivedi from comment #1)
> Hi Marco!
> I'm new to the community. I would like to take this up. Please assign it to
> me. As in the bug, do you mean like removing the declaration of local
> variable in the scope?

In most cases you can remove the local variable and name the loop variable as the local variable.
Assignee: nobody → dwivedi.aman96
Flags: needinfo?(mak77)
(Reporter)

Comment 3

7 months ago
note that scope.js should not be touched.
(Assignee)

Comment 4

6 months ago
Created attachment 8819381 [details] [diff] [review]
Bug Fix.patch

I hope the patch fixes the issue. I have not touched 'scope.js'. Worked out on all the files. Also, I changed 'aboutSupport.js', but I was unable to figure out whether 'onClickReset' needs any change or not.
Attachment #8819381 - Flags: review?(mak77)
(Reporter)

Comment 5

6 months ago
Comment on attachment 8819381 [details] [diff] [review]
Bug Fix.patch

Review of attachment 8819381 [details] [diff] [review]:
-----------------------------------------------------------------

thank you, comments following.

::: browser/base/content/sanitize.js
@@ +158,4 @@
>      // sanitization is asynchronous, or the function return value, otherwise.
>      let handles = [];
>      for (let itemName of itemsToClear) {
> +      let item = this.items[itemName];

The following code uses "name" that you removed so it doesn't look like it's going to work.
You should instead probably change the var name in the for loop definition to for (let name of...

::: browser/components/migration/MigrationUtils.jsm
@@ +290,3 @@
>        }
>        notify("Migration:Started");
>        for (let [key, value] of resourcesGroupedByItems) {

key, value are really generic names that would not help much later code.
it would be simpler and more readable if you'd just change this to for (let [migrationType, itemResources] of...

::: toolkit/content/aboutSupport.js
@@ +509,4 @@
>          let onClickReset = (function(guard) {
>            // Note - need this wrapper until bug 449811 fixes |guard| scoping.
>            return function() {
>              Services.prefs.setIntPref(guard.prefName, 0);

Regardless, you should remove the comment about bug 449811... Also, this can now be simplified, this is basically creating and invoking a function with the current guard value, just to create a new function that will be invoked with the right guard.
This was needed cause guard was always replaced with the last value in the array, but now it won't happen anymore. So this can be changed from:
onClickReset = (function (guard){return function() { use_guard; }})(guard);
to:
onClickReset = function() { use_guard; }
Attachment #8819381 - Flags: review?(mak77) → review-
(Assignee)

Comment 6

6 months ago
Created attachment 8820401 [details] [diff] [review]
Bug 1323685.patch

Thanks! I have made the required changes. Hope this patch works.
Attachment #8819381 - Attachment is obsolete: true
Attachment #8820401 - Flags: review?(mak77)
(Reporter)

Comment 7

6 months ago
Comment on attachment 8820401 [details] [diff] [review]
Bug 1323685.patch

Review of attachment 8820401 [details] [diff] [review]:
-----------------------------------------------------------------

please, set an appropriate author and commit message on the patch, see https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Also, please address the following comments:

::: toolkit/content/aboutSupport.js
@@ +510,2 @@
>              Services.prefs.setIntPref(guard.prefName, 0);
> +            $.new("button").removeEventListener("click", onClickReset);

please retain the local resetButton variable instead of repeating $.new("button") everywhere.

@@ +510,4 @@
>              Services.prefs.setIntPref(guard.prefName, 0);
> +            $.new("button").removeEventListener("click", onClickReset);
> +            $.new("button").disabled = true;
> +          }

missing semicolon and the indentation looks wrong? (the function body should only indent by 2 spaces, not 4)
Attachment #8820401 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 8

6 months ago
Created attachment 8821097 [details] [diff] [review]
BugFix1323685.patch

New patch attached. Please review. Hope this works.
Attachment #8820401 - Attachment is obsolete: true
Attachment #8821097 - Flags: review?(mak77)
(Reporter)

Comment 9

6 months ago
Comment on attachment 8821097 [details] [diff] [review]
BugFix1323685.patch

Review of attachment 8821097 [details] [diff] [review]:
-----------------------------------------------------------------

It looks good thank you!

I will now push this to the Try server and later set the checkin-needed flag.

For future patches I'd suggest you to have a look at the mozreview process, it's an alternative process to submit patches for review, and while the initial approach may look harder, it simplifies some of the boilerplate for pushing and reviewing. It's not mandatory, just a suggestion.
Attachment #8821097 - Flags: review?(mak77) → review+
(Reporter)

Comment 10

6 months ago
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=33fde01ed66732c41968da765dcd571a990c7402
(Reporter)

Updated

6 months ago
Keywords: checkin-needed
(Assignee)

Comment 11

6 months ago
Thanks Marco for helping me get started. Would love to contribute more to the Mozilla Codebase. Please assign/suggest some more bugs/tasks to perform. :)
(Reporter)

Comment 12

6 months ago
Do you know about http://www.joshmatthews.net/bugsahoy/ ?
(Assignee)

Comment 13

6 months ago
Yeah thanks for the info. Will look at that. I joined Mozillians. Can you help me with that. Need a vouch. :)
(Reporter)

Comment 14

6 months ago
Sure, done.

Comment 15

6 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd673cc65baa
Remove workarounds for bug 449811. r=mak
Keywords: checkin-needed

Comment 16

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cd673cc65baa
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.