Closed Bug 1397809 Opened 7 years ago Closed 6 years ago

Notify user on home page when an extension has updated it

Categories

(WebExtensions :: Frontend, enhancement, P3)

enhancement

Tracking

(firefox57 wontfix, firefox61 verified)

VERIFIED FIXED
mozilla61
Tracking Status
firefox57 --- wontfix
firefox61 --- verified

People

(Reporter: mconca, Assigned: mstriemer)

References

(Blocks 2 open bugs)

Details

Attachments

(6 files, 1 obsolete file)

When an a user views their home page controlled by an extension for the first time, they should be notified that this page is managed by an extension and be shown options to keep the changes or revert to the stock home page.

This is analogous to what was done for the New Tab override.
https://bugzilla.mozilla.org/show_bug.cgi?id=1390158
New Tab Mock: https://mozilla.invisionapp.com/share/6HCITJKP8#/screens/244736432
Blocks: 1342584
Severity: normal → enhancement
Priority: -- → P3
Assignee: nobody → mstriemer
Depends on: 1429590
Andrew is out so I'm sending this your way, Luca. If you test it locally the heading will be missing, this was regressed by bug 1369482. I added a patch to that bug which should fix it, so if that lands then this changeset should be fine. Otherwise it will need a minor tweak to use `startlabel` instead of `label`.
Depends on: 1369482
Attached image homepage-doorhanger.mov.gif (obsolete) —
Comment on attachment 8943416 [details]
Bug 1397809 - Part 1: Convert New Tab doorhanger to a generic class

https://reviewboard.mozilla.org/r/213730/#review219982

Hi Mark, here is a round of review comments on this first patch, most of the review comments are about nits, and the other ones are related to improvements or questions about the reasons behind some of the code from this patch.

I've been looking in most of this code for the first time, I did my best to provide as much useful feedback as possible (but I'd like if someone from our team that has worked or reviewed this pieces before could give a brief look, it could be Shane, which it looks that has reviewed some of this code before, or Bob which has worked on the webextension settings code a lot).

The customizableui pieces should also be signed off by jaws.

::: browser/components/customizableui/content/panelUI.inc.xul:709
(Diff revision 1)
>         hidden="true"
>         flip="slide"
>         position="bottomcenter topright"
>         tabspecific="true">
>    <popupnotification id="extension-new-tab-notification"
> +                     class="extension-controlled-notification"

This change and the one to the related css file looks good to me given the refactoring introduced by this patch, but it prefer if jaws would give his explicit sign-off on these two files (this xul file and the related css file)

::: browser/components/extensions/ExtensionControlledPopup.jsm:17
(Diff revision 1)
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "ExtensionSettingsStore",

