Closed Bug 1341458 Opened 8 years ago Closed 7 years ago

Move support for setting the homepage from chrome_url_overrides to chrome_settings_overrides

Categories

(WebExtensions :: General, defect, P2)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox54 wontfix, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [triaged])

Attachments

(1 file, 1 obsolete file)

Bug 1341458 - Move homepage to chrome_settings_overrides.
59 bytes, text/x-review-board-request
mixedpuppy
: review+
Details 
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.
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 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
Assignee: nobody → mozilla
Priority: -- → P2
Whiteboard: [triaged]
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?
pardon the drive-by comment, but we have the ExtensionSettingStore landed now, this should use that.
> 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?
(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 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/.
I'll wait to finish this up until the new store has landed.
Depends on: 1341277
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 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 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)
Depends on: 1344773
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 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 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
> 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.
New patch on mozreview has the additional test requested by mixedpuppy and the switch of the API mentioned by 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
Attachment #8839719 - Flags: review+
> 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)
OK, I think this covers everything.

Comments from bsilverberg addressed, new test added for disabling.
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
Attachment #8843526 - Attachment is obsolete: false
Attachment #8843526 - Attachment is obsolete: true
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/1af0390360c1
Move homepage to chrome_settings_overrides. r=bsilverberg,mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/1af0390360c1
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
looks good.
Flags: needinfo?(mozilla)
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: