Switch XPIProvider to use nicer ES6 features

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(7 attachments)

No description provided.
Assignee: nobody → dtownsend
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.
Attachment #8690957 - Flags: review?(gps)
Bug 1226386: Remove createWrapper function and replace with a memoized property. r?rhelmer
Attachment #8690958 - Flags: review?(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.
Attachment #8690959 - Flags: review?(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|.
Attachment #8690960 - Flags: review?(rhelmer)
Bug 1226386: Fix call to Utils.catch to pass the correct this and to give the correct this to AddonWrapper.uninstall(). r?gps
Attachment #8690961 - Flags: review?(gps)
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.
Attachment #8690962 - Flags: review?(rhelmer)
Bug 1226386: Remove or fix many uses of Array.forEach. r?rhelmer
Attachment #8690963 - Flags: review?(rhelmer)
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?
Attachment #8690957 - Flags: review?(gps) → review+
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.
Attachment #8690961 - Flags: review?(gps) → review+
(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? ;)
Attachment #8690958 - Flags: review?(rhelmer) → review+
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 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?
Attachment #8690959 - Flags: review?(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()`?
Attachment #8690960 - Flags: review?(rhelmer)
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/
Attachment #8690957 - Attachment description: MozReview Request: Bug 1226386: Remove most of the preprocessing from the add-ons manager. r?gps → MozReview Request: Bug 1226386: Remove most of the preprocessing from the add-ons manager. r=gps
Attachment #8690958 - Attachment description: MozReview Request: Bug 1226386: Remove createWrapper function and replace with a memoized property. r?rhelmer → MozReview Request: Bug 1226386: Remove createWrapper function and replace with a memoized property. r=rhelmer
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 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/
Attachment #8690959 - Flags: review?(rhelmer)
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/
Attachment #8690960 - Flags: review?(rhelmer)
Attachment #8690961 - Attachment description: 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 → 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
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 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 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/
Attachment #8690959 - Flags: review?(rhelmer) → review+
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 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
Attachment #8690962 - Flags: review?(rhelmer) → review+
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
Attachment #8690963 - Flags: review?(rhelmer) → review+
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
Attachment #8690960 - Flags: review?(rhelmer) → review+
You need to log in before you can comment on or make changes to this bug.