Move support for setting the homepage from chrome_url_overrides to chrome_settings_overrides

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: General
P2
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: mkaply, Assigned: mkaply)

Tracking

({dev-doc-complete})

Trunk
mozilla55
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 wontfix, firefox55 fixed)

Details

(Whiteboard: [triaged])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 months ago
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 hidden (mozreview-request)
(Assignee)

Comment 2

6 months ago
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

6 months ago
mozreview-review
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.
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.
Blocks: 1298950
Comment hidden (mozreview-request)

Updated

6 months ago
Assignee: nobody → mozilla
Priority: -- → P2
Whiteboard: [triaged]
Comment hidden (mozreview-request)

Comment 7

6 months ago
mozreview-review
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

6 months ago
pardon the drive-by comment, but we have the ExtensionSettingStore landed now, this should use that.
(Assignee)

Comment 9

6 months ago
> 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

6 months ago
(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

6 months ago
mozreview-review
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?
(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/.
(Assignee)

Comment 13

6 months ago
I'll wait to finish this up until the new store has landed.
Depends on: 1341277
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

6 months ago
mozreview-review
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)
Attachment #8839719 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)

Comment 18

6 months ago
mozreview-review
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...
Attachment #8843526 - Flags: review?(kmaglione+bmo) → review-
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

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

clearing the r? to me since Kris responded
Attachment #8843526 - Flags: review?(aswan)
(Assignee)

Updated

6 months ago
Depends on: 1344773
Comment hidden (mozreview-request)
Comment on attachment 8843526 [details]
Bug 1341458 - Move homepage to chrome_settings_overrides.

work jumped back to mkaply, obsoleting this
Attachment #8843526 - Attachment is obsolete: true

Comment 23

6 months ago
mozreview-review
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

6 months ago
mozreview-review
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
(Assignee)

Comment 25

6 months ago
> 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.
Bob, can you give a final ok on the changes Mike will do for ExtensionPreferencesManager use?
Flags: needinfo?(bob.silverberg)
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 hidden (mozreview-request)
(Assignee)

Comment 29

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

Comment 30

6 months ago
mozreview-review
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
Attachment #8839719 - Flags: review+
(Assignee)

Comment 31

6 months ago
> 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?
(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.
Flags: needinfo?(bob.silverberg)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

6 months ago
OK, I think this covers everything.

Comments from bsilverberg addressed, new test added for disabling.

Comment 35

6 months ago
mozreview-review
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 = "id@tests.mozilla.org";
> +
> +  // Create an array of extensions to install.

You're only creating one extension, so this comment is incorrect.
Attachment #8839719 - Flags: review+
Depends on: 1345583
Comment hidden (mozreview-request)
Attachment #8843526 - Attachment is obsolete: false
Attachment #8843526 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 39

5 months ago
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/1af0390360c1
Move homepage to chrome_settings_overrides. r=bsilverberg,mixedpuppy

Comment 40

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1af0390360c1
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 41

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d7eb689bb5b
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.
Flags: needinfo?(mozilla)
Keywords: dev-doc-needed
(Assignee)

Comment 43

4 months ago
looks good.
Flags: needinfo?(mozilla)
Keywords: dev-doc-needed → dev-doc-complete
status-firefox54: affected → wontfix
You need to log in before you can comment on or make changes to this bug.