1323685 - Remove workarounds for bug 449811

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marco Bonardo [:mak]]

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

Comment 1

[Aman Dwivedi]

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?

Comment 2

[Marco Bonardo [:mak]]

(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.

Comment 3

[Marco Bonardo [:mak]]

note that scope.js should not be touched.

Comment 4

[Aman Dwivedi]

Attachment: 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.

Comment 5

[Marco Bonardo [:mak]]

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; }

Comment 6

[Aman Dwivedi]

Attachment: Bug 1323685.patch

Thanks! I have made the required changes. Hope this patch works.

Comment 7

[Marco Bonardo [:mak]]

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)

Comment 8

[Aman Dwivedi]

Attachment: BugFix1323685.patch

New patch attached. Please review. Hope this works.

Comment 9

[Marco Bonardo [:mak]]

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.

Comment 10

[Marco Bonardo [:mak]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=33fde01ed66732c41968da765dcd571a990c7402

Comment 11

[Aman Dwivedi]

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. :)

Comment 12

[Marco Bonardo [:mak]]

Do you know about http://www.joshmatthews.net/bugsahoy/ ?

Comment 13

[Aman Dwivedi]

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

Comment 14

[Marco Bonardo [:mak]]

Sure, done.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd673cc65baa
Remove workarounds for bug 449811. r=mak

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cd673cc65baa