1384652 - Sync shield-recipe-client from GitHub (v61, commit e54228)

Status: RESOLVED INCOMPLETE

(Firefox :: Normandy Client, enhancement)

Description

[Michael Cooper [:mythmon]]

+++ This bug was initially created as a clone of Bug #1359655 +++

This is mostly an incremental change, not adding any major features. The biggest change is a change in how we load 3rd party libraries: They are now stored as webpack-built minified code that can be Cu.import'ed directly.

Relevant PRs since last sync (Commit PR# - Description):

b6c410ed PR #904 - Bug 1383221: Add tests that weren't being run to manifests.
0567564c PR #891 - Fixes #743: Add add-on information to filter expression context.
a9489da1 PR #878 - #749: Add StudyStorage.jsm for storing data about studies
22009dd9 PR #877 - Support loading arbitrary npm dependencies in the add-on via webpack.
04374ab2 PR #736 - Fix mis-placed comment.
7e0f3583 PR #729 - Refactor Storage.jsm into a class.
429188ce PR #723 - recipe-client-addon: Ensure trailing slash on API root.

Comment 1

[Michael Cooper [:mythmon]]

Attachment: Bug 1384652 - Sync shield-recipe-client from GitHub (v61, commit e54228)

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

Comment 2

[Michael Cooper [:mythmon]]

Adding in another PR to make bug 1383338 easier to reason about

a542289 PR #895 - fetch and run recipes early in startup

Comment 3

[Michael Cooper [:mythmon]]

Comment on attachment 8890445 [details]
Bug 1384652 - Sync shield-recipe-client from GitHub (v61, commit e54228)

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

Comment 4

[Robert Helmer [:rhelmer]]

Comment on attachment 8890445 [details]
Bug 1384652 - Sync shield-recipe-client from GitHub (v61, commit e54228)

https://reviewboard.mozilla.org/r/161572/#review167070

Since this already went through reviews on github, I mostly made sure that what I see in my local normandy repo matches up with this, although I did take a look over the changes as well and they seem sensible to me.

::: browser/extensions/shield-recipe-client/lib/StudyStorage.jsm:29
(Diff revision 2)
> +
> +const {utils: Cu} = Components;
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "IndexedDB", "resource://gre/modules/IndexedDB.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "ajv", "resource://shield-recipe-client/vendor/ajv.js");

I don't know much about ajv, but I know we do some JSON schema validation for WebExtensions. I believe it's in http://searchfox.org/mozilla-central/source/toolkit/components/extensions/Schemas.jsm

However this is just an observation not an issue, since I know this already went through review!

::: browser/extensions/shield-recipe-client/test/browser/browser_ClientEnvironment.js:142
(Diff revision 2)
> +
> +add_task(async function isFirstRun() {
> +  let environment = ClientEnvironment.getEnvironment();
> +
> +  // isFirstRun is set to false after the recipe client runs
> +  ok(!environment.isFirstRun, "isFirstRun has a default value");

I actually removed this line in bug 1383338 ... it seems to intermittently fail, I suspect add-on startup is racing the test but can't reproduce :/

Interested to see if this works on try for you.

Comment 5

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s ba801bb17e2b -d 556c779c67b4: rebasing 412428:ba801bb17e2b "Bug 1384652 - Sync shield-recipe-client from GitHub (v61, commit e54228) r=rhelmer" (tip)
merging browser/extensions/shield-recipe-client/install.rdf.in
merging browser/extensions/shield-recipe-client/lib/ClientEnvironment.jsm
merging browser/extensions/shield-recipe-client/lib/RecipeRunner.jsm
merging browser/extensions/shield-recipe-client/lib/ShieldRecipeClient.jsm
merging browser/extensions/shield-recipe-client/test/browser/browser_ClientEnvironment.js
merging browser/extensions/shield-recipe-client/test/browser/browser_PreferenceExperiments.js
merging browser/extensions/shield-recipe-client/test/browser/browser_RecipeRunner.js
merging browser/extensions/shield-recipe-client/test/browser/head.js
warning: conflicts while merging browser/extensions/shield-recipe-client/install.rdf.in! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/extensions/shield-recipe-client/lib/ClientEnvironment.jsm! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/extensions/shield-recipe-client/lib/RecipeRunner.jsm! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/extensions/shield-recipe-client/lib/ShieldRecipeClient.jsm! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/extensions/shield-recipe-client/test/browser/browser_ClientEnvironment.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/extensions/shield-recipe-client/test/browser/browser_PreferenceExperiments.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 6

[Michael Cooper [:mythmon]]

I didn't merge this in a timely fashion, and it is now conflicting. I'll fix the conflicts in a future sync.