Nit, we can merge this three `XPCOMUtils.defineLazyModuleGetter` calls into a single `XPCOMUtils.defineLazyModuleGetters` (e.g. like we do here: https://searchfox.org/mozilla-central/rev/2031c0f517185b2bd0e8f6f92f9491b3410c1f7f/toolkit/components/extensions/ext-storage.js#6-11)

::: browser/components/extensions/ExtensionControlledPopup.jsm:25
(Diff revision 1)
> +                                  "resource://gre/modules/AddonManager.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
> +                                  "resource:///modules/CustomizableUI.jsm");
> +
> +class ExtensionControlledPopup {
> +  constructor(opts) {

Given that `opts` doesn't tell much about what this class is going to get as parameters in the contructor, and looking at the rest of the class methods it looks that there a number of parameter that has to be there to make an instance of this class to work as expected (e.g. `confirmedType`, `observerEvent`, `onObserverAdded`, `onObserverRemoved`, `settingType`, `settingKey`, `windowTracker` and more)

I think that we should add some jsdoc here to detail what `opts`, in particular what are its property and property types and what role they have in the `ExtensionControlledPopup`.

When the actual parameters are the properties on a single parameter of type object, we are usually using the following convention:

- https://searchfox.org/mozilla-central/rev/2031c0f517185b2bd0e8f6f92f9491b3410c1f7f/toolkit/components/extensions/Extension.jsm#868-873

::: browser/components/extensions/ExtensionControlledPopup.jsm:26
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
> +                                  "resource:///modules/CustomizableUI.jsm");
> +
> +class ExtensionControlledPopup {
> +  constructor(opts) {
> +    // Keep opts as an object so `this` is maintained for it.

I'm not sure that I get this comment, maybe I'm missing something? (or we could make it more clear)

::: browser/components/extensions/ExtensionControlledPopup.jsm:33
(Diff revision 1)
> +    this.observerRegistered = false;
> +    this.observer = {
> +      observe: (subject, topic, data) => {
> +        // Do this work in an idle callback to avoid interfering with new tab performance tracking.
> +        this.opts.windowTracker
> +          .getCurrentWindow({})

it looks that it would be better to use [`windowTracker.topWindow`][1] here instead of  `windowTracker.getCurrent({})`, where `{}` is basically a `fake context object` that we are passing to `getCurrent` only to make it fallback to [`windowTracker.topWindow`][2].

Same for the `windowTracker.getCurrent` call at line 84.

[1]: https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-tabs-base.js#1285-1287
[2]: https://searchfox.org/mozilla-central/rev/1f9b4f0f0653b129b4db1b9fc5e9068054afaac0/toolkit/components/extensions/ext-tabs-base.js#1311-1313

::: browser/components/extensions/ExtensionControlledPopup.jsm:39
(Diff revision 1)
> +          .requestIdleCallback(this.handleEvent.bind(this));
> +      },
> +    };
> +  }
> +
> +  userWasNotified(id) {

it looks that this method is called to check if the "user has made his choice" (by pressing one of the two buttons in the doorhanger), and so I'm wondering if a different name for this method would make it much more clear (e.g. something like `hasUserChoice` or `isUserConfirmed`).

::: browser/components/extensions/ExtensionControlledPopup.jsm:74
(Diff revision 1)
> +    // We don't need to open the doorhanger again until the controlling add-on changes.
> +    this.removeObserver();

It looks that we were removing the observer around here even in the previous version of this code, but I'm wondering if, given that it seems that we always want to observe the observer service notification we are looking for only once, maybe we could call `this.removeObserver` from the `this.observer.observe` method (around line 31), so that we can't receive another handleEvent call while waiting for the idle callback to be called.

::: browser/components/extensions/ExtensionControlledPopup.jsm:79
(Diff revision 1)
> +    // We don't need to open the doorhanger again until the controlling add-on changes.
> +    this.removeObserver();
> +
> +    let item = ExtensionSettingsStore.getSetting(this.opts.settingType, this.opts.settingKey);
> +
> +    if (!item || !item.id || this.userWasNotified(item.id)) {

I see that this check has been just moved mostly unmodified from the ext-url-overrides.js, https://searchfox.org/mozilla-central/rev/2031c0f517185b2bd0e8f6f92f9491b3410c1f7f/browser/components/extensions/ext-url-overrides.js#51,53-55
but I'm wondering which are the conditions that make `ExtensionSettingsStore.getSetting` to return an `item` without an `id`? an inline comment which explain when we are going to return early from this function may be helpful.

::: browser/components/extensions/ExtensionControlledPopup.jsm:87
(Diff revision 1)
> +
> +    // Find the elements we need.
> +    let win = this.opts.windowTracker.getCurrentWindow({});
> +    let doc = win.document;
> +    let panel = doc.getElementById("extension-notification-panel");
> +    let popupnotification = doc.getElementById(this.opts.popupnotificationId);

How about also check if popupnotification has not been found here and raise an error message that could make it easier to investigate an issue related to a caller which as mistyped the popupnotificationId?

::: browser/components/extensions/ext-url-overrides.js:39
(Diff revision 1)
>    gBrowser.loadURIWithFlags(
>      url, {flags: Ci.nsIWebNavigation.LOAD_FLAGS_REPLACE_HISTORY});
>    return loaded;
>  }
>  
> -async function handleNewTabOpened() {
> +let newTabPopup = new ExtensionControlledPopup({

Can we create the ExtensionControlledPopup lazily?

(we can use XPCOMUtils.defineLazyGetter like we do in other WebExtensions API modules, e.g. https://searchfox.org/mozilla-central/rev/2031c0f517185b2bd0e8f6f92f9491b3410c1f7f/browser/components/extensions/ext-browsingData.js#32-36)

::: browser/components/extensions/ext-url-overrides.js:45
(Diff revision 1)
> -  // We don't need to open the doorhanger again until the controlling add-on changes.
> -  // eslint-disable-next-line no-use-before-define
> -  removeNewTabObserver();
> -
> -  let item = ExtensionSettingsStore.getSetting(STORE_TYPE, NEW_TAB_SETTING_NAME);
> -
> +  confirmedType: NEW_TAB_CONFIRMED_TYPE,
> +  settingType: STORE_TYPE,
> +  settingKey: NEW_TAB_SETTING_NAME,
> +  observerEvent: "browser-open-newtab-start",
> +  popupnotificationId: "extension-new-tab-notification",
> +  onObserverAdded() {

Nit, how about keeping these properties alphabetically ordered? 
(or at least group the onObserverAdded/onObserverRemoved/etc callbacks together)

::: browser/components/extensions/ext-url-overrides.js:59
(Diff revision 1)
> -      // Secondary action is to restore settings. Disabling an add-on should remove
> -      // the tabs that it has open, but we want to open the new New Tab in this tab.
> -      //   1. Replace the tab's URL with about:blank, wait for it to change
> +    //   1. Replace the tab's URL with about:blank, wait for it to change
> -      //   2. Now that this tab isn't associated with the add-on, disable the add-on
> +    //   2. Now that this tab isn't associated with the add-on, disable the add-on
> -      //   3. Replace the tab's URL with the new New Tab URL
> +    //   3. Replace the tab's URL with the new New Tab URL
> -      ExtensionSettingsStore.removeSetting(NEW_TAB_CONFIRMED_TYPE, item.id);
> +    let win = windowTracker.getCurrentWindow({});

Same as for the ExtensionControllerPopup.jsm, how about using `windowTracker.topWindow` here instead of `windowTracker.getCurrentWindow({})`?

::: browser/components/extensions/ext-url-overrides.js:66
(Diff revision 1)
> -      let tab = gBrowser.selectedTab;
> +    let tab = gBrowser.selectedTab;
> -      await replaceUrlInTab(gBrowser, tab, "about:blank");
> +    await replaceUrlInTab(gBrowser, tab, "about:blank");
> -      Services.obs.addObserver({
> +    Services.obs.addObserver({
> -        async observe() {
> +      async observe() {
> -          await replaceUrlInTab(gBrowser, tab, aboutNewTabService.newTabURL);
> +        await replaceUrlInTab(gBrowser, tab, aboutNewTabService.newTabURL);
> -          handleNewTabOpened();
> +        doorhanger.handleEvent();

Nit, I guess that we are calling `doorhander.handleEvent()` here in case the newtab is still controlled by another installed extension that wanted to control the newtab, and so an inline comment like the one added in the second patch in ext-chrome-settings-overrides.js could be useful.

We could also briefly mention that is `ExtensionControlledPopup` that is going to disable the addon internally, right after calling this onRestored async callback, and that is going to allow to another installed extension to inherit the control over the newtab (mostly because in the previous version `addon.userDisabled = true;` was immediately after this code, but in the new version is hidden inside the new `ExtensionControlledPopup`).

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:11
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "ExtensionSettingsStore",
> +                                  "resource://gre/modules/ExtensionSettingsStore.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "ExtensionControlledPopup",
> +                                  "resource:///modules/ExtensionControlledPopup.jsm");
> +
> +add_task(async function testExtensionControlledPopup() {

Nit, it could be helpful to mention in an inline comment that this test is using an empty extension because its goal is to "unit test" the ExtensionControlledPopup class itself and not its usage in the newtab and homepage doorhanger (which have their own tests with non-empty test extensions in separate test files).

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:15
(Diff revision 1)
> +
> +add_task(async function testExtensionControlledPopup() {
> +  let id = "ext-controlled@mochi.test";
> +  let extension = ExtensionTestUtils.loadExtension({
> +    manifest: {applications: {gecko: {id}}},
> +    useAddonManager: "temporary",

Nit, I would add a comment here to explain why we need the extension to use the "temporary" mode (which I'm pretty sure that is because we need the AddonManager to know about the test extension).

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:30
(Diff revision 1)
> +  let settingType = "extension-controlled";
> +  let settingKey = "some-key";
> +  let observerAdded = null;
> +  let restored = null;
> +  let popup = new ExtensionControlledPopup({
> +    confirmedType,

Nit, like in one of the previous comments: how about keeping these properties alphabetically ordered? 
(or at least group the onObserverAdded/onObserverRemoved/etc callbacks together)

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:48
(Diff revision 1)
> +    async onRestore(doorhanger) {
> +      restored = true;
> +    },
> +  });
> +
> +  let doc = windowTracker.getCurrentWindow({}).document;

Nit, this could also be just `windowTracker.topWindow.document`

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:50
(Diff revision 1)
> +    },
> +  });
> +
> +  let doc = windowTracker.getCurrentWindow({}).document;
> +  let panel = doc.getElementById("extension-notification-panel");
> +  let popupnotification = doc.createElement("popupnotification");

we could move the code that create the `popupnotification` XUL element in an helper function outside of the test case body, so that the test case itself can be easier to read.

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:69
(Diff revision 1)
> +  };
> +  Object.keys(attributes).forEach(key => {
> +    popupnotification.setAttribute(key, attributes[key]);
> +  });
> +  popupnotification.innerHTML = `
> +      <popupnotificationcontent orient="vertical">

Nit, even if it is just a template string (and so eslint is not going to complain), how about indent its content with 2 spaces instead of 4?

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:77
(Diff revision 1)
> +        </description>
> +      </popupnotificationcontent>
> +  `;
> +  panel.appendChild(popupnotification);
> +
> +  // Test that the popup can be ignored.

Does this comment mean that "creating the ExtensionControlledPopup instance doesn't mean that it is already actively listening for the event that it is supposed to observe"?

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:94
(Diff revision 1)
> +  let popupShown = promisePopupShown(panel);
> +  Services.obs.notifyObservers(null, "extension-controlled-event");
> +  await popupShown;

It seems that we repeat this sequence a couple of times during this test case, we could group it into an helper function (even just one that is defined inside the test case body itself) and reuse it to make the test case a bit shorter
(and maybe there is also some other parts of it that could be grouped in an helper function to be reused inside the test).

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:101
(Diff revision 1)
> +  await popupShown;
> +
> +  is(observerAdded, false, "Done observing the event");
> +  is(restored, null, "Settings have not been restored");
> +  is(panel.getAttribute("panelopen"), "true", "The panel is open");
> +  is(popupnotification.hidden, false, "The popup is open");

Nit, How about "the panel content is visible" instead?

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:102
(Diff revision 1)
> +
> +  is(observerAdded, false, "Done observing the event");
> +  is(restored, null, "Settings have not been restored");
> +  is(panel.getAttribute("panelopen"), "true", "The panel is open");
> +  is(popupnotification.hidden, false, "The popup is open");
> +  is(popup.userWasNotified(id), false, "The user has not been notified");

Nit, how about "The user did not made a choice yet"? 
(it is just that I personally read "The user has not been notified" as "The user has received an open popup to be notified of the change")

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:167
(Diff revision 1)
> +  is(restored, true, "The onRestore callback was fired");
> +  is(popup.userWasNotified(id), false, "The user has not confirmed");
> +  is(addon.userDisabled, true, "The extension is now disabled");
> +
> +  // Cleanup the DOM and unload extension.
> +  popupnotification.remove();

what if the test fails before removing the injected popupnotification XUL element?
(e.g. we could register a cleanup function when we inject it to be sure that even in the worst case, "the test fails and exits before being able to remove it", we are going to remove it and prevent it to make other unrelated tests to fail)
Comment on attachment 8943417 [details]
Bug 1397809 - Part 2: Add a doorhanger when an extension changes the homepage

https://reviewboard.mozilla.org/r/213732/#review219992

And here is a round of review comments for the second patch.

As for the first patch:
- the customizableui and the dtd pieces should also be signed off by jaws
- the pieces in browser.js should also be signed off by Gijs (or someone that he may suggest)
- it would be great if someone from our team that has worked or reviewed these pieces before could give a brief second look (e.g. Shane or Bob)

::: browser/base/content/browser.js:2151
(Diff revision 2)
>  function BrowserGoHome(aEvent) {
>    if (aEvent && "button" in aEvent &&
>        aEvent.button == 2) // right-click: do nothing
>      return;
>  
> +  Services.obs.notifyObservers(null, "browser-open-homepage-start");

This change may use an inline comment that explain why we are notifying "browser-open-homepage-start" here and what component/components are going to observe it.

This should also be signed-off by someone that has worked on or reviewed patches for the `browser/base/content/browser.js` file (e.g. Gijs is probably a good pick for this).

(to be fair we should also add an inline comment near where "browser-open-newtab-start" is notified, https://searchfox.org/mozilla-central/rev/2031c0f517185b2bd0e8f6f92f9491b3410c1f7f/browser/base/content/browser.js#2263, which does have an inline comment but it only mention its usage  "for modular peformance tracking" and does not mention that it is also used by the webextensions internals)

::: browser/components/customizableui/content/panelUI.inc.xul:726
(Diff revision 2)
>        <description id="extension-new-tab-notification-description">
>          &newTabControlled.message;
>        </description>
>      </popupnotificationcontent>
>    </popupnotification>
> +  <popupnotification id="extension-homepage-notification"

Do you mind to also ask jaws to review and sign-off this newly added popupnotification? (it looks like he also reviewed the new-tab one from Bug 1390158)

::: browser/components/extensions/ext-chrome-settings-overrides.js:5
(Diff revision 2)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -/* globals windowTracker */
> +/* import-globals-from ext-browser.js */

Just as a side note: I recall that Kris wanted to remove any usage of `import-globals-from` from our internals, but I'm not sure what we were going to use instead (and to be fair we are currently using `import-globals-from`  in a lot of the WebExtensions internals: https://searchfox.org/mozilla-central/search?q=%2F*+import-globals-from&case=false&regexp=false&path=components%2Fextensions%2F)

::: browser/components/extensions/ext-chrome-settings-overrides.js:11
(Diff revision 2)
>  
>  "use strict";
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "ExtensionPreferencesManager",

Nit, we could also merge all this into a single `XPCOMUtils.defineLazyModuleGetters`

::: browser/components/extensions/ext-chrome-settings-overrides.js:50
(Diff revision 2)
> +function waitForTabLoaded(tab) {
> +  return new Promise(resolve => {
> +    windowTracker.addListener("progress", {
> +      onLocationChange(browser, webProgress, request, locationURI, flags) {
> +        if (webProgress.isTopLevel
> +            && browser.ownerGlobal.gBrowser.getTabForBrowser(browser) == tab) {
> +          windowTracker.removeListener(this);
> +          resolve();
> +        }
> +      },
> +    });
> +  });
> +}
> +
> +function replaceUrlInTab(gBrowser, tab, url) {
> +  let loaded = waitForTabLoaded(tab);
> +  gBrowser.loadURIWithFlags(
> +    url, {flags: Ci.nsIWebNavigation.LOAD_FLAGS_REPLACE_HISTORY});
> +  return loaded;
> +}

It looks like we have a pretty similar helper function in ext-url-overrides.js, can we share one implementation for both?

::: browser/components/extensions/ext-chrome-settings-overrides.js:71
(Diff revision 2)
> +  gBrowser.loadURIWithFlags(
> +    url, {flags: Ci.nsIWebNavigation.LOAD_FLAGS_REPLACE_HISTORY});
> +  return loaded;
> +}
> +
> +let popup = new ExtensionControlledPopup({

it would be easier to understand what popup is if we give it a more specific name, e.g. the other one is currently named `newTabPopup` and we could call this one `homepagePopup` (or `homepageControlledPopup`)


Like for the `newTabPopup`, can we create this lazily?

::: browser/components/extensions/ext-chrome-settings-overrides.js:85
(Diff revision 2)
> +    // Disabling an add-on should remove the tabs that it has open, but we want
> +    // to open the new homepage in this tab (which might get closed).
> +    //   1. Replace the tab's URL with about:blank, wait for it to change
> +    //   2. Now that this tab isn't associated with the add-on, disable the add-on
> +    //   3. Trigger the browser's homepage method
> +    let win = windowTracker.getCurrentWindow({});

Nit, or just `windowTracker.topWindow`

::: browser/components/extensions/ext-chrome-settings-overrides.js:174
(Diff revision 2)
>  
>    async onManifestEntry(entryName) {
>      let {extension} = this;
>      let {manifest} = extension;
>  
>      await ExtensionSettingsStore.initialize();

Nit, we could add an empty line between the `await ...` and the `if (...)` (just to aid the readability and being consistent to the similar code from ext-url-overrides.js in the other patch).

::: browser/components/extensions/ext-chrome-settings-overrides.js:310
(Diff revision 2)
> +  onSetPrefs(item) {
> +    if (item.id) {
> +      popup.addObserver(item.id);
> +    } else {
> +      popup.removeObserver();
> +    }
> +  },

This `onSetPrefs` callback seems to be used as part of the mechanism that allows to notify the user that another extension is controlling the homepage after the extension that was previously controling it has been disabled, am I right? (an inline comment above it could help to make immediately clear what is its role).

::: browser/components/extensions/test/browser/browser_ext_chrome_settings_overrides_home.js:278
(Diff revision 2)
> +add_task(async function test_doorhanger() {
> +  let defaultHomePage = getHomePageURL();
> +  let ext1 = ExtensionTestUtils.loadExtension({
> +    manifest: {"chrome_settings_overrides": {"homepage": "ext1.html"}},
> +    files: {"ext1.html": "<h1>1</h1>"},
> +    useAddonManager: "temporary",

Nit, we could mention in an inline comment that both these test extension are using the "temporary" mode to: "to be recognized by the AddonManager" and "so that the extension shutdown handlers are going to be called"

::: browser/components/extensions/test/browser/browser_ext_chrome_settings_overrides_home.js:307
(Diff revision 2)
> +  document.getAnonymousElementByAttribute(popupnotification, "anonid", "secondarybutton").click();
> +  await prefPromise;
> +  await popupHidden;
> +
> +  // Expect a new doorhanger for the next extension.
> +  await promisePopupShown(panel);

I'm wondering if it would be better to create the promise before we click the secondarybutton on the doorhanger, so that we don't risk this await to fail intermittently (if the popupshown event would be sent before we create the promise that is going to listen for it), e.g. like we are doing for the popupHidden and prefPromise promises.

Nit, I usually try to make it clear when an identifier is a promise by prefixing its name with `on` or `once` (e.g. `popupHidden` -> `oncePopupHidden`, `prefPromise` -> `oncePrefChanged` etc.)

::: browser/locales/en-US/chrome/browser/browser.dtd:981
(Diff revision 2)
> +<!ENTITY homepageControlled.message "An extension has changed what you see as your home page. You can restore your settings if you do not want this change.">
> +<!ENTITY homepageControlled.header.message "Your home page has changed.">
> +<!ENTITY homepageControlled.keepButton.label "Keep Changes">
> +<!ENTITY homepageControlled.keepButton.accesskey "K">
> +<!ENTITY homepageControlled.restoreButton.label "Restore Settings">
> +<!ENTITY homepageControlled.restoreButton.accesskey "R">

This new entities looks ok to me, but I would prefer that they would also receive a final signoff from jaws (like for the changes to panelUI.inc.xul)

::: toolkit/components/extensions/ExtensionPreferencesManager.jsm:105
(Diff revision 2)
>        Preferences.set(pref, prefs[pref]);
> +      changed = true;
> +    }
> -    }
> +  }
> +  if (changed && typeof setting.onSetPrefs == "function") {
> +    setting.onSetPrefs(item);

The difference between the `setting.setCallback` and this new `setting.onSetPrefs` is clear once you read the code that use both, but:

- it is not easy to guess by their name
- we should also update the jsdoc comment on setPrefs to mention onSetPrefs (and make it if and when they are optional)

I would prefer to rename at least the new one so that it is easier to guess its role (e.g. `onChanged`?).

(I would also prefer if `setCallback` was called `getPrefsToSet` or another name that makes it clear that it doesn't set anything on its own, it just return the map of the preferences to set, at least if I'm not reading it wrong, but no need to renamed it in this patch, we can evaluate if we want to do that and if we do we can do it in a follow up).

::: toolkit/components/extensions/test/xpcshell/test_ext_extensionPreferencesManager.js:86
(Diff revision 2)
>    for (let pref of settingObj.prefNames) {
>      equal(Preferences.get(pref), settingObj.valueFn(pref, value), msg);
>    }
>  }
>  
> +function checkOnSetPrefs(setting, value, msg) {

This helper doesn't do anything if the setting in not "singlePref", which is reasonable because that is the only one that has a `setPrefs` callback defined, but we are using the helper inside the for that loops over all the settings that we are going to test, but I'm wondering if we could make it more clear, e.g. by not calling this helper at all when the `settingObj` doesn't have a `setPrefs` callback (and removing the `if (setting == "singlePrefs")` check the helper function body).
Comment on attachment 8943417 [details]
Bug 1397809 - Part 2: Add a doorhanger when an extension changes the homepage

https://reviewboard.mozilla.org/r/213732/#review219992

> I'm wondering if it would be better to create the promise before we click the secondarybutton on the doorhanger, so that we don't risk this await to fail intermittently (if the popupshown event would be sent before we create the promise that is going to listen for it), e.g. like we are doing for the popupHidden and prefPromise promises.
> 
> Nit, I usually try to make it clear when an identifier is a promise by prefixing its name with `on` or `once` (e.g. `popupHidden` -> `oncePopupHidden`, `prefPromise` -> `oncePrefChanged` etc.)

`promisePopupShown()` will resolve right away if the popup is currently shown. So we don't need to handle that case, it also means we can't create this promise any earlier.

https://searchfox.org/mozilla-central/rev/b7e3ec2468d42fa59d86c03ec7afeb209813f1d4/browser/components/extensions/test/browser/head.js#112
Luca has done an initial review but wanted jaws to look over the frontend changes and someone who's seen this code (aswan) to take another look.

@Gijs this also adds an observer notification to `BrowserGoHome()` in browser/base/content/browser.js [1]. Do these changes look okay?

[1] https://reviewboard.mozilla.org/r/213732/diff/3#1
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8943417 [details]
Bug 1397809 - Part 2: Add a doorhanger when an extension changes the homepage

https://reviewboard.mozilla.org/r/213732/#review220696


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/extensions/ext-browser.js:109
(Diff revision 3)
>    // FIXME: This allows for collisions.
>    return id.replace(/[^a-z0-9_-]/g, "_");
>  };
>  
> +
> +function replaceUrlInTab(gBrowser, tab, url) {

Error: 'replaceUrlInTab' is defined but never used. Allowed unused vars must match /^(Cc|Ci|Cr|Cu|EXPORTED_SYMBOLS)$/. [eslint: no-unused-vars]
(In reply to Mark Striemer [:mstriemer] from comment #11)
> Luca has done an initial review but wanted jaws to look over the frontend
> changes and someone who's seen this code (aswan) to take another look.
> 
> @Gijs this also adds an observer notification to `BrowserGoHome()` in
> browser/base/content/browser.js [1]. Do these changes look okay?
> 
> [1] https://reviewboard.mozilla.org/r/213732/diff/3#1

Eh, I don't love use of the observer service for this but I don't have a much better idea...

... that said, the summary of this bug is about the user seeing their homepage. The normal reason users see that page isn't actually the user clicking the 'home' button, but will be opening a new window (or opening the first window, for users who don't restore tabs from last session automatically, which is most users). From that perspective, it seems better to detect navigation to a particular URL, though that will obviously also trigger when an add-on has changed the homepage and the user individually navigates to the homepage URL. Alternatively, you'd need to add extra code to catch those 2 cases...
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mstriemer)
Comment on attachment 8943416 [details]
Bug 1397809 - Part 1: Convert New Tab doorhanger to a generic class

https://reviewboard.mozilla.org/r/213730/#review221040

Somebody like florian or jaws should also look closely at the frontend bits, but in the mean time, a few nits/questions about the API to ExtensionControlledPopup

::: browser/components/extensions/ExtensionControlledPopup.jsm:38
(Diff revision 2)
> +
> +class ExtensionControlledPopup {
> +  /* Provide necessary options for the popup.
> +   *
> +   * @param {object} opts Options for configuring popup.
> +   * @param {string} info.confirmedType

info -> opts in each `@param`

::: browser/components/extensions/ExtensionControlledPopup.jsm:41
(Diff revision 2)
> +   * @param {function} info.makeWidgetId
> +   *                   The makeWidgetId function from ext-browser.js.

Can we just have the caller pass in a widget id instead of this?

::: browser/components/extensions/ExtensionControlledPopup.jsm:54
(Diff revision 2)
> +   * @param {object} info.windowTracker
> +   *                 The windowTracker object from ext-browser.js.

if this is just for getting the top window, how about just using `Services.wm.getMostRecentWindow()` directly?

::: browser/components/extensions/ExtensionControlledPopup.jsm:63
(Diff revision 2)
> +   * @param {function} info.onRestore
> +   *                   A function that is called before disabling an extension when the
> +   *                   user decides to disable the extension. If this function is async
> +   *                   then the extension won't be disabled until it is fulfilled.

The "Restore" in the name refers to restoring the underlying page?  That confused me at first, `onDisableExtension` or something might be clearer?

::: browser/components/extensions/ExtensionControlledPopup.jsm:69
(Diff revision 2)
> +   *                   A function that is called before disabling an extension when the
> +   *                   user decides to disable the extension. If this function is async
> +   *                   then the extension won't be disabled until it is fulfilled.
> +   */
> +  constructor(opts) {
> +    // Keep opts as an object so `this` is maintained when calling methods on it.

I don't get this, won't this make `this` bound to the opts object?

::: browser/components/extensions/ExtensionControlledPopup.jsm:119
(Diff revision 2)
> +        this.opts.onObserverAdded();
> +      }
> +    }
> +  }
> +
> +  async handleEvent() {

Could we name this `showNotification()` (or `maybeShowNotification()` or something more descriptive?
Flags: needinfo?(mstriemer)
See Also: → webextensions-startup
Comment on attachment 8943416 [details]
Bug 1397809 - Part 1: Convert New Tab doorhanger to a generic class

https://reviewboard.mozilla.org/r/213730/#review221132

I'll clear review of the patch for now since aswan has done a good round of feedback already.

::: browser/components/extensions/ExtensionControlledPopup.jsm:23
(Diff revision 2)
> + * the change by triggering the primary action.
> + */
> +
> +var EXPORTED_SYMBOLS = ["ExtensionControlledPopup"];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

Please pay attention to bug 767640, as this may not be needed by the time you land this.

::: browser/components/extensions/ExtensionControlledPopup.jsm:69
(Diff revision 2)
> +   *                   A function that is called before disabling an extension when the
> +   *                   user decides to disable the extension. If this function is async
> +   *                   then the extension won't be disabled until it is fulfilled.
> +   */
> +  constructor(opts) {
> +    // Keep opts as an object so `this` is maintained when calling methods on it.

I agree, the use of the arrow function below will maintain the `this` reference.
Attachment #8943416 - Flags: review?(jaws)
Comment on attachment 8943417 [details]
Bug 1397809 - Part 2: Add a doorhanger when an extension changes the homepage

https://reviewboard.mozilla.org/r/213732/#review222096

::: browser/locales/en-US/chrome/browser/browser.dtd:981
(Diff revision 3)
>  <!ENTITY newTabControlled.keepButton.label "Keep Changes">
>  <!ENTITY newTabControlled.keepButton.accesskey "K">
>  <!ENTITY newTabControlled.restoreButton.label "Restore Settings">
>  <!ENTITY newTabControlled.restoreButton.accesskey "R">
>  
> +<!ENTITY homepageControlled.message "An extension has changed what you see as your home page. You can restore your settings if you do not want this change.">

We should include the name of the extension in this message so users will know what extension they can disable if they don't want to disable it right at this moment.
Attachment #8943417 - Flags: review?(jaws) → review-
See Also: → 1414029
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> Comment on attachment 8943417 [details]
> > +<!ENTITY homepageControlled.message "An extension has changed what you see as your home page. You can restore your settings if you do not want this change.">
> 
> We should include the name of the extension in this message so users will
> know what extension they can disable if they don't want to disable it right
> at this moment.

We had discussed this in the New Tab doorhanger as well (bug 1390158 comment 40 has Markus's response). There it was decided that we didn't need the extension name since it is in the identity block. In the homepage case however the extension name might not be in the identity bar like in the Yandex - Homepage [1] extension where it sets the homepage to yandex.ru.

Markus, do we want to include the extension name in the message here since it might not be in the identity block?

[1] https://addons.mozilla.org/en-US/firefox/addon/yandex-homepage/
Flags: needinfo?(mjaritz)
(In reply to Mark Striemer [:mstriemer] from comment #17)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #16)
> > Comment on attachment 8943417 [details]
> > > +<!ENTITY homepageControlled.message "An extension has changed what you see as your home page. You can restore your settings if you do not want this change.">
> > 
> > We should include the name of the extension in this message so users will
> > know what extension they can disable if they don't want to disable it right
> > at this moment.
> 
> We had discussed this in the New Tab doorhanger as well (bug 1390158 comment
> 40 has Markus's response). There it was decided that we didn't need the
> extension name since it is in the identity block. In the homepage case
> however the extension name might not be in the identity bar like in the
> Yandex - Homepage [1] extension where it sets the homepage to yandex.ru.
> 
> Markus, do we want to include the extension name in the message here since
> it might not be in the identity block?
> 
> [1] https://addons.mozilla.org/en-US/firefox/addon/yandex-homepage/

For the later case: people will also see what extension is controlling their home page in the home page settings in preferences. We do that, so that they do not need to remember.
Flags: needinfo?(mjaritz)
I updated this based on the comments above, I had to add another notifyObservers call. This time in nsBrowserContentHandler.js, I'm not sure if that's really the right spot though.

This now catches the case where a new window is opened. So if you install a homepage extension and open a new window you'll see the doorhanger. It still misses the first window though since the doorhanger code hasn't registered its observer yet.

I put together a not-so-pretty hack yesterday where I have nsBrowserContentHandler.js wait for an observer event from the extension before sending the open-homepage-start event. This likely has some race conditions but I thought it might work and it does in this case.

It seems like there should be a better way but I'm hesitant to import a bunch of code in nsBrowserContentHandler.js or browser.js.
Attachment #8943417 - Flags: review?(jaws) → review?(gijskruitbosch+bugs)
Attachment #8943416 - Flags: review?(jaws)
Attachment #8943416 - Flags: review?(gijskruitbosch+bugs)
Attachment #8943416 - Flags: review?(aswan)
Comment on attachment 8943416 [details]
Bug 1397809 - Part 1: Convert New Tab doorhanger to a generic class

/me stares at MozReview
Attachment #8943416 - Flags: review?(aswan)
There should be an update to this tomorrow to handle the initial homepage case without modifying nsBrowserContentHandler.js. It isn't a perfect solution but should work well enough.
(In reply to Mark Striemer [:mstriemer] from comment #23)
> There should be an update to this tomorrow to handle the initial homepage
> case without modifying nsBrowserContentHandler.js. It isn't a perfect
> solution but should work well enough.

I assume this means I should wait with reviewing this. Please let me know if not.
Comment on attachment 8943416 [details]
Bug 1397809 - Part 1: Convert New Tab doorhanger to a generic class

Clearing this for now per IRC / bug.
Attachment #8943416 - Flags: review?(gijskruitbosch+bugs)
Attachment #8943417 - Flags: review?(gijskruitbosch+bugs)
Attachment #8943416 - Flags: review?(aswan)
Attachment #8943417 - Flags: review?(aswan)
Comment on attachment 8943416 [details]
Bug 1397809 - Part 1: Convert New Tab doorhanger to a generic class

https://reviewboard.mozilla.org/r/213730/#review221040

> Can we just have the caller pass in a widget id instead of this?

We could probably pass a widget id to `addObserver()` and `open()` instead of holding on to this function. It seems simpler to make the widget ID when we need it to me though. I can make the change if you'd like.
Attached is a giant gif showing what I think are all the cases: startup, new windows and homepage button click.

This doesn't do anything for the case where the homepage is opened in the background.

The observer notification is triggered in BrowserGoHome() and OpenBrowserWindow(). When an extension starts at app startup it will check if it is on the homepage to handle the initial case since we'll likely miss the observer notification.
Attachment #8943423 - Attachment is obsolete: true
I'm afraid I'm likely not getting to this today - apologies for the delay. Hopefully tomorrow...
Comment on attachment 8943416 [details]
Bug 1397809 - Part 1: Convert New Tab doorhanger to a generic class

https://reviewboard.mozilla.org/r/213730/#review221040

> We could probably pass a widget id to `addObserver()` and `open()` instead of holding on to this function. It seems simpler to make the widget ID when we need it to me though. I can make the change if you'd like.

Hm, looks like we already have a copy of that function in ExtensionPopups.jsm :(  Can you just make that shareable and use it instead?  Its the passing around of the callable from ext-browser.js that I object to...
Comment on attachment 8943416 [details]
Bug 1397809 - Part 1: Convert New Tab doorhanger to a generic class

https://reviewboard.mozilla.org/r/213730/#review225854

I don't really love the way we're using ExtensionSettingsStore here but I guess its our best realistic option for the time being.

::: browser/components/extensions/ExtensionControlledPopup.jsm:100
(Diff revision 4)
> +  clearConfirmation(id) {
> +    return ExtensionSettingsStore.removeSetting(id, this.confirmedType, id);
> +  }

Is it the responsibility of users of this class to call this method when the related extension is uninstalled?  That should be documented very clearly if failing to do that means a storage leak (even if its a small leak)

::: browser/components/extensions/ExtensionControlledPopup.jsm:153
(Diff revision 4)
> +      if (event.originalTarget.getAttribute("anonid") == "button") {
> +        // Main action is to keep changes.
> +        await this.setConfirmation(item.id);
> +      } else {
> +        // Secondary action is to restore settings.
> +        await this.beforeDisableAddon(this);

Should the call to `pnael.hidePopup()` below happen before this call?  It seems like the popup should go away immediately to acknowledge that the user has pressed a button and we are handling that, even if some further work is still going to happen before the extension actually gets disabled...
Attachment #8943416 - Flags: review?(aswan) → review+
Comment on attachment 8943417 [details]
Bug 1397809 - Part 2: Add a doorhanger when an extension changes the homepage

https://reviewboard.mozilla.org/r/213732/#review225864

A few questions to resolve.  Also, I assume Gijs will evaluate the browser.js bits, I don't know enough to properly evaluate those changes.

::: browser/components/extensions/ExtensionControlledPopup.jsm:81
(Diff revision 5)
>        observe: (subject, topic, data) => {
>          // We don't need to open the doorhanger again until the controlling add-on changes.
>          this.removeObserver();
>  
>          // Do this work in an idle callback to avoid interfering with new tab performance tracking.
> -        this.topWindow.requestIdleCallback(this.open.bind(this));
> +        this.topWindow.requestIdleCallback(this.open.bind(this, subject));

Can you add some documentation to the ExtensionControlledPopup constructor that the observed event should have the window as its subject or else bad things will happen?

::: browser/components/extensions/ExtensionControlledPopup.jsm:125
(Diff revision 5)
>        }
>      }
>    }
>  
> -  async open() {
> +  async open(targetWindow) {
> +    // Don't listen anymore if the popup has been manually opened.

I don't understand this comment?

::: browser/components/extensions/ext-chrome-settings-overrides.js:62
(Diff revision 5)
> +      // Disabling an add-on should remove the tabs that it has open, but we want
> +      // to open the new homepage in this tab (which might get closed).
> +      //   1. Replace the tab's URL with about:blank, wait for it to change
> +      //   2. Now that this tab isn't associated with the add-on, disable the add-on
> +      //   3. Trigger the browser's homepage method
> +      let win = windowTracker.topWindow;

Is this safe?  I think you could get the actual window from `doorhanger.ownerDocument`?

::: browser/components/extensions/ext-chrome-settings-overrides.js:66
(Diff revision 5)
> +      //   3. Trigger the browser's homepage method
> +      let win = windowTracker.topWindow;
> +      let gBrowser = win.gBrowser;
> +      let tab = gBrowser.selectedTab;
> +      await replaceUrlInTab(gBrowser, tab, "about:blank");
> +      Preferences.observe(HOMEPAGE_PREF, async function prefObserver() {

Can you just use `Services.prefs.addObserver()` here

::: browser/components/extensions/ext-chrome-settings-overrides.js:78
(Diff revision 5)
> +      });
> +    },
> +  });
> +});
> +
> +async function handleInitialHomepagePopup(extensionId, homepageUrl) {

I don't understand the purpose of this function.  Is the issue that the event we're trying to observe has already fired by the time the extension startup code that registers a listener for it runs?

::: browser/components/extensions/ext-chrome-settings-overrides.js:182
(Diff revision 5)
>      await ExtensionSettingsStore.initialize();
> -    if (manifest.chrome_settings_overrides.homepage) {
> -      ExtensionPreferencesManager.setSetting(extension.id, "homepage_override",
> -                                             manifest.chrome_settings_overrides.homepage);
> +
> +    let homepageUrl = manifest.chrome_settings_overrides.homepage;
> +
> +    if (homepageUrl) {
> +      let inControl = await ExtensionPreferencesManager.setSetting(

I realize you didn't change this but why are we setting this preference every time the extension starts up?  Preferences are persistent, we should only need to do this on install.

::: browser/components/extensions/ext-chrome-settings-overrides.js:196
(Diff revision 5)
> +        }
> +      }
> +      extension.callOnClose({
> +        close: () => {
> +          if (extension.shutdownReason == "ADDON_DISABLE"
> +              || extension.shutdownReason == "ADDON_UNINSTALL") {

You should generally use `onUninstall()` for this, not `extension.callOnClose()`

::: browser/locales/en-US/chrome/browser/browser.dtd:985
(Diff revision 5)
>  
> +<!ENTITY homepageControlled.message "An extension has changed what you see as your home page. You can restore your settings if you do not want this change.">
> +<!ENTITY homepageControlled.header.message "Your home page has changed.">
> +<!ENTITY homepageControlled.keepButton.label "Keep Changes">
> +<!ENTITY homepageControlled.keepButton.accesskey "K">
> +<!ENTITY homepageControlled.restoreButton.label "Restore Settings">

This does not seem like a good description of what this button does, but enough cooks have already been in this particular kitchen...
Attachment #8943417 - Flags: review?(aswan) → review-
Comment on attachment 8943417 [details]
Bug 1397809 - Part 2: Add a doorhanger when an extension changes the homepage

https://reviewboard.mozilla.org/r/213732/#review225864

> I don't understand the purpose of this function.  Is the issue that the event we're trying to observe has already fired by the time the extension startup code that registers a listener for it runs?

Ah and now I see that you commented on this in the bug.  There should definitely be comments in the code here explaining this.  Also, the logic about about:blank logic doesn't seem right, can you check the page's document.readyState?
Comment on attachment 8943416 [details]
Bug 1397809 - Part 1: Convert New Tab doorhanger to a generic class

https://reviewboard.mozilla.org/r/213730/#review226062

Thanks! Haven't looked at the test in detail, but generally this looks fine. Some nitpicking follows.

::: browser/components/extensions/ExtensionControlledPopup.jsm:65
(Diff revision 4)
> +   *                   user decides to disable the extension. If this function is async
> +   *                   then the extension won't be disabled until it is fulfilled.
> +   */
> +  constructor(opts) {
> +    this.confirmedType = opts.confirmedType;
> +    this.makeWidgetId = opts.makeWidgetId;

I concur with making this not have to be an argument.

::: browser/components/extensions/ExtensionControlledPopup.jsm:66
(Diff revision 4)
> +   *                   then the extension won't be disabled until it is fulfilled.
> +   */
> +  constructor(opts) {
> +    this.confirmedType = opts.confirmedType;
> +    this.makeWidgetId = opts.makeWidgetId;
> +    this.observerEvent = opts.observerEvent;

Nit: `observerTopic` would be more consistent with what we use elsewhere.

::: browser/components/extensions/ExtensionControlledPopup.jsm:75
(Diff revision 4)
> +    this.observer = {
> +      observe: (subject, topic, data) => {

You can pass just a function to `Services.obs.addObserver/removeObserver`, because nsIObserver has the function keyword ( https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIObserver.idl ).

So in this case, could just do:

```js
this.observer = (subject, topic, data) => {
};
```

Or, for that matter, you could just add it as a method called `observe` on the class (adding a QueryInterface definition to the class) and then just pass `this` to the observer service? I'm not clear on why it needs to be set dynamically in the constructor.

::: browser/components/extensions/ext-url-overrides.js:58
(Diff revision 4)
> +      // ExtensionControlledPopup will disable the add-on once this function completes.
> +      // Disabling an add-on should remove the tabs that it has open, but we want
> +      // to open the new New Tab in this tab (which might get closed).

This is a pre-existing issue, but from what I can tell the popup may be open while the user switches tabs, or while the user loads something else in that tab. Replacing the contents of a tab is a destructive action. I guess it doesn't matter in most cases, but ideally this should check that the content we're replacing isn't something the user loaded independently / in a different tab.

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:28
(Diff revision 4)
> +  Object.keys(attributes).forEach(key => {
> +    popupnotification.setAttribute(key, attributes[key]);
> +  });

Nit: you can use `Object.entries(...).forEach((key, value) => {`.

Or you could just make the `attributes` thing an array of 2-element arrays ("pairs") and then you can just use a `for of` loop.

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:83
(Diff revision 4)
> +    onObserverRemoved() {
> +      observerAdded = false;
> +    },
> +    async beforeDisableAddon(doorhanger) {
> +      beforeDisableAddonCalled = true;
> +    },

It's not really worth changing here, but in future you may want to consider using `sinon` which is an in-tree library that does stubbing like this and lets you verify what methods get called with etc.
Attachment #8943416 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8943417 [details]
Bug 1397809 - Part 2: Add a doorhanger when an extension changes the homepage

https://reviewboard.mozilla.org/r/213732/#review226100

::: browser/base/content/browser.js:2185
(Diff revision 5)
>      break;
>    case "tabshifted":
>    case "tab":
>      urls = homePage.split("|");
>      var loadInBackground = getBoolPref("browser.tabs.loadBookmarksInBackground", false);
> +    notifyObservers = !loadInBackground;

Sorry, I'm a little confused. Why don't we put up the notification if the tab loads in the background? You stated this in comment 29 but without the reasoning, and I'm not sure what it is.

::: browser/components/extensions/ExtensionControlledPopup.jsm:162
(Diff revision 5)
> +      if (urlBarWasFocused) {
> -      win.gURLBar.focus();
> +        win.gURLBar.focus();
> +      }

Focus and panels is a giant pain in the ... Not your fault, but it means it'd take me more time than I have to work out exactly when this works and when it breaks. Without thinking too hard, I think this does the wrong thing if you follow e.g. these steps:

1. have url bar focused when this prompt is open
2. focus search bar while prompt is open (AIUI our new panels won't dismiss in this case?)
3. close panel with this command


If the panel *does* dismiss if you move focus, perhaps my concern doesn't really apply.

In any case, I don't know that this is correct. Why do we need to mess with focus at all? Put differently, can you describe what led you (or the original newtab notification implementer if that wasn't you, assuming that's where the focus call came from to begin with) to do this?

::: browser/components/extensions/ext-chrome-settings-overrides.js:79
(Diff revision 5)
> +  // browser.startup.page == 1 is show homepage.
> +  if (Services.prefs.getIntPref("browser.startup.page") == 1) {

This won't be right if the browser restores after a crash even if it normally shows the homepage. Edgecase, but we can do this right. :-)

I think you can use the same checks we use elsewhere (IIRC nsBrowserContentHandler.js or just browser.js) that, from memory, use a getter on session restore to figure out if we're restoring a session or not.

::: browser/components/extensions/ext-chrome-settings-overrides.js:88
(Diff revision 5)
> +        windowTracker.addListener("status", function listener() {
> +          windowTracker.removeListener("status", listener);
> +          resolve();

It's not clear to me what the 'status' event is and why this is always going to be the desired thing... What if a tab switch happens?
Attachment #8943417 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8943416 [details]
Bug 1397809 - Part 1: Convert New Tab doorhanger to a generic class

https://reviewboard.mozilla.org/r/213730/#review226062

> This is a pre-existing issue, but from what I can tell the popup may be open while the user switches tabs, or while the user loads something else in that tab. Replacing the contents of a tab is a destructive action. I guess it doesn't matter in most cases, but ideally this should check that the content we're replacing isn't something the user loaded independently / in a different tab.

The doorhanger will close if the user clicks on the page or changes tabs. So I think any content change would be done by the page that the doorhanger appeared on and we should still show it.

> It's not really worth changing here, but in future you may want to consider using `sinon` which is an in-tree library that does stubbing like this and lets you verify what methods get called with etc.

I had no idea sinon was available. I added it to this file and switched this to using `sinon.spy()` instead of setting booleans.
Comment on attachment 8943416 [details]
Bug 1397809 - Part 1: Convert New Tab doorhanger to a generic class

https://reviewboard.mozilla.org/r/213730/#review225854

> Is it the responsibility of users of this class to call this method when the related extension is uninstalled?  That should be documented very clearly if failing to do that means a storage leak (even if its a small leak)

I filed bug 1438364 to clean this up for all data when an extension is uninstalled. I'll make sure this is being cleaned up properly though for now.

> Should the call to `pnael.hidePopup()` below happen before this call?  It seems like the popup should go away immediately to acknowledge that the user has pressed a button and we are handling that, even if some further work is still going to happen before the extension actually gets disabled...

There was some test code relying on the ExtensionSettingsStore having been updated when the popup was hidden. I updated the tests to wait for that update rather than the popup being hidden.
Comment on attachment 8943417 [details]
Bug 1397809 - Part 2: Add a doorhanger when an extension changes the homepage

https://reviewboard.mozilla.org/r/213732/#review225864

> Ah and now I see that you commented on this in the bug.  There should definitely be comments in the code here explaining this.  Also, the logic about about:blank logic doesn't seem right, can you check the page's document.readyState?

`document.readyState` is "complete" and `currentUrl` is "about:blank" at this point.

> I realize you didn't change this but why are we setting this preference every time the extension starts up?  Preferences are persistent, we should only need to do this on install.

This would set the preferences to what we expect if it gets out of sync for some reason, but technically it should only need to happen on install. Would you like me to change it? I'm not sure how much it matters or if one way is better than the other.
Blocks: 1438396
Comment on attachment 8943417 [details]
Bug 1397809 - Part 2: Add a doorhanger when an extension changes the homepage

https://reviewboard.mozilla.org/r/213732/#review226100

> Sorry, I'm a little confused. Why don't we put up the notification if the tab loads in the background? You stated this in comment 29 but without the reasoning, and I'm not sure what it is.

The reasoning is basically it's an edge case I didn't want to deal with right now. It seems like it would have enough complexity on its own to justify its own bug. I filed bug 1438396 to handle that if we think we need to handle it.

> Focus and panels is a giant pain in the ... Not your fault, but it means it'd take me more time than I have to work out exactly when this works and when it breaks. Without thinking too hard, I think this does the wrong thing if you follow e.g. these steps:
> 
> 1. have url bar focused when this prompt is open
> 2. focus search bar while prompt is open (AIUI our new panels won't dismiss in this case?)
> 3. close panel with this command
> 
> 
> If the panel *does* dismiss if you move focus, perhaps my concern doesn't really apply.
> 
> In any case, I don't know that this is correct. Why do we need to mess with focus at all? Put differently, can you describe what led you (or the original newtab notification implementer if that wasn't you, assuming that's where the focus call came from to begin with) to do this?

These panels are dismissable and tab specific so if you manually focus the URL bar the popup will be dismissed.

The focus was originally being set for the new tab case because when the new tab opens the URL bar is focused by default. This popup steals the focus so if the user takes an action on the popup the focus will be restored to the URL bar.

There is also the case where the homepage and the new tab page are the same, in this case the URL bar is focused when the homepage is opened as well. But if the homepage is set to web content or a different extension page where the URL bar is filled in the page is focused.

The original focus state is checked so it can be restored when an action is taken on the popup.

I will add a comment.

> This won't be right if the browser restores after a crash even if it normally shows the homepage. Edgecase, but we can do this right. :-)
> 
> I think you can use the same checks we use elsewhere (IIRC nsBrowserContentHandler.js or just browser.js) that, from memory, use a getter on session restore to figure out if we're restoring a session or not.

I had looked through that code path and found `SessionStore.willOverrideHomepagePromise` [1]. I couldn't quite tell if it is safe to "call" this getter more than once and it doesn't guarantee that it won't override the homepage anyway [2].

I think this will still handle the case where the homepage is overridden since it checks that the URL is the same as the homepage URL.

[1] https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/browser/base/content/browser.js#1764
[2] https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/browser/components/sessionstore/nsSessionStartup.js#312-318

> It's not clear to me what the 'status' event is and why this is always going to be the desired thing... What if a tab switch happens?

Hmm, yeah, this isn't right. The status event will be for any tab which isn't what we want.

I had this using `waitForTabLoaded` before but I changed it for some reason. I think that's still what we want.

I'll switch it back to using that and also verify that we're still on the same tab.
Comment on attachment 8943417 [details]
Bug 1397809 - Part 2: Add a doorhanger when an extension changes the homepage

https://reviewboard.mozilla.org/r/213732/#review227010

It looks like the special handling of browser startup doesn't have a test?  But I guess it's not really practical to test from mochitest... :/

::: browser/components/extensions/ext-chrome-settings-overrides.js:67
(Diff revision 6)
> +        // Manually trigger an event in case this is controlled again.
> +        popup.open();

Why is this needed?  Is there a race between this code and the pref observer below that activates the observer?

::: browser/components/extensions/ext-chrome-settings-overrides.js:88
(Diff revision 6)
> +    let currentUrl = gBrowser.currentURI.spec;
> +    // When the first window is still loading the URL might be about:blank.
> +    // Wait for that the actual page to load before checking the URL, unless
> +    // the homepage is set to about:blank.
> +    if (currentUrl != homepageUrl && currentUrl == "about:blank") {
> +      await await waitForTabLoaded(tab);

One should be enough here :)

::: browser/components/extensions/test/browser/browser_ext_chrome_settings_overrides_home.js:31
(Diff revision 6)
>      HOMEPAGE_URL_PREF, Ci.nsIPrefLocalizedString).data;
>  };
>  
> +function isConfirmed(id) {
> +  let item = ExtensionSettingsStore.getSetting("homepageNotification", id);
> +  dump(`${id} ${JSON.stringify(item)}\n`);

This should be removed
Attachment #8943417 - Flags: review?(aswan) → review+
Comment on attachment 8943417 [details]
Bug 1397809 - Part 2: Add a doorhanger when an extension changes the homepage

https://reviewboard.mozilla.org/r/213732/#review227038

Thanks for all the explanations, r=me.

::: browser/components/extensions/ExtensionControlledPopup.jsm:79
(Diff revisions 5 - 6)
>      this.onObserverAdded = opts.onObserverAdded;
>      this.onObserverRemoved = opts.onObserverRemoved;
>      this.beforeDisableAddon = opts.beforeDisableAddon;
>      this.observerRegistered = false;
>  
> -    this.observer = {
> +    this.observe = this.observe.bind(this);

You're passing `this` to the observer service, and it'll take care of calling the `observe` method with the right `this`, so this line shouldn't be necessary.
Attachment #8943417 - Flags: review?(gijskruitbosch+bugs) → review+
Fixed up the comments. I also added some `await ExtensionSettingsStore.initialize()` calls to ExtensionControlledPopup since it was causing some test failures.
Can you please confirm the updated copy for this with the extension name, like in bug 1444149, Meridel.
Flags: needinfo?(mwalkington)
Is copy review still needed from me on this? I believe we finalized via Slack?
Flags: needinfo?(mwalkington) → needinfo?(mark)
Copy was confirmed over Slack.
Flags: needinfo?(mark)
Attachment #8943416 - Flags: review+ → review?(aswan)
Attachment #8943417 - Flags: review+ → review?(aswan)
Comment on attachment 8943416 [details]
Bug 1397809 - Part 1: Convert New Tab doorhanger to a generic class

https://reviewboard.mozilla.org/r/213730/#review239064

looks good to me with comments addressed

::: browser/components/extensions/ExtensionControlledPopup.jsm:77
(Diff revision 9)
> +    this.descriptionId = opts.descriptionId;
> +    this.descriptionMessageId = opts.descriptionMessageId;
> +    this.learnMoreMessageId = opts.learnMoreMessageId;
> +    this.learnMoreLink = opts.learnMoreLink;

please document these above

::: browser/components/extensions/ExtensionControlledPopup.jsm:112
(Diff revision 9)
> +  observe(subject, topic, data) {
> +    // We don't need to open the doorhanger again until the controlling add-on changes.
> +    this.removeObserver();
> +
> +    // Do this work in an idle callback to avoid interfering with new tab performance tracking.
> +    this.topWindow.requestIdleCallback(this.open.bind(this));

nit: we usually use `() => this.open();` instead of `.bind`

::: browser/components/extensions/ExtensionControlledPopup.jsm:128
(Diff revision 9)
> +  }
> +
> +  async addObserver(extensionId) {
> +    await ExtensionSettingsStore.initialize();
> +
> +    if (!this.observerRegistered && extensionId && !this.userHasConfirmed(extensionId)) {

when does this get called without an extensionId?

::: browser/components/extensions/ExtensionControlledPopup.jsm:141
(Diff revision 9)
> +
> +  async open() {
> +    await ExtensionSettingsStore.initialize();
> +
> +    // Ensure that an observer event won't cause this to open again.
> +    this.removeObserver();

This is also done in `observe()` do we need both?

::: browser/components/extensions/ExtensionControlledPopup.jsm:163
(Diff revision 9)
> +    if (!popupnotification) {
> +      throw new Error(`No popupnotification found for id "${this.popupnotificationId}"`);
> +    }

should this be up a few lines?  seems like if something has gone haywire and this could throw, something inside `populateDescription()` will probably throw before we ever get here.

::: browser/components/extensions/test/browser/browser_ext_ExtensionControlledPopup.js:72
(Diff revision 9)
> +  let {
> +    Management: {global: {windowTracker}},
> +  } = ChromeUtils.import("resource://gre/modules/Extension.jsm", {});

If you're just using this for topWindow, maybe easier to just use `Services.wm.getMostRecentWindow()` directly?
Attachment #8943416 - Flags: review?(aswan) → review+
Comment on attachment 8943417 [details]
Bug 1397809 - Part 2: Add a doorhanger when an extension changes the homepage

https://reviewboard.mozilla.org/r/213732/#review239066

::: browser/components/extensions/ext-browser.js:10
(Diff revision 10)
>  // This file provides some useful code for the |tabs| and |windows|
>  // modules. All of the code is installed on |global|, which is a scope
>  // shared among the different ext-*.js scripts.
>  
>  /* global EventEmitter:false, TabContext:false, WindowEventManager:false,
> -          makeWidgetId:false, tabTracker:true, windowTracker:true */
> +          waitForTabLoaded:false, replaceUrlInTab:false, makeWidgetId:false,

You'll probably notice this anyway when you rebase but this should be in .eslintrc instead

::: browser/components/extensions/ext-browser.json:175
(Diff revision 10)
>    },
>    "urlOverrides": {
>      "url": "chrome://browser/content/ext-url-overrides.js",
>      "schema": "chrome://browser/content/schemas/url_overrides.json",
>      "scopes": ["addon_parent"],
> +    "events": ["uninstall"],

this should actually be in the part 1 patch right?

::: browser/components/extensions/ext-chrome-settings-overrides.js:50
(Diff revision 10)
> +XPCOMUtils.defineLazyGetter(this, "strBundle", function() {
> +  return Services.strings.createBundle("chrome://global/locale/extensions.properties");
> +});

this doesn't appear to be used?
Attachment #8943417 - Flags: review?(aswan) → review+
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ff9b41cb0dd
Part 1: Convert New Tab doorhanger to a generic class r=aswan,Gijs
https://hg.mozilla.org/integration/autoland/rev/8f31c3153ea0
Part 2: Add a doorhanger when an extension changes the homepage r=aswan,Gijs
https://hg.mozilla.org/mozilla-central/rev/0ff9b41cb0dd
https://hg.mozilla.org/mozilla-central/rev/8f31c3153ea0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Verified as fixed in Latest Firefox Nightly on Windows 10x64.
I will attach postfix videos.
Status: RESOLVED → VERIFIED
Attached image Postfix video Homepage
Attached image Postfix video new tab
Product: Toolkit → WebExtensions
Depends on: 1466846
Regressions: 1570330
See Also: → 1570407
You need to log in before you can comment on or make changes to this bug.