Closed Bug 1468025 Opened 2 years ago Closed 2 years ago

Lazy load TCPListener in Marionette component

Categories

(Testing :: Marionette, enhancement)

enhancement
Not set

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

Attachments

(1 file)

We can move the import of testing/marionette/server.js’ TCPListener
to a lazy import to avoid having to define it inline.
Assignee: nobody → ato
Status: NEW → ASSIGNED
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?
Attachment #8984692 - Flags: review?(hskupin)
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
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.
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
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.
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.
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.
Comment on attachment 8984692 [details]
Bug 1468025 - Define lazy getter for TCPListener in Marionette component.

https://reviewboard.mozilla.org/r/250544/#review257574
Attachment #8984692 - Flags: review?(hskupin) → review+
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e77ceef3485b
Define lazy getter for TCPListener in Marionette component. r=whimboo
https://hg.mozilla.org/mozilla-central/rev/e77ceef3485b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.