1451033 - Split and extract generic parts of ClientEnvironment.jsm into resusable module

Status: RESOLVED FIXED

(Firefox :: Normandy Client, enhancement, P1)

Description

[Mathieu Leplatre [:leplatrem]]

In Remote Settings, we want to mimic how targeting is done in Normandy/Shield Recipe Client.

We would add a ``filter`` JEXL string on our entries, and filter out entries that don't match.

We are interested by most of the attributes in lib/ClientEnvironment.jsm and would like to extract it as a reusable class/module.

Normandy would extend it to add its domain specific attributes.

It looks like components/utils/ClientEnvironment.jsm could be a potential place to put it.

Comment 1

[Mathieu Leplatre [:leplatrem]]

Attachment: Bug 1451033 - Extract Normandy ClientEnvironment into reusable module

Review commit: https://reviewboard.mozilla.org/r/233664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233664/

Comment 2

[Mathieu Leplatre [:leplatrem]]

Attachment: Bug 1451033 - Extract Normandy ClientEnvironment to toolkit/components/utils

Review commit: https://reviewboard.mozilla.org/r/233666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/233666/

Comment 3

[:Gijs (out; back Jun 21; he/him)]

Comment on attachment 8964920 [details]
Bug 1451033 - Extract Normandy ClientEnvironment into reusable module

https://reviewboard.mozilla.org/r/233664/#review239494

Ah, hm. I'm sympathetic to the aims here, but I think we need to make some small adjustments...

1) I think this unfortunately already bitrotted due to https://bugzilla.mozilla.org/show_bug.cgi?id=1446445 .
2) the XPCOMUtils getter would ensure that the getter is deleted and the result of invoking the passed callback is cached for the lifetime of the object. The getters you're creating in this patch don't do that, and obviously the 'class' bit (ie having multiple instances) means that now these properties get accessed more often anyway. It would be good if their values were shared and not looked up more than once, as they won't change... I think the class-y equivalent would be something like:

```js
class Foopy { static get bar() { delete Foopy.bar; console.log('hi'); Foopy.bar = 5; } }
```

which from a quick test seems like it should work. XPCOMUtils can /maybe/ still do the legwork here? I'm not sure... Though thinking about it more, if most of the things on this thing want to be static getters, does it really want to be a class, or does it kind of just want to be a singleton object anyway? Perhaps we want a second, 'thinner' object that does what you envisage the class instances to do, relying on the singleton for information? I'd be curious about mythmon's thoughts here, too. :-)

Comment 4

[:Gijs (out; back Jun 21; he/him)]

Comment on attachment 8964921 [details]
Bug 1451033 - Extract Normandy ClientEnvironment to toolkit/components/utils

https://reviewboard.mozilla.org/r/233666/#review239496

Much less sure about this, I'd like to hear more from mythmon about this. :-)

Comment 5

[Michael Cooper [:mythmon]]

Mathieu and I are on the same team now, and we talked about this general approach at a work week last week. The idea of re-using ClientEnvironment.jsm (and later FilterExpression.jsm) is something I'm interested in. The ultimate goal is to re-use Normandy's filter expression infrastructure in Remote Settings v2, which I think is a really good match.

I haven't had a chance to do an in depth review yet (I'm busy with preference roll-out right now). I'm not sure about the XPCOMUtils vs getter problem. My general impression when we talked about it last week was that the benefit we got from XPCOM here was that items that were never used were never computed. Caching values doesn't seem as interesting to me, as I expect JEXL may do that internally. Maybe this isn't true though, it could be worth testing.

As far as making Normandy's ClientEnvironment a subclass of a BaseClientEnvironment (the second patch), I'm in favor of this general relationship. Normandy has things specific to it that don't make sense for other users of filter expressions (classify client and normandy user id, for example). I proposed it be a parent class/subclass relationship, but I'm open to other ideas if we have to jump through too many hoops to make that work. Another viable thing I could think of would be to keep the same shape as today, and have Normandy add extra XPCOM-controlled fields to the client environment generated by the common version.

Comment 6

[Mathieu Leplatre [:leplatrem]]

Thanks for the detailed review! I'll rework the patch your feedback and ideas :)

Comment 7

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8964920 [details]
Bug 1451033 - Extract Normandy ClientEnvironment into reusable module

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

Comment 8

[Mathieu Leplatre [:leplatrem]]

I made the changes you suggested:
- rebased
- rewrote using class static attributes
- added a test that clarifies the fact that JEXL does not cache context attributes
- added a "cache" Proxy around the ClientEnvironment in the recipe runner
- added a test that clarifies the fact the ClientEnvironment attributes are cached

Please let me know what do you think :)

Thanks in advance!

Comment 9

[:Gijs (out; back Jun 21; he/him)]

Thanks! I meant to get to this today but it might be tomorrow before I manage to review this. I'm not sure if bug 1440782 conflicts with this or not, we should probably keep an eye on this.

Comment 10

[:Gijs (out; back Jun 21; he/him)]

Comment on attachment 8964920 [details]
Bug 1451033 - Extract Normandy ClientEnvironment into reusable module

https://reviewboard.mozilla.org/r/233664/#review241322

r=me, sorry for the delay.

::: toolkit/components/utils/ClientEnvironment.jsm:119
(Diff revision 2)
> +  static get locale() {
> +    if (Services.locale.getAppLocaleAsLangTag) {
> +      return Services.locale.getAppLocaleAsLangTag();
> +    }
> +
> +    return Cc["@mozilla.org/chrome/chrome-registry;1"]
> +      .getService(Ci.nsIXULChromeRegistry)
> +      .getSelectedLocale("global");
> +  }
> +

I think by now we're always shipping `getAppLocaleAsLangTag` - :gandalf can confirm. I think this is legacy from when normandy was a system add-on and needed to be compatible with several Firefox versions. If so, we can remove the feature detection and just return the result of that method, and remove the nsIXULChromeRegistry code.

Comment 11

[Mathieu Leplatre [:leplatrem]]

Thanks, you're right, I could find other places where `getAppLocaleAsLangTag()` is used without this safety check.

Comment 12

[Mathieu Leplatre [:leplatrem]]

Comment on attachment 8964920 [details]
Bug 1451033 - Extract Normandy ClientEnvironment into reusable module

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1071bcc2d649
Extract Normandy ClientEnvironment into reusable module r=Gijs

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1071bcc2d649