Closed Bug 1330494 Opened 7 years ago Closed 7 years ago

Use the pref manager to handle precedence for extensions using chrome_url_overrides

Categories

(WebExtensions :: Frontend, defect, P2)

Product:
WebExtensions
Component:
Frontend
Type:
defect

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed
Blocking Flags:
webextensions ?

People

(Reporter: mattw, Assigned: bsilverberg)

References

Details

(Whiteboard: triaged)

Attachments

(2 files, 1 obsolete file)

Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,
59 bytes, text/x-review-board-request
aswan
: review+
Details
Uflift for part 1
11.13 KB, patch
jcristau
: approval-mozilla-beta-
Details | Diff | Splinter Review 
Handling precedence should be done by the preference manager.

The most recent extension which requests permission to override a page should gain precedence. 

Note: multiple extensions can have precedence with chrome_url_overrides if they each request access to override different pages (e.g. both extensions should have precedence if the first overrides "about:newtab" and the second overrides "about:home").
(In reply to Matthew Wein [:mattw] from comment #0)
> 
> The most recent extension which requests permission to override a page
> should gain precedence. 
> 

This is contrary to our discussion on Tuesday. Maybe it needs to be different for page overrides than it is for preferences, but in our meeting we discussed that the earliest extension would always win in terms of precedence, so if a later extension requests a change it would *not* override an earlier one.
> This is contrary to our discussion on Tuesday. Maybe it needs to be
> different for page overrides than it is for preferences, but in our meeting
> we discussed that the earliest extension would always win in terms of
> precedence, so if a later extension requests a change it would *not*
> override an earlier one.

Okay, sorry, I had it backwards, but that works too. I'm happy as long as we are consistent with either earliest or most recent taking precedence. 

If we could treat "about:newtab", "about:home", etc as preferences, then we wouldn't need to do anything different for page overrides.
I think it was decided in bug 1234150 comment 12 to use the most recent one.

I'm fine either way as long as we stay consistent, both have their pros and cons.
I suggest that we allow user to choose which extension's override should be used, or to keep the Firefox default.

 Example implementation:
Whenever the user installs an extension which uses "chrome_url_overrides", Firefox shows a notification which allows the user to keep the previous extension's override, or use the new one. (notification is shown even if the user wasn't previously using any extension which uses "chrome_url_overrides")

Additionally, an option will be added to "about:preferences" or to "about:addons" to select which extension's override should be used for each supported override or to switch back to the Firefox default at any time.
See Also: → 1373853
Assignee: nobody → bob.silverberg
Keywords: dev-doc-needed
There is also an issue that currently the new tab override does not persist across browser sessions because it is set by calling aboutNewTabService.newTabURL, but only when the manifest is read.

This should be changed to call aboutNewTabService.newTabURL during onStartup() so that it resets every time the browser is restarted.
webextensions: --- → ?
Blocks: 1322308
Comment on attachment 8882352 [details]
Bug 1330494 - Part 1: Remove async from functions in ExtensionSettingsStore that don't need to be async,

https://reviewboard.mozilla.org/r/153438/#review159010
Attachment #8882352 - Flags: review?(aswan) → review+
Comment on attachment 8882353 [details]
Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,

https://reviewboard.mozilla.org/r/153440/#review159012

::: browser/components/extensions/ext-url-overrides.js:17
(Diff revision 1)
> +
>  XPCOMUtils.defineLazyServiceGetter(this, "aboutNewTabService",
>                                     "@mozilla.org/browser/aboutnewtab-service;1",
>                                     "nsIAboutNewTabService");
>  
> -// Bug 1320736 tracks creating a generic precedence manager for handling
> +const STORE_TYPE = "values";

nit: this name is vague

