1226386 - Switch XPIProvider to use nicer ES6 features

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Comment 1

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1226386: Remove most of the preprocessing from the add-ons manager. r=gps

Bug 1226386: Remove most of the preprocessing from the add-ons manager. r?gps

For build speed, for correct line numbers in errors, for faster development, for so many reasons.
Still a couple of cases left mostly in XUL files for different strings on Windows.

Bonus: The new lexical scope means ADDON_SIGNING and REQUIRE_SIGNING can just
be declared as regular constants and outside code can't get to them easily.

Comment 2

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1226386: Remove createWrapper function and replace with a memoized property. r=rhelmer

Bug 1226386: Remove createWrapper function and replace with a memoized property. r?rhelmer

Comment 3

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1226386: Remove functions names where possible. r?rhelmer

Bug 1226386: Remove functions names where possible. r?rhelmer

We used to need explicit names for functions to make stack traces display
properly. The JS engine is smarter now so doesn't need them and they just
make the code messy and redundant.

Comment 4

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1226386: Switch to fat arrow functions where it makes sense. r?rhelmer

Bug 1226386: Switch to fat arrow functions where it makes sense. r?rhelmer

Both for brevity and to remove the use of |self = this|.

Comment 5

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1226386: Fix call to Utils.catch to pass the correct this and to give the correct this to AddonWrapper.uninstall(). r=gps

Bug 1226386: Fix call to Utils.catch to pass the correct this and to give the correct this to AddonWrapper.uninstall(). r?gps

Comment 6

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1226386: Remove use of non-standard __defineGetter__ and __defineSetter__. r?rhelmer

Bug 1226386: Remove use of non-standard __defineGetter__ and __defineSetter__. r?rhelmer

Moved these mostly onto the prototype. We couldn't do this before without making
the target of the wrapper a property of the wrappers and we don't want to expose
that but now WeakMaps allow us to get the target without exposing it.

Once change with this approach is that when the test suite shuts down the
add-ons manager it kills the map and so wrappers cease to function. A couple of
tests were relying on accessing wrapper properties after that but that would
have likely been unsafe anyway.

Comment 7

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1226386: Remove or fix many uses of Array.forEach. r?rhelmer

Bug 1226386: Remove or fix many uses of Array.forEach. r?rhelmer

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8690957 [details]
MozReview Request: Bug 1226386: Remove most of the preprocessing from the add-ons manager. r=gps

https://reviewboard.mozilla.org/r/26019/#review23451

LGTM. Thank you for the net reduction of preprocessed files, especially in meaningful files that people touch a lot.

Regarding the new JS constants JSM, if this turns out to be a common pattern, we could add support to the build system for writing an auto-genenerated jsm with all these constants.

::: toolkit/mozapps/extensions/AddonManager.jsm:26
(Diff revision 1)
> +const MOZ_COMPATIBILITY_NIGHTLY = !['aurora', 'beta', 'release', 'esr'].includes(AppConstants.MOZ_UPDATE_CHANNEL);

It's been a while since I've touched JS. I thought there was an idiomatic `X in [...]` syntax or `[...].contains(X)` function?

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8690961 [details]
MozReview Request: Bug 1226386: Fix call to Utils.catch to pass the correct this and to give the correct this to AddonWrapper.uninstall(). r=gps

https://reviewboard.mozilla.org/r/26027/#review23453

Oof.

I'm kinda curious how you discovered this bug and what the impact was.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #8)
> Comment on attachment 8690957 [details]
> MozReview Request: Bug 1226386: Remove most of the preprocessing from the
> add-ons manager. r?gps
> 
> https://reviewboard.mozilla.org/r/26019/#review23451
> 
> LGTM. Thank you for the net reduction of preprocessed files, especially in
> meaningful files that people touch a lot.
> 
> Regarding the new JS constants JSM, if this turns out to be a common
> pattern, we could add support to the build system for writing an
> auto-genenerated jsm with all these constants.
> 
> ::: toolkit/mozapps/extensions/AddonManager.jsm:26
> (Diff revision 1)
> > +const MOZ_COMPATIBILITY_NIGHTLY = !['aurora', 'beta', 'release', 'esr'].includes(AppConstants.MOZ_UPDATE_CHANNEL);
> 
> It's been a while since I've touched JS. I thought there was an idiomatic `X
> in [...]` syntax or `[...].contains(X)` function?

Err there is, Array.includes which I use here ;)

(In reply to Gregory Szorc [:gps] from comment #9)
> Comment on attachment 8690961 [details]
> MozReview Request: Bug 1226386: Fix call to Utils.catch to pass the correct
> this and to give the correct this to AddonWrapper.uninstall(). r?gps
> 
> https://reviewboard.mozilla.org/r/26027/#review23453
> 
> Oof.
> 
> I'm kinda curious how you discovered this bug and what the impact was.

The existing code calls addon.uninstall() as a bare function, i.e. |this| isn't bound to anything. That works right now but a later patches requires |this| to be correctly bound causing an exception when called otherwise making us attempt to call this._log.debug() in Utils.catch which throws because |this| wasn't bound correctly for that call either. Make sense? ;)

