Shield study #4

jonathanKingston · 2018-05-16T12:44:55Z

No description provided.

johannhof

I'm leaving some review comments but not approving for now, I'll need a little bit more looking at this, I think.

Overall I don't think there should be any major blockers, this looks solid and works, as far as I can tell.

johannhof · 2018-05-16T19:35:29Z

src/study.html

+    <header>
+      <h1 data-i18n-message="studyTitle"></h1>
+      <form id="disable">
+        <button data-i18n-message="disableButtonText"></button>


This button does not have a hover or active state, which made it very confusing to me whether I could click it or not.

Please add one.

johannhof · 2018-05-16T19:41:03Z

src/study.js

@@ -0,0 +1,67 @@
+async function init() {
+  // TODO speak with gandalf about new i18n lib to use fragments instead of this


I suppose this isn't happening anymore? :(

For a study (and since we can inspect the localizations in here) I can live with this, but it would be great if we could avoid this in the future.

Removed the comment. Yeah the lib isn't ready yet :(

johannhof · 2018-05-17T10:47:07Z

src/study.js

+    });
+  }
+
+  const brandStrings = await browser.experiments.settings.getStrings(["brandShortName", "brandFullName"]);


I don't really mind this (it's pretty clean), but I'm also not sure why you need brandStrings for a Nightly-only experiment :)

Just in case we then reuse this code and roll it out to other releases.

johannhof · 2018-05-17T10:49:20Z

src/background.js

+      url: STUDY_URL
+    });
+    browser.tabs.remove(tabs.map((tab) => tab.id));
+    browser.experiments.notifications.clear("rollout-prompt");


So why are you not uninstalling the study if the user opts out? Is there anything left that this addon does?

I think this is actually quite an important point, considering that the about:addons page says If you don’t want to participate in this study, you can select the Disable button on this page, even if the pref is disabled...

Then there's also the question whether you should flip the rollout pref to -1 when the user disables the add-on explicitly. Maybe that's overthinking it, but leaving it at 1 wouldn't correctly reflect user choice for follow-up studies.

Added addon uninstall ✔️

State of disable needs to persist ✔️

johannhof · 2018-05-17T10:51:40Z

src/background.js