::: browser/components/extensions/ext-url-overrides.js:20
(Diff revision 1)
>                                     "nsIAboutNewTabService");
>  
> -// Bug 1320736 tracks creating a generic precedence manager for handling
> -// multiple addons modifying the same properties, and bug 1330494 has been filed
> -// to track utilizing this manager for chrome_url_overrides. Until those land,
> -// the edge cases surrounding multiple addons using chrome_url_overrides will
> +const STORE_TYPE = "values";
> +const NEW_TAB_SETTING_NAME = "newTabURL";
> +
> +const processNewTabSetting = (extension, action) => {

nit: why not just `function processNewTabSetting(...)`

::: browser/components/extensions/ext-url-overrides.js:21
(Diff revision 1)
>  
> -// Bug 1320736 tracks creating a generic precedence manager for handling
> -// multiple addons modifying the same properties, and bug 1330494 has been filed
> -// to track utilizing this manager for chrome_url_overrides. Until those land,
> -// the edge cases surrounding multiple addons using chrome_url_overrides will
> -// be ignored and precedence will be first come, first serve.
> +const STORE_TYPE = "values";
> +const NEW_TAB_SETTING_NAME = "newTabURL";
> +
> +const processNewTabSetting = (extension, action) => {
> +  let item = ExtensionSettingsStore[action](extension, STORE_TYPE, NEW_TAB_SETTING_NAME);

aren't some (all?) of the actions async?

::: browser/components/extensions/ext-url-overrides.js:51
(Diff revision 1)
> +});
> +/* eslint-enable mozilla/balanced-listeners */
> +
> +
>  this.urlOverrides = class extends ExtensionAPI {
> -  onManifestEntry(entryName) {
> +  async onManifestEntry(entryName) {

I think you should actually use `onStartup` here and then use `getSetting()` and compare what it returns to the contents of `extension.manifest.chrome_url_overrides`.  So for instance, if one version of an extension overrides newtab and then it is updated to a new version that does not, the setting needs to be removed.

::: browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js:24
(Diff revision 1)
> +    let listener = (_eventName, ...args) => {
> +      if (_eventName === eventName) {
> +        Management.off(eventName, listener);
> +        resolve(...args);
> +      }
> +    };
> +
> +    Management.on(eventName, listener);

just use something like `Management.once(eventName, (e, ...args) => resolve(...args));`

::: browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js:35
(Diff revision 1)
> +
> +    Management.on(eventName, listener);
> +  });
> +}
> +
> +function awaitDisabled() {

can you use `AddonTestUtils.promiseAddonEvent()` for this?
Attachment #8882353 - Flags: review?(aswan) → review-
Comment on attachment 8882353 [details]
Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,

https://reviewboard.mozilla.org/r/153440/#review159012

> nit: this name is vague

I changed it to `dynamic_values`, which you may still consider to be vague. I'm not really sure what is a good name for this. It's supposed to be a category for the settings. Up until now we used `prefs` because all settings related to prefs, but this one does not. There's not actually any current reason to need to put this into a different category, but considering that the SettingsStore is set up to store different types of settings I think it makes sense to _not_ just put this in `prefs`.

> nit: why not just `function processNewTabSetting(...)`

Because https://bugzilla.mozilla.org/show_bug.cgi?id=1374237

> aren't some (all?) of the actions async?

No. Part 1 attached to this review request converts all of the functions in SettingsStore to sync, except for `addSetting` and `processNewTabSetting` never calls `addSetting`.
Comment on attachment 8882353 [details]
Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,

https://reviewboard.mozilla.org/r/153440/#review159012

> I changed it to `dynamic_values`, which you may still consider to be vague. I'm not really sure what is a good name for this. It's supposed to be a category for the settings. Up until now we used `prefs` because all settings related to prefs, but this one does not. There's not actually any current reason to need to put this into a different category, but considering that the SettingsStore is set up to store different types of settings I think it makes sense to _not_ just put this in `prefs`.

How about something like `"url_overrides"`?

> Because https://bugzilla.mozilla.org/show_bug.cgi?id=1374237

Meh, "everybody should always remember to do this in the future" is never going to work.  But that's not an issue for this bug...
Comment on attachment 8882353 [details]
Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,

https://reviewboard.mozilla.org/r/153440/#review159972

Thanks for writing the xpcshell test, I think a bunch of stuff that's in the existing browser chrome test could move over there if you're so inclined...
A few nits but lets get the startup event handling straightened out.

::: browser/components/extensions/ext-url-overrides.js:22
(Diff revision 3)
> -// Bug 1320736 tracks creating a generic precedence manager for handling
> -// multiple addons modifying the same properties, and bug 1330494 has been filed
> -// to track utilizing this manager for chrome_url_overrides. Until those land,
> -// the edge cases surrounding multiple addons using chrome_url_overrides will
> -// be ignored and precedence will be first come, first serve.
> -let overrides = {
> +const STORE_TYPE = "dynamic_values";
> +const NEW_TAB_SETTING_NAME = "newTabURL";
> +
> +const processNewTabSetting = (extension, action) => {
> +  let item = ExtensionSettingsStore[action](extension, STORE_TYPE, NEW_TAB_SETTING_NAME);
> +  if (["disable", "removeSetting"].includes(action) && item) {

Why doesn't this run on eg enable?

::: browser/components/extensions/ext-url-overrides.js:42
(Diff revision 3)
> +Management.on("startup", (type, extension) => {
> +  if (["ADDON_ENABLE", "ADDON_UPGRADE", "ADDON_DOWNGRADE"].includes(extension.startupReason)) {
> +    processNewTabSetting(extension, "enable");
> +  }
> +});

why do you need both this and the `onStartup()` method below?

::: browser/components/extensions/ext-url-overrides.js:61
(Diff revision 3)
> +    if (manifest.chrome_url_overrides && manifest.chrome_url_overrides.newtab) {
>        let newtab = manifest.chrome_url_overrides.newtab;
>        let url = extension.baseURI.resolve(newtab);
>  
> -      // Only set the newtab URL if no other extension is overriding it.
> -      if (!overrides.newtab.length) {
> +      await ExtensionSettingsStore.addSetting(
> +        extension, STORE_TYPE, NEW_TAB_SETTING_NAME, url, () => {

nit, you can shorten the last arg to `() => aboutNewTabService.newTabURL`

::: browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js:79
(Diff revision 3)
> +  await disabledPromise;
> +  is(aboutNewTabService.newTabURL, "about:newtab",
> +       "Newtab url is about:newtab after second extension is disabled.");
>  
> +  // Re-enable the second extension.
> +  let enabledPromise = awaitEvent("ready");

nit: you could just use `promiseAddonEvent("onEnabled")` here (or use `awaitEvent("shutdown")` above, to keep the disable/enable code symmetric.
Attachment #8882353 - Flags: review?(aswan) → review-
Blocks: 1378882
Comment on attachment 8882353 [details]
Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,

https://reviewboard.mozilla.org/r/153440/#review159972

> Why doesn't this run on eg enable?

This deals with cases where the current aboutNewTabService.newTabURL needs to be changed because the extension that was controlling it has been disabled or removed. In that case onStartup() won't fire, so we need to make the change to aboutNewTabService.newTabURL here. When the case is that an extension is being enabled which will require the aboutNewTabService.newTabURL to be changed, that will be dealt with in onStartup, which is why it is being excluded here. We still need the first line of `processNewTabSetting` to be run in the case of `enable` because we want to update the settings store with the fact that the setting is being enabled, but we don't want to update the aboutNewTabService.newTabURL here. More on the relationship with onStartup below...

> why do you need both this and the `onStartup()` method below?

We need to know when an extension is being re-enabled, so that we can update the SettingsStore to mark the setting as enabled, which may or may not require updating the actual aboutNewTabService.newTabURL. My understanding is that we can only know that the extension was re-enabled here, but if there is a way to know that in the `onStartup()` then yes, we can get rid of this.

> nit: you could just use `promiseAddonEvent("onEnabled")` here (or use `awaitEvent("shutdown")` above, to keep the disable/enable code symmetric.

I tried using `promiseAddonEvent("onEnabled")` when I introduced `promiseAddonEvent("onDisabled")` but the timing was off. I tried the opposite (using `awaitEvent("shutdown")`) and that seems to work fine. Thanks.
Comment on attachment 8882353 [details]
Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,

https://reviewboard.mozilla.org/r/153440/#review159972

> This deals with cases where the current aboutNewTabService.newTabURL needs to be changed because the extension that was controlling it has been disabled or removed. In that case onStartup() won't fire, so we need to make the change to aboutNewTabService.newTabURL here. When the case is that an extension is being enabled which will require the aboutNewTabService.newTabURL to be changed, that will be dealt with in onStartup, which is why it is being excluded here. We still need the first line of `processNewTabSetting` to be run in the case of `enable` because we want to update the settings store with the fact that the setting is being enabled, but we don't want to update the aboutNewTabService.newTabURL here. More on the relationship with onStartup below...

So about just have `onStartup()` call `ExtensionSettingsStore.enable()` directly?  Its confusing that it calls `processNewTabSetting()` but it doesn't want that function to actually process the setting.

> We need to know when an extension is being re-enabled, so that we can update the SettingsStore to mark the setting as enabled, which may or may not require updating the actual aboutNewTabService.newTabURL. My understanding is that we can only know that the extension was re-enabled here, but if there is a way to know that in the `onStartup()` then yes, we can get rid of this.

Perhaps I'm misunderstanding but you can read `extension.startupReason` at any time, why wouldn't it work in `onStartup()` ?
Comment on attachment 8882353 [details]
Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,

https://reviewboard.mozilla.org/r/153440/#review161664

A collection of little things, but getting close...

::: browser/components/extensions/ext-url-overrides.js:28
(Diff revision 5)
> +Management.on("shutdown", (type, extension) => {
> +  switch (extension.shutdownReason) {
> +    case "ADDON_DISABLE":
> +    case "ADDON_DOWNGRADE":
> +    case "ADDON_UPGRADE":
> +      processNewTabSetting(extension, "disable");
> +      break;
> +
> +    case "ADDON_UNINSTALL":
> +      processNewTabSetting(extension, "removeSetting");
> +      break;
> +  }
> +});

why not just `extension.callOnClose()` ?

::: browser/components/extensions/ext-url-overrides.js:71
(Diff revision 5)
> -        aboutNewTabService.newTabURL = overrides.newtab[0].url;
> -      } else {
> -        aboutNewTabService.resetNewTabURL();
> -      }
> +    // Set the newTabURL to the current value of the setting.
> +    let item = ExtensionSettingsStore.getSetting(STORE_TYPE, NEW_TAB_SETTING_NAME);
> +    if (item) {
> +      aboutNewTabService.newTabURL = item.value || item.initialValue;
>      }

Why is this necessary?  If we didn't take either branch of the if/else above then we don't need to touch newtab at all.  And if we took the else branch, this already got done once in `processNewTabSetting()`.  So it looks like you can just move this into the if clause?  (and perhaps simplify it a bit)

::: browser/components/extensions/test/browser/browser_ext_url_overrides_newtab.js:68
(Diff revision 5)
> +  let addon = await AddonManager.getAddonByID(EXT_2_ID);
> +  let disabledPromise = awaitEvent("shutdown");
> +  addon.userDisabled = true;
> +  await disabledPromise;
> +  is(aboutNewTabService.newTabURL, "about:newtab",
> +       "Newtab url is about:newtab after second extension is disabled.");

nit: fix indentation

::: browser/components/extensions/test/xpcshell/test_ext_url_overrides_newtab.js:1
(Diff revision 5)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */

This looks nice, thanks!  But now it looks like we're basically doing the same thing in both xpcshell and mochitest?
Attachment #8882353 - Flags: review?(aswan) → review-
Comment on attachment 8882353 [details]
Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,

https://reviewboard.mozilla.org/r/153440/#review161664

> why not just `extension.callOnClose()` ?

I couldn't figure out where/how to add `extension.callOnClose()` into this, but `onShutdown` seems to work, so I've changed it to that. If that is not appropriate or if `callOnClose` would be better, please let me know, and perhaps provide a clue as to how to fit that into this API.

> Why is this necessary?  If we didn't take either branch of the if/else above then we don't need to touch newtab at all.  And if we took the else branch, this already got done once in `processNewTabSetting()`.  So it looks like you can just move this into the if clause?  (and perhaps simplify it a bit)

Good point. You are correct that I can just move that logic into the first `if` clause. I'm not sure how any of it can be simplified however. I did combine two lines into one at the top of the clause, but other than that it all seems necessary.
Comment on attachment 8882353 [details]
Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,

https://reviewboard.mozilla.org/r/153440/#review161664

> Good point. You are correct that I can just move that logic into the first `if` clause. I'm not sure how any of it can be simplified however. I did combine two lines into one at the top of the clause, but other than that it all seems necessary.

I think that both `addSetting()` and `enable()` both return the most recent item so you could avoid calling `getSetting()`?
Comment on attachment 8882353 [details]
Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,

https://reviewboard.mozilla.org/r/153440/#review161778
Attachment #8882353 - Flags: review?(aswan) → review+
Comment on attachment 8882353 [details]
Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,

https://reviewboard.mozilla.org/r/153440/#review161664

> I think that both `addSetting()` and `enable()` both return the most recent item so you could avoid calling `getSetting()`?

Incorrect. `addSetting` only returns an item if the item just added is now the current top item, and `enable` only returns an item if the current top item was changed as a result of the call to `enable`, so in both cases it is possible that no item was returned from either.
Comment on attachment 8882353 [details]
Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,

https://reviewboard.mozilla.org/r/153440/#review161664

> Incorrect. `addSetting` only returns an item if the item just added is now the current top item, and `enable` only returns an item if the current top item was changed as a result of the call to `enable`, so in both cases it is possible that no item was returned from either.

But in those cases where they don't return an item, that also means this code shouldn't be changing the newtab url...
Comment on attachment 8882353 [details]
Bug 1330494 - Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides,

https://reviewboard.mozilla.org/r/153440/#review161664

> But in those cases where they don't return an item, that also means this code shouldn't be changing the newtab url...

I suppose that's true. I'll make that change.
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0185de5f7c5a
Part 1: Remove async from functions in ExtensionSettingsStore that don't need to be async, r=aswan
https://hg.mozilla.org/integration/autoland/rev/1edbb90fcd51
Part 2: Use the ExtensionsSettingsStore to handle precedence for extensions using chrome_url_overrides, r=aswan
https://hg.mozilla.org/mozilla-central/rev/0185de5f7c5a
https://hg.mozilla.org/mozilla-central/rev/1edbb90fcd51
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Approval Request Comment
[Feature/Bug causing the regression]: 1320736
[User impact if declined]: This is needed to support uplifting bug 1381297 which addresses an issue with multiple extensions being installed which manage the same setting.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: The blocks the uplift of bug 1381297 
[Is the change risky?]: No
[Why is the change risky/not risky?]: These are minor changes to the ExtensionSettingsStore.jsm which are covered by tests.
[String changes made/needed]: None
Attachment #8882352 - Attachment is obsolete: true
Attachment #8890837 - Flags: approval-mozilla-beta?
Comment on attachment 8890837 [details] [diff] [review]
Uflift for part 1

same as 1381297
Attachment #8890837 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: