Bootstrapless addon #3456

meandavejustice · 2018-03-30T16:15:46Z

Rewrite of addon as webextension

Functionality:

add button to toolbar which links to testpilot based on the currentEnv (local/prod etc...)
toolbar link passes correct utm params
show notification when new experiments or Major news updates are added to the site.
submit a daily ping with a list of currently installed experiments

Structure:

got rid of all preprocessing of files
everything is in addon/background.js or options/options.js
npm scripts in package.json

Issues:

- refs mozilla#3182

- fixes mozilla#3176

- options page is not showing up yet, I have a hunch that this may be because of the addon is still a bootstrap hyrid, something seems to be blocking the loading of the options.html page. Going to keep investigating, but wanted to commit to get these changes pushed to remote.

meandavejustice · 2018-03-30T17:51:45Z

addon/background.js

+  });
+}
+
+async function setup() {


This is the starting point for the webextension.

Here we:

fetch or generate a clientUUID(used to daily metric ping)

setupEnvironment(setup listener for changing of addon environement(local/dev/stage/prod)

setup our browser action button

populate resources (experiments and news feed)

detect our installed test pilot addons and add listeners to keep that updated on install/uninstall events.

setup our daily ping interval

meandavejustice · 2018-03-30T18:13:23Z

addon/background.js

+  });
+}
+
+function submitPing(object, event, addons) {


Telemetry and ping centre are no longer called in the new webextension. We're only using google analytics now.

meandavejustice · 2018-03-30T18:19:19Z

addon/background.js

@@ -0,0 +1,206 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


This is where everything happens for the webextension. Most of what is in there was pulled from src/webextension/lib/{browserAction, storage} and src/lib/pings.

meandavejustice · 2018-03-30T18:20:08Z

addon/bin/update-version

-var manifestJSON = JSON.stringify(manifest, null, '  ');
-fs.writeFileSync(manifestFilename, manifestJSON);
-
-var installFilename = __dirname + '/../build/install.rdf';


This file is virtually the same except for generation of install.rdf was removed.

meandavejustice · 2018-03-30T18:20:45Z

addon/manifest.json

@@ -16,6 +17,7 @@
  },
  "default_locale": "en_US",
  "permissions": [
+    "management",


used for tracking addon installs/uninstalls

meandavejustice · 2018-03-30T18:21:56Z

addon/options/options.js

@@ -0,0 +1,26 @@
+/* global browser */


Environments came from src/webextension/lib/environments.js. They are now part of the options page as we no longer have access to about:config prefs.

meandavejustice · 2018-03-30T18:23:22Z

addon/src/chrome/scripts/frame-script.js

@@ -1,108 +0,0 @@
-/*


No longer use frame scripts for detecting test pilot addon on certain mozilla properties

meandavejustice · 2018-03-30T18:23:53Z

addon/src/install.rdf

@@ -1,26 +0,0 @@
-<?xml version="1.0" encoding="utf-8"?>


bootstrap-removal

meandavejustice · 2018-03-30T18:24:39Z

addon/src/lib/addonManager.js

@@ -1,66 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


now using the management api to fetch addons and determine which txp addons are installed at the time of the daily ping.

meandavejustice · 2018-03-30T18:25:22Z

addon/src/lib/appStartup.js

@@ -1,30 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


bootstrap-removal, sessionstore-windows-restored was a hack to speed up startup for hybrid bootstrap embedded webextensions.

meandavejustice · 2018-03-30T18:26:10Z

addon/src/lib/channels.js

@@ -1,176 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


bootstrap-removal, no longer provide metrics channel for experiments

meandavejustice · 2018-03-30T18:26:43Z

addon/src/lib/events.js

@@ -1,87 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


bootstrap-removal, this was a bridge for talking to the embedded webextension

meandavejustice · 2018-03-30T18:27:26Z

addon/src/lib/frameScripts.js

@@ -1,63 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


removal of frame scripts, see https://github.com/mozilla/testpilot/pull/3456/files#r178343655

meandavejustice · 2018-03-30T18:28:21Z

addon/src/lib/pings.js

@@ -1,100 +0,0 @@
-// Shared metrics ping formatting & logic for bootstrap and webextension, with


ga ping snippet pulled for background.js. Everything else removed.

meandavejustice · 2018-03-30T18:28:51Z

addon/src/lib/prefs.js

@@ -1,53 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


Bootstrap-removal, no longer using prefs

meandavejustice · 2018-03-30T18:29:05Z

addon/src/lib/telemetry.js

@@ -1,126 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


bootstrap-removal, no more telemetry

meandavejustice · 2018-03-30T18:29:48Z

addon/src/lib/topics.js

@@ -1,65 +0,0 @@
-// Shared list of topics for PubSub usage in bootstrap and webextension


bootstrap-removal, part of pubsub bridge to webextension. no longer needed

meandavejustice · 2018-03-30T18:30:12Z

addon/src/lib/utils.js

@@ -1,17 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


log is commented out for development use in background.js

meandavejustice · 2018-03-30T18:31:10Z

addon/src/lib/webExtension.js

@@ -1,83 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


bootstrap-removal, embedded webextension management and bridge

meandavejustice · 2018-03-30T18:38:33Z

addon/src/webextension/lib/browserAction.js

@@ -1,102 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


almost this whole file is now in background.js

meandavejustice · 2018-03-30T18:38:53Z

addon/src/webextension/background.js

@@ -1,36 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


bootstrap-removal

meandavejustice · 2018-03-30T18:40:03Z

addon/src/webextension/lib/bootstrap.js

@@ -1,28 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


bootstrap-removal

meandavejustice · 2018-03-30T18:41:55Z

addon/src/webextension/lib/metrics.js

@@ -1,148 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


bootstrap-removal, daily-ping was written fresh using browser.alarms in background.js

meandavejustice · 2018-03-30T18:42:13Z

addon/src/webextension/lib/storage.js

@@ -1,23 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


now in background.js

meandavejustice · 2018-03-30T18:42:23Z

addon/src/webextension/lib/utils.js

@@ -1,14 +0,0 @@
-/* This Source Code Form is subject to the terms of the Mozilla Public


bootstrap-removal

meandavejustice · 2018-03-30T18:42:33Z

addon/test/test-setup.js

@@ -1 +0,0 @@
-// Global setup for all tests


meandavejustice · 2018-03-30T18:42:56Z

addon/webpack.config.js

-      // HACK: Only perform bundling if the second half of build has been
-      // performed, but we still want to rebuild & post the first half on file
-      // watching.
-      const buildPackagePath = './build/package.json';


moved into npm scripts

- fixes mozilla#3382 - fixes mozilla#3183 - fixes mozilla#3176 - fixes mozilla#3177 - fixes mozilla#3182 - fixes mozilla#3180 - fixes mozilla#3313

meandavejustice · 2018-03-30T20:59:50Z

I seem to be having an issue with the update-version script and web-ext.

It appears that npm is cacheing $npm_package_version which is breaking the build.

Building web extension from /home/circleci/testpilot/addon
Your web extension is ready: /home/circleci/testpilot/addon/web-ext-artifacts/test_pilot-3.0.0-dev-6951b1b.zip
mv: cannot stat ‘web-ext-artifacts/test_pilot-3.0.0-dev-1207ff1.zip’: No such file or directory

I'm going to remove update-version from the prepackage script in order to get this working. Filed an issue to check into it after I can talk to the author(les) #3459

…on install

dannycoates · 2018-03-30T22:28:02Z

addon/background.js

+  browser.management.onEnabled.addListener(setInstalledTxpAddons);
+  browser.management.onDisabled.addListener(setInstalledTxpAddons);
+  browser.management.onInstalled.addListener(setInstalledTxpAddons);
+  browser.management.onUninstalled.addListener(setInstalledTxpAddons);


It looks like we only use installedTxpAddons in the daily ping. It might be more polite to just run setInstalledTxpAddons then. Not a blocker. Your call.

dannycoates · 2018-03-30T22:28:33Z

addon/background.js

+  browser.storage.onChanged.addListener((changes) => {
+    Object.keys(changes).forEach((k) => {
+      if (k === "environment") {
+        currentEnvironment = changes[k];


should be changes[k].newValue

dannycoates · 2018-03-30T22:37:39Z

addon/background.js

+  const delayInMinutes = 5;
+  const periodInMinutes = 60 * 24; // daily
+
+  browser.alarms.create("daily-ping", {


So this will fire after 5 minutes every time I restart the browser, which might be ok? (not my call) If we really only want at most one ping per day we should save a timestamp to storage so we can check it on startup.

dannycoates · 2018-03-30T22:38:50Z

addon/background.js

+  log("fetchResources");
+  return Promise.all(
+    Object.keys(resources).map(path =>
+                               fetch(`${currentEnvironment.baseUrl}/api/${path}.json`)


nit: this indenting is hard to read

dannycoates · 2018-03-30T22:44:47Z

addon/background.js

+    text: (newExperiments.length || newsUpdates.length) > 0 ? "!" : ""
+  });
+
+  browser.browserAction.setBadgeText({


idk why this is here twice. bad merge?

dannycoates · 2018-03-30T22:46:22Z

addon/options/options.html

+        <option value="https://testpilot.dev.mozaws.net">Development</option>
+        <option value="https://testpilot-l10n.dev.mozaws.net">L10n</option>
+        <option value="https://testpilot.stage.mozaws.net">Stage</option>
+        <option value="https://testpilot.firefox.com">Production</option>


since production is the default in background.js I think it should be the one initialized to selected here

dannycoates · 2018-03-30T23:00:05Z

addon/background.js

+  const twoWeeksAgo = Date.now() - TWO_WEEKS;
+  const newsUpdates = (news_updates || []).filter((u) => u.major)
+        .filter((u) => new Date(u.published).getTime() >= twoWeeksAgo)
+        .filter((u) => new Date(u.published).getTime() >= new Date(lastViewed))


nit: lastViewed shouldn't need a Date wrapper

dannycoates · 2018-03-31T01:46:58Z

addon/background.js

+async function setup() {
+  const data = await storage.get("clientUUID");
+  if (!data.clientUUID) {
+    await storage.set({ clientUUID: clientUUID = uuidv4() });


your let clientUUID above doesn't get initialized once it's already stored

dannycoates · 2018-03-31T01:48:34Z

addon/manifest.json

@@ -16,6 +17,7 @@
  },
  "default_locale": "en_US",
  "permissions": [
+    "management",


needs "alarms" permission

dannycoates · 2018-03-31T01:55:30Z

addon/background.js

+}
+
+const storage = browser.storage.local;
+const RESOURCE_UPDATE_INTERVAL = 10000; // 4 hours


10000 isn't actually 4 hours ;)

oh that was a testing mistake. forgot to change, thanks for catching that!

- remove unneccessary date wrapper - fix daily ping to test date before sending request - indent cleanup - change default in options.html to local - fix setting of environment from onchanged listener

dannycoates · 2018-04-02T16:47:58Z

addon/background.js

+                                              (c ^ crypto.getRandomValues(new Uint8Array(1))[0] & 15 >> c / 4).toString(16));
+}
+
+function setInstalledTxpAddons() {


nit: it might be better to call this getInstalledTxpAddons now

dannycoates · 2018-04-02T16:52:31Z

addon/background.js

+      const ONE_DAY = 60 * 60 * 1000 * 24; /* ms */
+      const { lastPing } = await storage.get("lastPing");
+      if (initial || ((new Date) - lastPing) > ONE_DAY) {
+        setInstalledTxpAddons().then((installedTxpAddons) => {


nit: await instead of then

dannycoates · 2018-04-02T16:53:08Z

addon/background.js

+    periodInMinutes
+  });
+
+  browser.alarms.onAlarm.addListener((alarmInfo) => {


needs async on the callback

dannycoates · 2018-04-02T16:54:51Z

addon/background.js

+  setupEnvironment();
+  setupBrowserAction();
+  await fetchResources();
+  setDailyPing();


should have await for consistency

dannycoates · 2018-04-02T17:11:47Z

addon/background.js

+  const { clientUUID } = await storage.get("clientUUID");
+  if (!clientUUID) {
+    await storage.set({ clientUUID: uuidv4() });
+  }


putting all the clientUUID code in setDailyPing is more cohesive

dannycoates · 2018-04-02T17:14:14Z

addon/background.js

+      if (initial || ((new Date) - lastPing) > ONE_DAY) {
+        setInstalledTxpAddons().then((installedTxpAddons) => {
+          const { clientUUID } = await storage.get({ clientUUID: uuidv4() });
+          submitPing(alarmInfo.name, "installed-addons", installedTxpAddons, clientUUID);


lastPing needs to be set after one is sent

dannycoates · 2018-04-02T17:15:31Z

addon/background.js

+  if (!lastPing) {
+    initial = true;
+    await storage.set({ lastPing: Date.now()});
+  }


we can remove initial and skip this if we default lastPing to yesterday

woah clever 👍

dannycoates · 2018-04-02T17:59:12Z

addon/background.js

+
+async function sendPingOnAlarm(alarmInfo) {
+  if (alarmInfo.name === "daily-ping") {
+    const { lastPing } = await storage.get("lastPing");


I was thinking more along the lines of storage.get({ lastPing: Date.now() - ONE_DAY })

I don't think this would ever return successfully 🤔

dannycoates · 2018-04-02T18:06:06Z

addon/background.js

-      }
-    }
-  });
+  browser.alarms.onAlarm.addListener(sendPingOnAlarm);


codecov-io · 2018-04-02T18:11:34Z

Codecov Report

Merging #3456 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3456   +/-   ##
=======================================
  Coverage   83.48%   83.48%           
=======================================
  Files         113      113           
  Lines        3172     3172           
=======================================
  Hits         2648     2648           
  Misses        524      524

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3da7f5...0f7af11. Read the comment docs.

meandavejustice requested review from fzzzy and dannycoates March 30, 2018 16:15

dave justice added 4 commits March 30, 2018 12:16

Remove appStartup

f834bfe

- refs mozilla#3182

Remove Frame scripts

a540823

- fixes mozilla#3176

Copy over options to build dir in webpack.config

7d255b7

meandavejustice force-pushed the bootstrapless-addon branch from 163f8fb to 1207ff1 Compare March 30, 2018 16:19

meandavejustice mentioned this pull request Mar 30, 2018

review testpilot-metrics package and metrics docs #3457

Closed

meandavejustice commented Mar 30, 2018

View reviewed changes

addon/src/install.rdf

@@ -1,26 +0,0 @@

<?xml version="1.0" encoding="utf-8"?>

Copy link

Contributor Author

meandavejustice Mar 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bootstrap-removal

meandavejustice commented Mar 30, 2018

View reviewed changes

addon/test/test-setup.js

@@ -1 +0,0 @@

// Global setup for all tests

Copy link

Contributor Author

meandavejustice Mar 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

empty file

meandavejustice commented Mar 30, 2018

View reviewed changes

Full conversion of addon to webextension

6951b1b

- fixes mozilla#3382 - fixes mozilla#3183 - fixes mozilla#3176 - fixes mozilla#3177 - fixes mozilla#3182 - fixes mozilla#3180 - fixes mozilla#3313

meandavejustice force-pushed the bootstrapless-addon branch from 1207ff1 to 6951b1b Compare March 30, 2018 19:41

dave justice added 3 commits March 30, 2018 17:16

Remove update-version from prepackage script

bdcc9d7

Attempting fix for integration test, detecting web ext notifications …

a8d841a

…on install

next pass as fixing install integration test

e3f3f64

dannycoates reviewed Mar 30, 2018

View reviewed changes

try same notification as experiment install

d4171fc

dannycoates reviewed Mar 31, 2018

View reviewed changes

addon/manifest.json Outdated

@@ -16,6 +17,7 @@

},

"default_locale": "en_US",

"permissions": [

"management",

Copy link

dannycoates Mar 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs "alarms" permission

dannycoates reviewed Mar 31, 2018

View reviewed changes

dave justice added 2 commits April 2, 2018 11:33

Address review comments

e81b7fa

- remove unneccessary date wrapper - fix daily ping to test date before sending request - indent cleanup - change default in options.html to local - fix setting of environment from onchanged listener

add alarm permission, clientUUID storage, resource fetch interval

069e088

ghost mentioned this pull request Apr 2, 2018

Whitelist about:newtab #3451

Closed

dannycoates reviewed Apr 2, 2018

View reviewed changes

dave justice added 2 commits April 2, 2018 13:40

organize dailyPing, clientUUID, set=>getInstalltxpAddons

052099f

fix indentation for flake8

f912eee

dannycoates reviewed Apr 2, 2018

View reviewed changes

add confirmation notifcation handling to experiment page test

1f9864d

dannycoates reviewed Apr 2, 2018

View reviewed changes

addon/background.js Outdated

}

}

});

browser.alarms.onAlarm.addListener(sendPingOnAlarm);

Copy link

dannycoates Apr 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

cleanup lastPing defaults and remove async from setDailyPing

0f7af11

meandavejustice merged commit 6bf5cda into mozilla:master Apr 2, 2018

meandavejustice deleted the bootstrapless-addon branch April 2, 2018 18:44

		@@ -0,0 +1,206 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,66 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,30 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,176 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,87 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,63 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,100 +0,0 @@
		// Shared metrics ping formatting & logic for bootstrap and webextension, with

		@@ -1,53 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,126 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,65 +0,0 @@
		// Shared list of topics for PubSub usage in bootstrap and webextension

		@@ -1,17 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,83 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,102 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,36 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,28 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,148 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,23 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -1,14 +0,0 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

Bootstrapless addon #3456

Bootstrapless addon #3456

Conversation

meandavejustice commented Mar 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meandavejustice commented Mar 30, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 2, 2018 • edited

Codecov Report

meandavejustice commented Mar 30, 2018 •

edited

meandavejustice commented Mar 30, 2018 •

edited

codecov-io commented Apr 2, 2018 •

edited