+  /**
+   * Ensure that the user hasn't modified any pref in the prerequisite list
+   */
+  async checkPrerequisites() {


nit: it's hard to tell what checkPrerequisites returns, you should probably call it hasUnmodifiedPrerequisites or something like that.

johannhof · 2018-05-17T10:55:01Z

src/background.js

+    }
+  },
+
+  async getStateName() {


nit: is there a reason why this is not called getState()?

johannhof · 2018-05-17T13:10:01Z

src/background.js

+      case null:
+      case "loaded":
+        if (await stateManager.checkPrerequisites()) {
+          await stateManager.setState("loaded");


Why are you manually setting "loaded" again here? Aren't we already in loaded state? Shouldn't you set "enabled" here instead of in this.show();?

johannhof · 2018-05-17T13:16:05Z

src/experiments/notifications/api.js

+    let buttonsOutput = [];
+    if (options.buttons) {
+      let buttonIndex = 0;
+      for (let buttonIndex in options.buttons) {


bah. If this code were landing in m-c I'd strongly prefer you to use for .. of and just count buttonIndex++ :)

Heh so I went for a hybrid of those I didn't need to init the buttonIndex. I think I meant to do what you want then realised the event closure received the latest value where using the index works as the closure didn't need it. I'll throw it into a function instead. ✔️

johannhof · 2018-05-17T14:25:33Z

src/experiments/settings/api.js

+  },
+  async add(id, settingConfig) {
+    await this.init();
+    await ExtensionSettingsStore.addSetting(id, SETTING_TYPE, `settingConfig_${settingConfig.name}`, settingConfig);


Having looked into this for a while I get the feeling you're just duplicating efforts with ExtensionPreferencesManager by maintaining a separate ExtensionSettingsStore setting.

Can you try just ripping out ExtensionSettingsStore usage here and using ExtensionPreferencesManager.getSetting() for settings.get() instead?

https://searchfox.org/mozilla-central/rev/d4b9e50875ad7e5d20f2fee6a53418315f6dfcc0/toolkit/components/extensions/ExtensionPreferencesManager.jsm#314

If that works this code would get much simpler...

…nto shield-study

…ch design more. Fix feedback from johannh on functin naming.

jonathanKingston · 2018-05-18T11:13:16Z

Added most of the changes. However it could do with the settings cleanup now for certain! Lunchtime though.

…hich will be set for anything other than a disable.

jonathanKingston · 2018-05-18T15:34:45Z

@johannhof I think I have covered all your feedback.

Notable changes include (1e418ae):

I removed all the Preferences code and essentially rolling it myself. We clear the prefs everytime it changes "state"
I added an "uninstall" state which is triggered whenever the addon is removed causing all the prefs to be reset. The exception is "disable" which is also an exit state (I added your feedback of uninstalling too), disabling sets the trr state still which is persisted.

johannhof

A couple of things to fix (mostly small nits, but please look closely at the disable issue)

Thanks!

johannhof · 2018-05-18T19:16:44Z

src/study.css

+  margin-inline-end: auto;
+}
+
+button {


Sorry to be pedantic about this, but please give this cursor: pointer and a different background color for :active.

johannhof · 2018-05-18T20:45:46Z

src/experiments/settings/api.js

+            // TODO Decide if we need to do something here
+            break;
+          case "ADDON_UNINSTALL":
+            settingManager.clear(extension.id);


Moving away from ExtensionPreferencesManager means you don't get the prefs cleanup for free when the extension is disabled, which may be what we want, because that means you can set the -1 state on ADDON_DISABLE (because arguably disable means the user wanted to opt out of the experiment). I'm not sure how to guard this against disables that happen without user interaction, though...

In any case, please update this to call .clear() when the add-on is disabled, because currently you're leaking the pref :)

✔️ I set this to clear which sets the state to -2 due to the issue mentioned about guarding reasons. This is better than not having clearing at all... we could re-address though to change to set to -1

johannhof · 2018-05-18T20:54:03Z

src/experiments/settings/api.js

+  },
+  getUserPref(name, value) {
+    if (!Services.prefs.prefHasUserValue(name)) {
+      return null;


shouldn't this return value?

Ah yeah it was... thanks ✔️

johannhof · 2018-05-18T20:56:54Z

src/experiments/settings/api.js

+      }
+      await ExtensionSettingsStore.removeSetting(id, SETTING_TYPE, key);
+    }
+    return true;


nit: why return true?

✔️ Just habit of always having a positive return when things went well. I will remove.

johannhof · 2018-05-18T21:02:26Z

src/experiments/settings/api.js

+  },
+  async add(id, settingConfig) {
+    await this.init();
+    await ExtensionSettingsStore.addSetting(id, SETTING_TYPE, `${SETTING_PREFIX}${settingConfig.name}`, settingConfig);


Ok, this went a little in the opposite direction of what I thought (only using ExtensionPreferencesManager and not using ExtensionSettingsStore directly). I didn't mean to make you rewrite this whole component, sorry for that.

I can live with this, even though I'd still say we're using ExtensionSettingsStore a bit outside of what it was intented to be :)

Yeah it wasn't possible to remove the ExtensionPreferencesManager. Likely we could migrate to just basic storage now the code is simpler anyway.
As I said on slack it was likely super over engineered to make this as reusable as possible.

johannhof · 2018-05-18T22:01:17Z

src/study.js

+          continue;
+        }
+        const textOutput = childEl.textContent;
+        el.appendChild(document.createTextNode(textOutput));


nit: you could use el.append(textOutput);

johannhof · 2018-05-18T22:03:52Z

src/study.js

+  const brandStrings = await browser.experiments.settings.getStrings(["brandShortName", "brandFullName"]);
+
+  function createNodes(el, tagConfig) {
+    for (const childEl of [...tagConfig.childNodes]) {


nit: NodeList is iterable, no need for the spread here

johannhof · 2018-05-18T22:04:01Z

src/study.js

@@ -0,0 +1,66 @@
+async function init() {
+  const elements = [...document.querySelectorAll("[data-i18n-message]")];


nit: NodeList is iterable, no need for the spread here

johannhof · 2018-05-18T22:07:03Z

src/study.js

+    const el = document.createElement(tagConfig.tagName);
+    for (const attr of [...tagConfig.attributes]) {
+      el[attr.name] = attr.value;
+    }


you could just call .cloneNode(false) instead, right?

johannhof · 2018-05-18T22:10:43Z

src/study.js

+    return el;
+  }
+
+  const templates = [...document.querySelectorAll("template")];


don't need the spread

…ll state for disabled extension.

Update Russian localization

…he addon is removed.

johannhof

I have to say that the uninstall-on-disable flow doesn't feel as bad as I thought, so I'm in favor of that!

I had, however, a few final notes before I'd consider this good to ship.

johannhof · 2018-05-25T14:36:16Z

src/background.js

+      case null:
+      case "loaded":
+        if (await stateManager.hasUnmodifiedPrerequisites()) {
+          await stateManager.setState("loaded");


Maybe I missed it, was this question answered?

Why are you manually setting "loaded" again here? Aren't we already in loaded state? Shouldn't you set "enabled" here instead of in this.show();?

The loaded state is mostly to track if we broke before enabling... it shouldn't ever be observable. However the user needs to go through it when in null state. I think the loaded state case couldn't be removed from this switch for some reason, will check...

Ah I remember now... as I'm not observing the state changing, I'm just doing the init on addon install. My hope was if we ever had an error in showing the extension we could ship a new study to the users with loaded as a state. I can remove the loaded state and make null go straight to calling show() which will set the enabled state.
That said enabled really should be at the bottom of the show function to make this worthwhile.

I pushed a change to make null fall through to loaded which is always going to be the normal loading path. I moved enabled to the bottom of the fn as mentioned.

johannhof · 2018-05-25T14:46:25Z

src/experiments/settings/api.js

+      }
+      await ExtensionSettingsStore.removeSetting(id, SETTING_TYPE, key);
+      const addon = await AddonManager.getAddonByID(id);
+      addon.uninstall();


I don't think these lines are supposed to happen inside the while loop...

✔️ thanks, no it wasn't however this addon wouldn't have an observable difference here given we only have one settings thing. Either way I made the change. Thanks

johannhof · 2018-05-25T15:02:30Z

src/experiments/settings/api.js

+      const state = await this.get(settingName);
+      const config = await this.getSettingConfig(settingName);
+      // If we don't have a valid state or it's not a minus state then set uninstalled
+      if (!config.states[state] || config.states[state].id >= 0) {


So currently that means once the user clicks "OK" and gets into state -4, uninstalling or disabling in the add-on manager will not overwrite that.

Is that intended? It doesn't seem like it.

This is again the dilemma of "we're uninstalling but did the user really uninstall?". I'm not sure how to solve this :(

This is the intended, as soon as the user clicks OK then we consider that as a final state. The only change we make is clearing the non persistent prefs.

As I was putting it in though I felt the same, it's very hard to track what users are intending to do here. -4 is basically just a different uninstall giving us slightly more data to reason with on the next study.

You probably want to note that briefly in the _comment for the "disabled" state.

… end of function.

johannhof · 2018-05-28T10:21:58Z

src/background.js

+  },
+
+  async show() {
+    browser.experiments.notifications.onButtonClicked.addListener((options) => {


Just a passing note that you're not handling [x] button clicks, which is probably intended...

johannhof · 2018-05-28T10:30:05Z

src/experiments/settings/api.js

+      const state = await this.get(settingName);
+      const config = await this.getSettingConfig(settingName);
+      // If we don't have a valid state or it's not a minus state then set uninstalled
+      if (!config.states[state] || config.states[state].id >= 0) {


You probably want to note that briefly in the _comment for the "disabled" state.

…to avoid loading without prerequisites check.

…mit API into extension.

Fix ESLint config

johannhof · 2018-06-01T08:43:50Z

src/background.js

+          await this.show();
+        } else {
+          // If the user hasn't met the criteria clean up
+          browser.management.uninstallSelf();


This will reset the user prefs through calling the UNINSTALL handler in api.js. Please make sure that doesn't happen, e.g. through defining a new state for this.

✔️ Done, I moved this to use the new null clear state that is in the experiment.

…nto shield-study

…modified state.

jonathanKingston and others added 15 commits May 16, 2018 13:26

Initial code

64b5951

Move to using settings and preference manager to handle addon uninstalls

7d550a3

Add in updated translations and DOMPurify to prevent DOM injection

4e6bad6

Ensure settings are initialised

afe9cde

Update link

8c042bd

Adding in placeholders into study page

2835342

Add in brand strings

dde5b0c

Update manifest desc

ab4bf24

Remove double space

19c9b8c

Removal of styling code for notifications

0a76c7f

Add pontoon-to-webext

fe3197b

Import locales

5abb1c2

Adding translations to build

ac9d47a

Update localizations (en-GB, fr, ru)

d0100f4

Update README.md

fcbc7b4

johannhof reviewed May 17, 2018

View reviewed changes

Merge branch 'shield-study' of github.com:jonathanKingston/http-dns i…

87b41bc

…nto shield-study

jonathanKingston force-pushed the shield-study branch from 35474e7 to db969ec Compare May 18, 2018 09:57

Update install steps and README to simplify building web-ext.

57c32ea

jonathanKingston force-pushed the shield-study branch from db969ec to 57c32ea Compare May 18, 2018 09:57

jonathanKingston added 3 commits May 18, 2018 11:40

Adding in latest russian locale changes

6f8e53a

Uninstall extension when the user clicks disable. Make study page mat…

20f4701

…ch design more. Fix feedback from johannh on functin naming.

Add persisting of prefs that we want to keep post addon install.

28974ef

Softvision-AndreeaCupsa mentioned this pull request May 18, 2018

"network.trr.experimentalRollout" pref value doesn't change to -1 when removing the experiment from "about:addons" page #10

Closed

Adding in simplification of storage states and an uninstalled state w…

1e418ae

…hich will be set for anything other than a disable.

johannhof suggested changes May 18, 2018

View reviewed changes

jonathanKingston added 2 commits May 19, 2018 00:19

Removal of more unused code, add active state to button, call uninsta…

da20c7c

…ll state for disabled extension.

Removal of aync functions that are not needed

dc04c49

flodolo and others added 4 commits May 22, 2018 10:02

Update Russian localization

325a5ef

Change to shield addon study id. Fixes #15

5e1bb40

Merge pull request #14 from flodolo/l10n_20180522

e2704be

Update Russian localization

Remove addon on disable. Add persistant pref states that stay after t…

fa963dc

…he addon is removed.

johannhof suggested changes May 25, 2018

View reviewed changes

Remove uninstall step from loop.

011581b

jonathanKingston force-pushed the shield-study branch from cb6aa16 to 011581b Compare May 25, 2018 16:27

Fall through to loaded state from null state. Moving enabled state at…

464d20e

… end of function.

johannhof approved these changes May 28, 2018

View reviewed changes

jonathanKingston and others added 7 commits May 29, 2018 15:57

Adding some comments based on explaining the states of the application.

c9858e4

Removing storage permission and making loading step not fall through …

559615d

…to avoid loading without prerequisites check.

Fix ESLint config

060450e

Moving state JSON reading and pref checks into experiment files to li…

861f31a

…mit API into extension.

Remove extension if the user doesn't meet the pref criteria

8a951a8

Change blog post to live article

4c3ec9f

Merge pull request #16 from pdehaan/fix-eslint

a15ae9e

Fix ESLint config

johannhof reviewed Jun 1, 2018

View reviewed changes

jonathanKingston added 3 commits June 1, 2018 11:23

Merge branch 'shield-study' of github.com:jonathanKingston/http-dns i…

b722d3d

…nto shield-study

Fixing linting errors

a18707d

Removal of management code as we need to clear the state for already …

7bcf8ce

…modified state.

jonathanKingston merged commit f6c2d9a into master Jun 4, 2018

		@@ -0,0 +1,67 @@
		async function init() {
		// TODO speak with gandalf about new i18n lib to use fragments instead of this

		@@ -0,0 +1,66 @@
		async function init() {
		const elements = [...document.querySelectorAll("[data-i18n-message]")];

Shield study #4

Shield study #4

Conversation

jonathanKingston commented May 16, 2018

johannhof left a 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

jonathanKingston May 18, 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

jonathanKingston May 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanKingston commented May 18, 2018

jonathanKingston commented May 18, 2018

johannhof left a 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

jonathanKingston May 18, 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

johannhof left a 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

jonathanKingston May 18, 2018 •

edited

jonathanKingston May 18, 2018 •

edited

jonathanKingston May 18, 2018 •

edited