Andreas Tolfsen ❲:ato❳ Assignee
	Description • 6 years ago

We can move the import of testing/marionette/server.js’ TCPListener
to a lazy import to avoid having to define it inline.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 2 • 6 years ago
mozreview-review

Comment on attachment 8984692 [details]
Bug 1468025 - Define lazy getter for TCPListener in Marionette component.

https://reviewboard.mozilla.org/r/250544/#review256906

::: testing/marionette/components/marionette.js:18
(Diff revision 1)
>  } = ChromeUtils.import("chrome://marionette/content/prefs.js", {});
> +
>  ChromeUtils.defineModuleGetter(this, "Preferences",
>      "resource://gre/modules/Preferences.jsm");
> +ChromeUtils.defineModuleGetter(this, "Log",
> +    "resource://gre/modules/Log.jsm");

nit: Please put before Preferences.

Also shouldn't we keep the global imports separated in it's own block at the top? Why this change?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 6 years ago
mozreview-review-reply

Comment on attachment 8984692 [details]
Bug 1468025 - Define lazy getter for TCPListener in Marionette component.

https://reviewboard.mozilla.org/r/250544/#review256906

> nit: Please put before Preferences.
> 
> Also shouldn't we keep the global imports separated in it's own block at the top? Why this change?

I don’t think this matters?

I’m happy to re-organise the list in alphebetical order, but I
really don’t see the pupose

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 4 • 6 years ago
mozreview-review-reply

Comment on attachment 8984692 [details]
Bug 1468025 - Define lazy getter for TCPListener in Marionette component.

https://reviewboard.mozilla.org/r/250544/#review256906

> I don’t think this matters?
> 
> I’m happy to re-organise the list in alphebetical order, but I
> really don’t see the pupose

One could make the argument that it does not make any difference to separate global and local imports, were it not for organisational reasons.  The lazy getters are differenct, by definition, because they explicitly _do not_ get loaded in order.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 5 • 6 years ago
mozreview-review-reply

Comment on attachment 8984692 [details]
Bug 1468025 - Define lazy getter for TCPListener in Marionette component.

https://reviewboard.mozilla.org/r/250544/#review256906

> One could make the argument that it does not make any difference to separate global and local imports, were it not for organisational reasons.  The lazy getters are differenct, by definition, because they explicitly _do not_ get loaded in order.

> I’m happy to re-organise the list in alphebetical order, but I really don’t see the pupose

We always sorted imports like that. So I don't see why we should change that now.

Also please note that when multiple getters are needed we could also use `XPCOMUtils.defineLazyModuleGetter` which makes it way more friendlier to read. For example see: https://dxr.mozilla.org/mozilla-central/rev/8ab6afabc78cd909ff90ba74c1eab098985f83ef/browser/components/places/PlacesUIUtils.jsm#14-24

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 6 • 6 years ago
mozreview-review-reply

Comment on attachment 8984692 [details]
Bug 1468025 - Define lazy getter for TCPListener in Marionette component.

https://reviewboard.mozilla.org/r/250544/#review256906

> > I’m happy to re-organise the list in alphebetical order, but I really don’t see the pupose
> 
> We always sorted imports like that. So I don't see why we should change that now.
> 
> Also please note that when multiple getters are needed we could also use `XPCOMUtils.defineLazyModuleGetter` which makes it way more friendlier to read. For example see: https://dxr.mozilla.org/mozilla-central/rev/8ab6afabc78cd909ff90ba74c1eab098985f83ef/browser/components/places/PlacesUIUtils.jsm#14-24

> We always sorted imports like that. So I don't see why we should
> change that now.

The list has been reordered, but I strongly feel it wasn’t necessary
to block the patch from landing for this reason.

> Also shouldn't we keep the global imports separated in it's own
> block at the top? Why this change?

They aren’t global imports: they are lazy getters.  If you look
at the PlacesUIUtils.jsm you linked, this is exactly how it is
organised here as well.

> Also please note that when multiple getters are needed we could
> also use XPCOMUtils.defineLazyModuleGetter which makes it way more
> friendlier to read.

This isn’t using XPCOMUtils, but ChromeUtils, where
defineModuleGetters (plural) does not exist.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 8 • 6 years ago
mozreview-review-reply

Comment on attachment 8984692 [details]
Bug 1468025 - Define lazy getter for TCPListener in Marionette component.

https://reviewboard.mozilla.org/r/250544/#review256906

> > We always sorted imports like that. So I don't see why we should
> > change that now.
> 
> The list has been reordered, but I strongly feel it wasn’t necessary
> to block the patch from landing for this reason.
> 
> > Also shouldn't we keep the global imports separated in it's own
> > block at the top? Why this change?
> 
> They aren’t global imports: they are lazy getters.  If you look
> at the PlacesUIUtils.jsm you linked, this is exactly how it is
> organised here as well.
> 
> > Also please note that when multiple getters are needed we could
> > also use XPCOMUtils.defineLazyModuleGetter which makes it way more
> > friendlier to read.
> 
> This isn’t using XPCOMUtils, but ChromeUtils, where
> defineModuleGetters (plural) does not exist.

> They aren’t global imports: they are lazy getters.  If you look
> at the PlacesUIUtils.jsm you linked, this is exactly how it is
> organised here as well.

As it looks like globals in this case aren't js modules from the browser itself, but objects like `URL` and `Elements`. So it's actually fine

> > This isn’t using XPCOMUtils, but ChromeUtils, where
> defineModuleGetters (plural) does not exist.

Sure and this is clear. But I actually mentioned the wrong method here while referencing the correct code in `PlacesUIUtils.jsm`. So we should be able to make use of `XPCOMUtils.defineLazyModuleGetters()` here.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 9 • 6 years ago
mozreview-review-reply

Comment on attachment 8984692 [details]
Bug 1468025 - Define lazy getter for TCPListener in Marionette component.

https://reviewboard.mozilla.org/r/250544/#review256906

> > They aren’t global imports: they are lazy getters.  If you look
> > at the PlacesUIUtils.jsm you linked, this is exactly how it is
> > organised here as well.
> 
> As it looks like globals in this case aren't js modules from the browser itself, but objects like `URL` and `Elements`. So it's actually fine
> 
> > > This isn’t using XPCOMUtils, but ChromeUtils, where
> > defineModuleGetters (plural) does not exist.
> 
> Sure and this is clear. But I actually mentioned the wrong method here while referencing the correct code in `PlacesUIUtils.jsm`. So we should be able to make use of `XPCOMUtils.defineLazyModuleGetters()` here.

> Sure and this is clear. But I actually mentioned the
> wrong method here while referencing the correct code in
> PlacesUIUtils.jsm. So we should be able to make use of
> XPCOMUtils.defineLazyModuleGetters() here.

Oh I see.  For the Marionette component I think you are right it
probably makes sense to convert these to lazy getters, since this
component is loaded a lot of times when Marionette support isn’t
really required.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 11 • 6 years ago
mozreview-review

Comment on attachment 8984692 [details]
Bug 1468025 - Define lazy getter for TCPListener in Marionette component.

https://reviewboard.mozilla.org/r/250544/#review257574

	Pulsebot
	Comment 12 • 6 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e77ceef3485b
Define lazy getter for TCPListener in Marionette component. r=whimboo

	Cristian Brindusan [:cbrindusan]
	Comment 13 • 6 years ago
bugherder

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