Closed
Bug 1451033
Opened 3 years ago
Closed 3 years ago
Split and extract generic parts of ClientEnvironment.jsm into resusable module
Categories
(Firefox :: Normandy Client, enhancement, P1)
Tracking
()
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: leplatrem, Assigned: leplatrem)
References
Details
Attachments
(1 file, 1 obsolete file)
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•3 years ago
|
Attachment #8964920 -
Flags: feedback?(mcooper)
Assignee | ||
Updated•3 years ago
|
Attachment #8964921 -
Flags: feedback?(mcooper)
Updated•3 years ago
|
Version: 57 Branch → 61 Branch
Updated•3 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Comment 3•3 years ago
|
||
mozreview-review |
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. :-)
Attachment #8964920 -
Flags: review?(gijskruitbosch+bugs)
Comment 4•3 years ago
|
||
mozreview-review |
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. :-)
Attachment #8964921 -
Flags: review?(gijskruitbosch+bugs)
Comment 5•3 years ago
|
||
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.
Assignee | ||
Comment 6•3 years ago
|
||
Thanks for the detailed review! I'll rework the patch your feedback and ideas :)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•3 years ago
|
Attachment #8964921 -
Attachment is obsolete: true
Attachment #8964921 -
Flags: feedback?(mcooper)
Assignee | ||
Comment 8•3 years ago
|
||
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•3 years ago
|
||
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.
See Also: → 1440782
Comment 10•3 years ago
|
||
mozreview-review |
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.
Attachment #8964920 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 11•3 years ago
|
||
Thanks, you're right, I could find other places where `getAppLocaleAsLangTag()` is used without this safety check.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•3 years ago
|
Keywords: checkin-needed
Comment 13•3 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/1071bcc2d649 Extract Normandy ClientEnvironment into reusable module r=Gijs
Keywords: checkin-needed
Comment 14•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1071bcc2d649
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in
before you can comment on or make changes to this bug.
Description
•