Observe prefs and control webextension startup and shutdown via a pref #2397

ianb · 2017-03-15T20:21:03Z

For handoff to @6a68

ianb · 2017-03-15T20:21:23Z

addon/bootstrap.js

+      browser.runtime.onMessage.addListener(handleMessage);
+    });
+  } else if ((! start) && webExtensionStarted) {
+    // pass


Of course, this is where shutdown should occur

ianb · 2017-03-15T20:22:13Z

addon/bootstrap.js

+    // aSubject is the nsIPrefBranch we're observing (after appropriate QI)
+    // aData is the name of the pref that's been changed (relative to aSubject)
+    if (aData == "userdisabled" || aData == "systemdisabled") {
+      handleStartup();


I thought about observing the Telemetry pref here, but it's not that easy – there's no way to initiate a message from bootstrap.js to the webextension. So I think the current logic for the telemetry pref is good as it is.

pdehaan · 2017-03-17T19:05:37Z

addon/bootstrap.js

+Components.utils.import("resource://gre/modules/Services.jsm");
+const { EmbeddedExtensionManager } = Components.utils.import("resource://gre/modules/LegacyExtensionsUtils.jsm");
+
+const prefs = Services.prefs;


/home/travis/build/mozilla-services/pageshot/addon/bootstrap.js 15:15 error 'Services' is not defined no-undef 78:19 error 'Services' is not defined no-undef

Not sure if we need to tweak the ESLint envs, or just manually add another globals to appease the ESLint gods.

I've been generous adding /* globals */ to my code

@ianb: "I've been generous adding /* globals */ to my code"

I've noticed. We can also add them to the root .eslintrc.yml file, or even add nested .eslintrc.yml files, so you don't have to shotgun a small manifesto of global vars at the top of each file.

They are like pretend import statements!

Ah yeah, thanks @pdehaan. Updated globals.

jaredhirsch · 2017-03-17T21:45:00Z

@ianb Would you mind giving this branch a try? It seems to be totally working for me.

Cases to try out:

start without the extensions.pageshot.userdisabled pref set, then toggle it on and off a few times
start with userdisabled set to false (should check the pref, then init the webextension)
start with userdisabled set to true, verify it's not loaded, then toggle the pref and verify it works

Then, same three cases for extensions.pageshot.systemdisabled.

If this seems to be working for you as well, feel free to review now, and I'll plan to squash before merging.

ianb

I tested this, and systemdisabled didn't work, because of bad boolean logic per my comment. Otherwise things seemed to work well.

ianb · 2017-03-17T22:46:31Z

addon/bootstrap.js

+const ADDON_ID = "pageshot@mozilla.org";
+const TELEMETRY_PREF = "toolkit.telemetry.enabled";
+const USER_ENABLE_PREF = "extensions.pageshot.userdisabled";
+const SYSTEM_ENABLE_PREF = "extensions.pageshot.systemdisabled";


Well, this is my fault, but these variables have a really bad name – ENABLE when in fact they are DISABLE

ianb · 2017-03-17T22:47:34Z

addon/bootstrap.js

+    prefs.getPrefType(SYSTEM_ENABLE_PREF) &&
+    prefs.getBoolPref(SYSTEM_ENABLE_PREF)
+  );
+}


This is probably my fault too, but the expression here is incorrect. I think it's (! USER_ENABLE_PREF) || SYSTEM_ENABLE_PREF – as a result setting extensions.pageshot.systemdisabled to true forces Page Shot on rather than off.

jaredhirsch · 2017-03-17T23:08:51Z

thanks for looking! I'll fixup the logic

Other changes: * Fix #2370, unset deviceId pref set by old addon. * Update Promise rejection / Error handling to match behavior documented in the addons-related Gecko code.

jaredhirsch · 2017-03-18T06:16:19Z

@ianb Mind taking another look? Fixed up the disable check logic, did a general cleanup pass, and squashed everything into one commit. Hopefully bootstrap.js will get through Firefox review without too many problems :-P

Note that I renamed the user pref to just extensions.pageshot.disabled, because the system-disabled pref will be gone within a few weeks of launch, so calling it user-disabled seemed unnecessary.

ianb commented Mar 15, 2017

View reviewed changes

jaredhirsch force-pushed the enabling-pref branch 4 times, most recently from 66d553f to c020dcc Compare March 17, 2017 04:02

pdehaan reviewed Mar 17, 2017

View reviewed changes

ianb commented Mar 17, 2017

View reviewed changes

jaredhirsch force-pushed the enabling-pref branch from 4c4319f to a40bf2d Compare March 18, 2017 04:52

jaredhirsch changed the title ~~WIP, observe prefs and control webextension startup and shutdown via a pref~~ Observe prefs and control webextension startup and shutdown via a pref Mar 18, 2017

jaredhirsch force-pushed the enabling-pref branch 5 times, most recently from a7ca371 to b28d687 Compare March 18, 2017 06:11

Fix #2332, fix #2333: toggle webextension on pref change

b28d687

Other changes: * Fix #2370, unset deviceId pref set by old addon. * Update Promise rejection / Error handling to match behavior documented in the addons-related Gecko code.

ianb merged commit cd33c8a into master Mar 20, 2017

ianb deleted the enabling-pref branch March 20, 2017 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Observe prefs and control webextension startup and shutdown via a pref #2397

Observe prefs and control webextension startup and shutdown via a pref #2397

ianb commented Mar 15, 2017

ianb Mar 15, 2017

ianb Mar 15, 2017

pdehaan Mar 17, 2017

ianb Mar 17, 2017

pdehaan Mar 17, 2017

ianb Mar 17, 2017

jaredhirsch Mar 17, 2017

jaredhirsch commented Mar 17, 2017

ianb left a comment

ianb Mar 17, 2017

ianb Mar 17, 2017

jaredhirsch commented Mar 17, 2017

jaredhirsch commented Mar 18, 2017

Observe prefs and control webextension startup and shutdown via a pref #2397

Observe prefs and control webextension startup and shutdown via a pref #2397

Conversation

ianb commented Mar 15, 2017

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

jaredhirsch commented Mar 17, 2017

ianb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredhirsch commented Mar 17, 2017

jaredhirsch commented Mar 18, 2017