Comment 11

[Robert Helmer [:rhelmer]]

Comment on attachment 8690958 [details]
MozReview Request: Bug 1226386: Remove createWrapper function and replace with a memoized property. r=rhelmer

https://reviewboard.mozilla.org/r/26021/#review23487

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:5341
(Diff revision 1)
> -    //let newIcon = createWrapper(this.addon).iconURL;
> +    //let newIcon = this.addon.wrapper.iconURL;

Is there a reason to leave this commented-out code here and have to maintain it? :)

Comment 12

[Robert Helmer [:rhelmer]]

Comment on attachment 8690959 [details]
MozReview Request: Bug 1226386: Remove functions names where possible. r?rhelmer

https://reviewboard.mozilla.org/r/26023/#review23505

::: toolkit/mozapps/extensions/nsBlocklistService.js:451
(Diff revision 1)
>                                                                       aAddon) {

move to previous line?

Comment 13

[Robert Helmer [:rhelmer]]

Comment on attachment 8690960 [details]
MozReview Request: Bug 1226386: Switch to fat arrow functions where it makes sense. r?rhelmer

https://reviewboard.mozilla.org/r/26025/#review23527

::: toolkit/mozapps/extensions/AddonManager.jsm:2203
(Diff revision 1)
> -        aInstall.cancel();
> +        i.cancel();

Should this be `install.cancel()` instead of `i.cancel()`?

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8690957 [details]
MozReview Request: Bug 1226386: Remove most of the preprocessing from the add-ons manager. r=gps

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

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 8690958 [details]
MozReview Request: Bug 1226386: Remove createWrapper function and replace with a memoized property. r=rhelmer

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

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 8690959 [details]
MozReview Request: Bug 1226386: Remove functions names where possible. r?rhelmer

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

Comment 17

[Dave Townsend [:mossop]]

Comment on attachment 8690960 [details]
MozReview Request: Bug 1226386: Switch to fat arrow functions where it makes sense. r?rhelmer

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

Comment 18

[Dave Townsend [:mossop]]

Comment on attachment 8690961 [details]
MozReview Request: Bug 1226386: Fix call to Utils.catch to pass the correct this and to give the correct this to AddonWrapper.uninstall(). r=gps

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

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 8690962 [details]
MozReview Request: Bug 1226386: Remove use of non-standard __defineGetter__ and __defineSetter__. r?rhelmer

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

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 8690963 [details]
MozReview Request: Bug 1226386: Remove or fix many uses of Array.forEach. r?rhelmer

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

Comment 21

[Robert Helmer [:rhelmer]]

Comment on attachment 8690959 [details]
MozReview Request: Bug 1226386: Remove functions names where possible. r?rhelmer

https://reviewboard.mozilla.org/r/26023/#review23851

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3770
(Diff revision 2)
>                                                    aVersion, aBrowser, aCallback) {

formatting nit, align these args with the function above.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4287
(Diff revision 2)
>                                                        aMultiprocessCompatible,
>                                                        aRunInSafeMode) {

formatting, argument alignment

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:1298
(Diff revision 2)
>                                                            aDescriptor) {

formatting, argument alignment

lgtm, just a few formatting nits.

Comment 22

[Robert Helmer [:rhelmer]]

Comment on attachment 8690962 [details]
MozReview Request: Bug 1226386: Remove use of non-standard __defineGetter__ and __defineSetter__. r?rhelmer

https://reviewboard.mozilla.org/r/26029/#review23853

Comment 23

[Robert Helmer [:rhelmer]]

Comment on attachment 8690963 [details]
MozReview Request: Bug 1226386: Remove or fix many uses of Array.forEach. r?rhelmer

https://reviewboard.mozilla.org/r/26031/#review23855

Comment 24

[Robert Helmer [:rhelmer]]

Comment on attachment 8690960 [details]
MozReview Request: Bug 1226386: Switch to fat arrow functions where it makes sense. r?rhelmer

https://reviewboard.mozilla.org/r/26025/#review23863

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f47a7466e36b
https://hg.mozilla.org/integration/fx-team/rev/976f9118f11b
https://hg.mozilla.org/integration/fx-team/rev/067cde34cbad
https://hg.mozilla.org/integration/fx-team/rev/0fb853fdfcc8
https://hg.mozilla.org/integration/fx-team/rev/1462234985f0
https://hg.mozilla.org/integration/fx-team/rev/7dde3c932d01
https://hg.mozilla.org/integration/fx-team/rev/b21eff7211f4

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f47a7466e36b
https://hg.mozilla.org/mozilla-central/rev/976f9118f11b
https://hg.mozilla.org/mozilla-central/rev/067cde34cbad
https://hg.mozilla.org/mozilla-central/rev/0fb853fdfcc8
https://hg.mozilla.org/mozilla-central/rev/1462234985f0
https://hg.mozilla.org/mozilla-central/rev/7dde3c932d01
https://hg.mozilla.org/mozilla-central/rev/b21eff7211f4