1341458 - Move support for setting the homepage from chrome_url_overrides to chrome_settings_overrides

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[Mike Kaply [:mkaply]]

Code was added in bug 1298950 to override the homepage via chrome_url_overrides.

I agree with the idea, but that code should have actually overrode the about:home URL similar to how about:newtab works, perhaps by registering a factory for about.

In chrome extensions, the proper way to set the homepage is via chrome_settings_override with homepage (since they don't have the concept of about:home). We shoudl be using that.

Also, I don't think we should set browser.startup.homepage as a user pref, I think we should probably have the add-on just set it as a default pref and maybe reset the user pref at that point? Then the user can easily override and when the add-on goes away, it resets.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: Bug 1341458 - Move homepage to chrome_settings_overrides.

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

Comment 2

[Mike Kaply [:mkaply]]

I'm getting this error on the console:

1487718147595	addons.webextension.<unknown>	WARN	Loading extension 'null': Reading manifest: Error processing chrome_settings_overrides: An unexpected property was found in the WebExtension manifest.

but everything is working. I'm not sure why. I think I did everything correctly.

(Obviously we still need some tests here).

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

https://reviewboard.mozilla.org/r/114304/#review116032

::: browser/components/extensions/ext-chrome-settings-overrides.js:15
(Diff revision 1)
> +
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("manifest_chrome_settings_overrides", (type, directive, extension, manifest) => {
> +  if ("homepage" in manifest.chrome_settings_overrides) {
> +     Services.prefs.getDefaultBranch("browser.startup.")
> +                   .setCharPref("homepage", "data:text/plain,browser.startup.homepage=" + manifest.chrome_settings_overrides.homepage);

extension.baseURI.resolve(homepage)

edit: maybe not, looks like schema is handling this

::: browser/components/extensions/ext-chrome-settings-overrides.js:20
(Diff revision 1)
> +                   .setCharPref("homepage", "data:text/plain,browser.startup.homepage=" + manifest.chrome_settings_overrides.homepage);
> +  }
> +});
> +
> +extensions.on("shutdown", (type, extension) => {
> +  // Default branch will undo itself

It would be better to say it doesn't persist across restart.  However, we'll need to queue the setting to support multiple extensions, like what was done in url overrides.

::: browser/components/extensions/extensions-browser.manifest:5
(Diff revision 1)
>  # scripts
>  category webextension-scripts bookmarks chrome://browser/content/ext-bookmarks.js
>  category webextension-scripts browserAction chrome://browser/content/ext-browserAction.js
>  category webextension-scripts browsingData chrome://browser/content/ext-browsingData.js
> +category webextension-scripts chrome-settings-overrides chrome://browser/content/ext-chrome-settings-overrides.js

need to add the schema file in here as well, that will fix the error you mention in the bug.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Sigh.  I was under the impression that chrome had *no* setting for homepage, so we had it in url overrides.  Since this is here, we'll need to back out bug 1298950.

Comment 5

[Mike Kaply [:mkaply]]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

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

Comment 6

[Mike Kaply [:mkaply]]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

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

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

https://reviewboard.mozilla.org/r/114304/#review117484

::: browser/components/extensions/ext-chrome-settings-overrides.js:13
(Diff revision 3)
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",
> +                                  "resource://gre/modules/Services.jsm");
> +
> +// Because we are setting a default pref for browser.startup.homepage, we
> +// can't reset it to its original value. So we need to store that.
> +let originalHomepage = null;

This should be preserved in a pref, otherwise I could see loosing the original.

::: browser/components/extensions/ext-chrome-settings-overrides.js:32
(Diff revision 3)
> + * are in line, the homepage is reset to its default value, queried at startup.
> + */
> +function updateHomepage() {
> +  if (overrides["homepage"].length) {
> +    Services.prefs.getDefaultBranch("browser.startup.")
> +                  .setCharPref("homepage", "data:text/plain,browser.startup.homepage=" + overrides["homepage"][0].url);

In the distro's we have a couple other homepage prefs that get set at times.  homepage_override, homepage_welcome...should we tackled those at the same time, or later?

Comment 8

[Andrew Swan [:aswan]]

pardon the drive-by comment, but we have the ExtensionSettingStore landed now, this should use that.

Comment 9

[Mike Kaply [:mkaply]]

> This should be preserved in a pref, otherwise I could see loosing the original.

Good call.

> In the distro's we have a couple other homepage prefs that get set at times.  homepage_override, homepage_welcome...should we tackled those at the same time, or later?

I still see those being overridden in distribution.ini. This is primarily for WebExtensions that want to override the homepage. I don't think we should give them all the other URLs.

> pardon the drive-by comment, but we have the ExtensionSettingStore landed now, this should use that.

For what specifically?

Comment 10

[Andrew Swan [:aswan]]

(In reply to Mike Kaply [:mkaply] from comment #9)
> > pardon the drive-by comment, but we have the ExtensionSettingStore landed now, this should use that.
> 
> For what specifically?

Well to preserve the original value for one thing, but also to ensure that if multiple extensions try to override this, that the "winner" is chosen in an orderly and predictable way.  And in the long term, to enable things like bug 1298955.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

https://reviewboard.mozilla.org/r/114302/#review117498

::: browser/components/extensions/ext-url-overrides.js
(Diff revision 3)
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>  XPCOMUtils.defineLazyServiceGetter(this, "aboutNewTabService",
>                                     "@mozilla.org/browser/aboutnewtab-service;1",
>                                     "nsIAboutNewTabService");
> -XPCOMUtils.defineLazyModuleGetter(this, "Preferences",
> -                                  "resource://gre/modules/Preferences.jsm");

Can we also backout the url overrides in a seperate backout patch?

Comment 12

[Bob Silverberg [:bsilverberg]]

(In reply to Andrew Swan [:aswan] from comment #10)
> (In reply to Mike Kaply [:mkaply] from comment #9)
> > > pardon the drive-by comment, but we have the ExtensionSettingStore landed now, this should use that.
> > 
> > For what specifically?
> 
> Well to preserve the original value for one thing, but also to ensure that
> if multiple extensions try to override this, that the "winner" is chosen in
> an orderly and predictable way.  And in the long term, to enable things like
> bug 1298955.

Pardon my drive-by to Andrew's drive-by. In addition to agreeing that we should use ExtensionSettingStore, I also wanted to point out that the version currently in m-c is slightly different from an updated one that I hope to land this week, so you'll probably want to use the API for the new one, which is currently in review at https://reviewboard.mozilla.org/r/114940/.

Comment 13

[Mike Kaply [:mkaply]]

I'll wait to finish this up until the new store has landed.

Comment 14

[Mike Kaply [:mkaply]]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114304/diff/3-4/

Comment 15

[Mike Kaply [:mkaply]]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114304/diff/4-5/

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

https://reviewboard.mozilla.org/r/114304/#review118876

r+ With the additional changes.

::: browser/components/extensions/ext-chrome-settings-overrides.js:10
(Diff revision 5)
> +"use strict";
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Services",

remove, not used

::: browser/components/extensions/ext-url-overrides.js:33
(Diff revision 5)
> -      overrides[page].push({id: extension.id, url});
> -      updatePage(page);
> -      break;
>      }
> +
> +    overrides.newtab.push({extension, url});

lets use the improvement from the reverted patch: push({id: extension.id, url})

::: browser/components/extensions/ext-url-overrides.js:38
(Diff revision 5)
> +    overrides.newtab.push({extension, url});
>    }
>  });
>  
>  extensions.on("shutdown", (type, extension) => {
> -  for (let page of Object.keys(overrides)) {
> +  let i = overrides.newtab.findIndex(o => o.extension === extension);

can we keep the use of extension.id here?  That will avoid a reference to the extension.

::: browser/components/extensions/test/browser/browser_ext_url_overrides_home.js:75
(Diff revision 5)
> -     "Home url should be overriden by the third extension.");
>  
> -  yield ext3.unload();
> +  // Because we are unloading the last extension, we need to wait for
> +  // browser.startup.homepage to change, otherwise the next test will
> +  // run too early
> +  var prefPromise = new Promise((resolve, reject) =>

make this a function since it's used more than once, put it in head.js (I'm guessing it will get used more)

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1341458 - Move homepage to chrome_settings_overrides.

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

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8843526 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

https://reviewboard.mozilla.org/r/117230/#review118902

This is a setting that we expose to users, unlike (e.g.) the new tab page. When the user changes (or resets) the homepage preference after an add-on has changed it, we silently throw away their changes on first restart. I really don't like the fact that we let extensions change this setting without even prompting, but given that we do, we really need to respect users choices when they change it on their own.

::: browser/components/extensions/ext-chrome-settings-overrides.js:15
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "ExtensionPreferencesManager",
> +                                  "resource://gre/modules/ExtensionPreferencesManager.jsm");
> +
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("manifest_chrome_settings_overrides", (type, directive, extension, manifest) => {
> +  if ("homepage" in manifest.chrome_settings_overrides &&

No `in` check. It will always be true, since omitted optional properties are normalized to `null` or their default value.

::: browser/components/extensions/ext-chrome-settings-overrides.js:17
(Diff revision 1)
> +
> +/* eslint-disable mozilla/balanced-listeners */
> +extensions.on("manifest_chrome_settings_overrides", (type, directive, extension, manifest) => {
> +  if ("homepage" in manifest.chrome_settings_overrides &&
> +      manifest.chrome_settings_overrides.homepage) {
> +    ExtensionPreferencesManager.addSetting("homepage_override", {

Presumably we don't want to call this once for every extension that changes the homepage?

::: browser/components/extensions/schemas/chrome_settings_overrides.json:11
(Diff revision 1)
> +        "$extend": "WebExtensionManifest",
> +        "properties": {
> +          "chrome_settings_overrides": {
> +            "type": "object",
> +            "optional": true,
> +            "properties": {

Need to allow unknown properties here.

::: browser/components/extensions/schemas/url_overrides.json:11
(Diff revision 1)
>          "$extend": "WebExtensionManifest",
>          "properties": {
>            "chrome_url_overrides": {
>              "type": "object",
>              "optional": true,
>              "properties": {

We need to allow unknown properties here.

::: browser/components/extensions/test/browser/browser_ext_url_overrides_home.js:14
(Diff revision 1)
> -const HOME_URI_1 = "webext-home-1.html";
> -const HOME_URI_2 = "webext-home-2.html";
> -const HOME_URI_3 = "webext-home-3.html";
> +// Named this way so they correspond to the extensions
> +const HOME_URI_2 = "http://example.com/";
> +const HOME_URI_3 = "http://example.org/";
> +const HOME_URI_4 = "http://example.net/";
>  
>  add_task(function* test_multiple_extensions_overriding_newtab_page() {

s/newtab_/home/

::: browser/components/extensions/test/browser/browser_ext_url_overrides_home.js:18
(Diff revision 1)
>  
>  add_task(function* test_multiple_extensions_overriding_newtab_page() {
>    let defaultHomePage = Preferences.get("browser.startup.homepage");
>  
>    is(Preferences.get("browser.startup.homepage"), defaultHomePage,
>       `Default home url should be ${defaultHomePage}`);

No need for the expected value in the assertion message.

::: browser/components/extensions/test/browser/browser_ext_url_overrides_home.js:43
(Diff revision 1)
>    });
>  
>    yield ext1.startup();
>  
>    is(Preferences.get("browser.startup.homepage"), defaultHomePage,
>         `Default home url should still be ${defaultHomePage}`);

Weird indentation. Why doesn't ESLint complain about this...

Comment 19

[Kris Maglione [:kmag]]

It might be a good idea to backout the chrome_url_overrides "home" property rather than let it ride the trains before this change lands.

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8843526 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

clearing the r? to me since Kris responded

Comment 21

[Mike Kaply [:mkaply]]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114304/diff/5-6/

Comment 22

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8843526 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

work jumped back to mkaply, obsoleting this

Comment 23

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

https://reviewboard.mozilla.org/r/114304/#review119680

Please add a test that the user set homepage is restored after an addon is uninstalled.  Also run through try.  Then r+

Comment 24

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

https://reviewboard.mozilla.org/r/114304/#review119694

::: browser/components/extensions/ext-chrome-settings-overrides.js:22
(Diff revision 6)
> +                                           manifest.chrome_settings_overrides.homepage);
> +  }
> +});
> +
> +extensions.on("shutdown", (type, extension) => {
> +  ExtensionPreferencesManager.unsetSetting(extension, "homepage_override");

Pardon the drive-by, but it looks like this is using an older version of ExtensionPreferencesManager. ExtensionPreferencesManager no longer has an `unsetSetting` function. It instead has `removeSetting` and `disableSetting`. 

Also is this code sufficient/accurate? Do you want this to be unset/removed for every shutdown reason, including browser shutdown, or should the logic be a bit more complex as is being done in the privacy API [1]?

[1] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-privacy.js#17-35

Comment 25

[Mike Kaply [:mkaply]]

> Pardon the drive-by, but it looks like this is using an older version of ExtensionPreferencesManager. ExtensionPreferencesManager no longer has an `unsetSetting` function. It instead has `removeSetting` and `disableSetting`. 

Thanks for the info. I guess I had based mine on a version that happened right before your updates. I'll make it similar to the privacy API.

Comment 26

[Shane Caraveo (:mixedpuppy)]

Bob, can you give a final ok on the changes Mike will do for ExtensionPreferencesManager use?

Comment 27

[Bob Silverberg [:bsilverberg]]

Certainly! With the caveat that I am only around until Friday and then off for two weeks. I don't want to be a blocker.

Comment 28

[Mike Kaply [:mkaply]]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114304/diff/6-7/

Comment 29

[Mike Kaply [:mkaply]]

New patch on mozreview has the additional test requested by mixedpuppy and the switch of the API mentioned by bsilverberg.

Comment 30

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

https://reviewboard.mozilla.org/r/114304/#review119758

::: browser/components/extensions/ext-chrome-settings-overrides.js:21
(Diff revision 7)
> +    await ExtensionPreferencesManager.setSetting(extension, "homepage_override",
> +                                                 manifest.chrome_settings_overrides.homepage);
> +  }
> +});
> +
> +extensions.on("shutdown", async (type, extension) => {

This looks good, but it also needs a similar `extensions.on("startup")` to ext-privacy.js otherwise the setting will not be re-enabled when an extension is re-enabled or when an extension update completes. 

It's probably a good idea to add a test to confirm that this works in the case of disabling/enabling an extension to catch any possible bugs in that logic. There's an existing test at [1] which is very similar.

[1] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_privacy_disable.js

Comment 31

[Mike Kaply [:mkaply]]

> This looks good, but it also needs a similar `extensions.on("startup")` to ext-privacy.js otherwise the setting will not be re-enabled when an extension is re-enabled or when an extension update completes. 

I thought that at first, but it appears that the manifest_chrome_settings_overrides call happens at every startup as well and things get set.

So I didn't know what the right thing to do here was. Should I be doing everything on startup and not using manifest_chrome_settings_overrides at all?

Comment 32

[Bob Silverberg [:bsilverberg]]

(In reply to Mike Kaply [:mkaply] from comment #31)
> > This looks good, but it also needs a similar `extensions.on("startup")` to ext-privacy.js otherwise the setting will not be re-enabled when an extension is re-enabled or when an extension update completes. 
> 
> I thought that at first, but it appears that the
> manifest_chrome_settings_overrides call happens at every startup as well and
> things get set.
> 
> So I didn't know what the right thing to do here was. Should I be doing
> everything on startup and not using manifest_chrome_settings_overrides at
> all?

The issue isn't with the setting getting set, which will happen on every startup, as you point out, but with it getting re-enabled. The code in on.shutdown will disable the setting, for example when an extension is uninstalled as part of an update, and that will leave the setting as disabled unless it is explicitly re-enabled, which is what the code for on.startup does. 

So your code for manifest_chrome_settings_overrides is correct, as it is used to set the value, but you also need the code on on.startup in order to re-enable the setting after an update completes or the extension itself is re-enabled.

Comment 33

[Mike Kaply [:mkaply]]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114304/diff/7-8/

Comment 34

[Mike Kaply [:mkaply]]

OK, I think this covers everything.

Comments from bsilverberg addressed, new test added for disabling.

Comment 35

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

https://reviewboard.mozilla.org/r/114304/#review120118

::: browser/components/extensions/test/browser/browser_ext_url_overrides_home.js:144
(Diff revisions 7 - 8)
>  });
> +
> +add_task(function* test_disable() {
> +  let defaultHomePage = Preferences.get("browser.startup.homepage");
> +
> +  is(Preferences.get("browser.startup.homepage"), defaultHomePage,

This seems a bit odd. You are comparing the result of `Preferences.get("browser.startup.homepage")` to a variable in which you just stored the result of `Preferences.get("browser.startup.homepage")`. Won't that just always be true?

::: browser/components/extensions/test/browser/browser_ext_url_overrides_home.js:149
(Diff revisions 7 - 8)
> +  is(Preferences.get("browser.startup.homepage"), defaultHomePage,
> +     "Home url should be the default");
> +
> +  const ID = "[e-mail]";
> +
> +  // Create an array of extensions to install.

You're only creating one extension, so this comment is incorrect.

Comment 36

[Mike Kaply [:mkaply]]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114304/diff/8-9/

Comment 37

[Mike Kaply [:mkaply]]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114304/diff/9-10/

Comment 38

[Mike Kaply [:mkaply]]

Comment on attachment 8839719 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114304/diff/10-11/

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1af0390360c1
Move homepage to chrome_settings_overrides. r=bsilverberg,mixedpuppy

Comment 40

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1af0390360c1

Comment 41

[Mike Kaply [:mkaply]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d7eb689bb5b

Comment 42

[Will Bamberg [:wbamberg]]

I've added something on this, but please let me know if we need anything else: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/chrome_settings_overrides.

Comment 43

[Mike Kaply [:mkaply]]

looks good.