Closed Bug 1207497 Opened 8 years ago Closed 8 years ago

Remove use of expression closure from toolkit/, except toolkit/components/.

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: arai, Unassigned)

References

Details

Attachments

(2 files)

MozReview Request: Bug 1207497 - Part 1: Remove use of expression closure from toolkit/, exept tests. r?Gijs
40 bytes, text/x-review-board-request
Gijs
: review+
Details
MozReview Request: Bug 1207497 - Part 2: Remove use of expression closure from tests in toolkit/. r?Gijs
40 bytes, text/x-review-board-request
Gijs
: review+
Details 
Need to replace non-standard expression closure with one of:
  * function declaration
  * function expression
  * arrow function
before fixing bug 1083458.

converting rules are following:
  * function declaration
    add `return` and braces
  * standalone named function expression
    add `return` and braces
  * standalone anonymous function expression contans and receives `this` (Array.filter, bind, etc)
    convert to arrow function, and remove code passing |this|
  * standalone anonymous function expression contans no `this`
    convert to arrow function
  * property with anonymous function expression, contains `this`
    add `return` and braces
  * property with anonymous function expression, contains no `this`, short body
    convert to arrow function
  * property with anonymous function expression, contains no `this`, long body
    add `return` and braces
  * property with named function expression
    add `return` and braces
  * getter property
    add `return` and braces
  * setter property
    add braces

Since there are a lot of patches, separated into 8 bugs, each bug corresponds to one of following directories:
  * browser/, except browser/components/.
  * browser/components/.
  * dom/.
  * layout/.
  * services/.
  * toolkit/, except toolkit/components/.
  * toolkit/components/.
  * b2g/, chrome/, docshell/, mobiles/, modules/, netwerk/, parser/, security/, storage/, testing/, webapprt/, widget/, xpcom/
(not yet touched addon-sdk)

I have draft patches, will post them (may take some time to prepare and post).
Bug 1207497 - Part 1: Remove use of expression closure from toolkit/, exept tests. r?Gijs
Attachment #8665397 - Flags: review?(gijskruitbosch+bugs)
Bug 1207497 - Part 2: Remove use of expression closure from tests in toolkit/. r?Gijs
Attachment #8665398 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8665397 [details]
MozReview Request: Bug 1207497 - Part 1: Remove use of expression closure from toolkit/, exept tests. r?Gijs

https://reviewboard.mozilla.org/r/20225/#review18201

Basically, I think if we are using filter/some/map array methods, and pass in the function expression there, even if it does have a name it should be fine to replace it with an (anonymous) arrow function. If that breaks reference, then the function shouldn't be declared inside the filter/some/map array method call. :-\

::: toolkit/mozapps/extensions/AddonManager.jsm:1101
(Diff revision 1)
> -      this.types[type].providers = this.types[type].providers.filter(function filterProvider(p) p != aProvider);
> +      this.types[type].providers = this.types[type].providers.filter(function filterProvider(p) {
> +        return p != aProvider;
> +      });

filter(p => p != aProvider)

::: toolkit/mozapps/extensions/AddonManager.jsm:1685
(Diff revision 1)
>      this.startupChanges[aType] = this.startupChanges[aType].filter(
> -                                 function filterItem(aItem) aItem != aID);
> +                                 function filterItem(aItem) {
> +                                   return aItem != aID;
> +                                 });

aItem => aItem != aID

::: toolkit/mozapps/extensions/content/extensions.js:2338
(Diff revision 1)
>      var self = this;
>      this._listBox.addEventListener("keydown", function listbox_onKeydown(aEvent) {
>        if (aEvent.keyCode == aEvent.DOM_VK_RETURN) {
>          var item = self._listBox.selectedItem;
>          if (item)
>            item.showInDetailView();
>        }
>      }, false);

bonus points for rm'ing var self = this and using aEvent => {
}

and s/self/this/ inside the function body.

::: toolkit/mozapps/extensions/content/extensions.js:2347
(Diff revision 1)
> -    this._filter.addEventListener("command", function filter_onCommand() self.updateView(), false);
> +    this._filter.addEventListener("command", function filter_onCommand() {
> +      return self.updateView();
> +    }, false);

() => this.updateView()

(I think?)

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:991
(Diff revision 1)
>      let elementsList = Array.filter(aElement.children,
> -                                    function arrayFiltering(aChild) aChild.tagName == aTagName);
> +                                    function arrayFiltering(aChild) {
> +                                      return aChild.tagName == aTagName;
> +                                    });

aChild => aChild.tagName == aTagName

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:1308
(Diff revision 1)
> -      if (requiredAttributes.some(function parseAddons_attributeFilter(aAttribute) !result.addon[aAttribute]))
> +      if (requiredAttributes.some(function parseAddons_attributeFilter(aAttribute) {
> +        return !result.addon[aAttribute];
> +      }))

aAttribute => !resulrt.addon[aAttribute]

::: toolkit/mozapps/extensions/internal/AddonRepository.jsm:1435
(Diff revision 1)
> -                               .filter(function compatRangesFilter(aItem) !!aItem);
> +                               .filter(function compatRangesFilter(aItem) {
> +                                 return !!aItem;
> +                               });

aItem => !!aItem
Attachment #8665397 - Flags: review?(gijskruitbosch+bugs) → review+
(more generally, I expect that this is all leftovers from when stacks and debuggers didn't deal well with anonymous functions. That's all much better now, and there is generally no reason anymore to name these functions, IMO.)
Attachment #8665398 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8665398 [details]
MozReview Request: Bug 1207497 - Part 2: Remove use of expression closure from tests in toolkit/. r?Gijs

https://reviewboard.mozilla.org/r/20227/#review18203
Thank you for reviewing :)
Replaced all those named function expression with arrow.
(In reply to Jan de Mooij [:jandem] from bug 1207491 comment #44)
> (In reply to Tooru Fujisawa [:arai] from bug 1207491 comment #0)
> >   * property with anonymous function expression, contains `this`
> >     add `return` and braces
> 
> Just to be sure, you know `arguments` is also lexical right? See bug 889158
> and
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/
> Arrow_functions#Lexical_arguments

Fixed wrong replacement in toolkit/mozapps/downloads/tests/unit/test_lowMinutes.js, which uses arguments.
You need to log in before you can comment on or make changes to this bug.