Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) Assignee
	Description • 10 years ago

Similar to bug 996448 in mozilla-central, this bug is about lazily loading PluralForm.jsm

	Philipp Kewisch [:Fallen][📱📆]
	Comment 5 • 10 years ago

Comment on attachment 8569972 [details] [diff] [review]
calendar, v1

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

r=philipp with this comment considered:

::: calendar/base/content/calendar-task-tree.xml
@@ +117,5 @@
>          Components.utils.import("resource://gre/modules/Services.jsm");
>          Components.utils.import("resource://calendar/modules/calItemUtils.jsm");
>          Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +        XPCOMUtils.defineLazyModuleGetter(this, "PluralForm",
> +                                          "resource://gre/modules/PluralForm.jsm");

passing |this| here will define the getter on the binding instance. I think you will have to use Components.getGlobalForObject(this) instead, or maybe just pass |window|

	neil@parkwaycc.co.uk
	Comment 6 • 10 years ago

Comment on attachment 8569978 [details] [diff] [review]
suite, v1

Nit: missed one in navigator.js

	aleth [:aleth]
	Comment 7 • 10 years ago

Comment on attachment 8569974 [details] [diff] [review]
chat/im, v1

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

::: mail/components/im/modules/chatNotifications.jsm
@@ +11,2 @@
>  Cu.import("resource://gre/modules/Timer.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

I don't think you need this import, as imServices already imports it?

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) Assignee
	Comment 8 • 10 years ago

(In reply to neil@parkwaycc.co.uk from comment #6)
> Nit: missed one in navigator.js
That's part of
> __defineGetter__("PluralForm", function() {
>   Components.utils.import("resource://gre/modules/PluralForm.jsm");
>   return this.PluralForm;
> });
and only used at https://dxr.mozilla.org/comm-central/source/suite/common/bookmarks/browser-places.js#176

Doesn't this get only loaded when it's called?

(In reply to aleth [:aleth] from comment #7)
> ::: mail/components/im/modules/chatNotifications.jsm
> @@ +11,2 @@
> >  Cu.import("resource://gre/modules/Timer.jsm");
> > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> 
> I don't think you need this import, as imServices already imports it?
But it doesn't export it (it is itself a module). Or did you mean im/content/extensions.js?

	aleth [:aleth]
	Comment 9 • 10 years ago

(In reply to Archaeopteryx [:aryx] from comment #8)
> (In reply to aleth [:aleth] from comment #7)
> > ::: mail/components/im/modules/chatNotifications.jsm
> > @@ +11,2 @@
> > >  Cu.import("resource://gre/modules/Timer.jsm");
> > > +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> > 
> > I don't think you need this import, as imServices already imports it?
> But it doesn't export it (it is itself a module). Or did you mean
> im/content/extensions.js?

Ah, you're right.