1429593 - Show that a WebExtension is managing the proxy config setting

Status: VERIFIED FIXED

(WebExtensions :: Frontend, enhancement, P3)

Description

[Bob Silverberg [:bsilverberg]]

A new API is being introduced for browserSettings.proxyConfig, which will allow an extension to control the prefs that are exposed via about:preferences for proxy settings. This is the panel displayed when one clicks the "Settings..." button in the "Network Proxy" section at the bottom of the main about:preferences page.

When an extension is controlling these prefs the user should be shown the extension name with an option to disable the extension, similar to what is being done for Tracking Protection and Cookies.

The user should be able to see the values set by the extension, so it makes sense for the message to appear on the panel in question, probably at the top, and if an extension is in control all the controls on the panel should be disabled.

I'm not sure if we need a mockup for this. I can just implement a version and then attach a screenshot of it to the bug.

Comment 1

[Bob Silverberg [:bsilverberg]]

Markus, I'm not sure if we need a mockup for this or not. If you'd like to create one that would be great, or I can just take an initial stab at it and attach a screenshot.

Comment 2

[Markus Jaritz [:designakt] (UX)]

If we implement this message in the panel, the user will not be aware of the fact that an extension controls this, until they click the "settings..." button. 
So either we 
1) show an additional hint next to the button, (which I would need to invent a new element for)
or we 
2) show the full message right in settings, instead of in the panel, and disable the settings button, until the user want's to take back control. (this would more closer align with what we do with other settings)

Bob, I think option 2 is a simpler first implementation we should go for. And we can refine, if we have evidence that we need to provide more info to users by showing the panel even when an extension controls the proxy.

Comment 3

[Bob Silverberg [:bsilverberg]]

Attachment: Main page with extension controlled message

This screenshot shows the main about:preferences page with an extension controlling proxy settings. All I've done is displayed the standard extension controlled message, without the disable button.

Comment 4

[Bob Silverberg [:bsilverberg]]

Attachment: Connections panel with extension controlled message

This is a screenshot of the panel that appears when a user clicks "Settings..." in the "Network Proxy" section of about:preferences. The standard extension controlled message appears at the top of the panel, along with a button to disable the extension. All of the controls in the panel are disabled. I left the OK and Cancel buttons enabled, but maybe the OK button should be disabled too.

Markus, please take a look at this and the other screenshot I attached and let me know what changes you'd like to see.

Comment 5

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1429593 - Part 1: Extract functions for dealing with extensions into a separate file

This moves all the functions that manage/change the UI because of extensions being in
control of certain preferences to a separate file, so it can be included in SubDialogs.

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

Comment 6

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

Update the general page of about:preferences, as well as the Connection Settings panel, to show
when an extension is controlling proxy settings, and allow a user to disable the extension to
regain control.

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

Comment 7

[Mark Striemer [:mstriemer]]

Comment on attachment 8944832 [details]
Bug 1429593 - Part 1: Extract functions for dealing with extensions into a separate file

https://reviewboard.mozilla.org/r/214968/#review220620

Comment 8

[Bob Silverberg [:bsilverberg]]

Attachment: Connections panel with extension controlled message

This is a new screenshot of the panel, which shows the latest version, which fixes the problems with the socks version radio buttons.

Comment 9

[Mark Striemer [:mstriemer]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

https://reviewboard.mozilla.org/r/214970/#review220622

::: browser/components/preferences/connection.js:36
(Diff revision 1)
>    { id: "network.proxy.backup.socks", type: "string" },
>    { id: "network.proxy.backup.socks_port", type: "int" },
>  ]);
>  
> +function setEventListener(aId, aEventType, aCallback) {
> +  document.getElementById(aId)

It looks like you could inline this. I don't see `setEventListener` used elsewhere and it doesn't seem to need `gConnectionsDialog`.

```
document
  .getElementById("disableProxyExtension")
  .addEventListener(
    "command", makeDisableControllingExtension(PREF_SETTING_TYPE, PROXY_KEY));
```

::: browser/components/preferences/connection.js:253
(Diff revision 1)
> +      pref => Services.prefs.prefIsLocked(pref));
> +
> +    function setInputsDisabledState(isControlled) {
> +      let disabled = isLocked || isControlled;
> +      let controlGroup = document.getElementById("networkProxyType");
> +      for (let element of controlGroup.querySelectorAll(":scope > radio")) {

This would likely read better with a helper function that returns all of the elements that you need to disable/enable.

```
function getProxyInputs() {
  let controlGroup = document.getElementById("networkProxyType");
  return [
    controlGroup,
    ...controlGroup.querySelectorAll("label, textbox, checkbox"),
    ...document.querySelectorAll(
      "#networkProxySOCKSVersion > radio, #ConnectionsDialogPane > checkbox"),
  ];
}

getProxyInputs().forEach(element => {
  element.disabled = disabled;
});
```

::: browser/components/preferences/connection.js:278
(Diff revision 1)
> +    if (isLocked) {
> +      // An extension can't control this setting if either pref is locked.
> +      hideControllingExtension(PROXY_KEY);
> +      setInputsDisabledState(false);
> +    } else {
> +      handleControllingExtension(

nit: inline the first call

```
handleControllingExtension(PREF_SETTING_TYPE, PROXY_KEY)
  .then(setInputsDisabledState);
```

::: browser/components/preferences/in-content/extensionControlled.js:65
(Diff revision 1)
> +function debounce(func, wait) {
> +  let timer = null;
> +
> +  return function() {
> +    if (timer) {
> +      clearTimeout(timer);

I would have expected this to return here. I suppose it doesn't matter either way, but if you keep getting calls to this function in intervals less than `wait` then it will never execute.

Either way likely works fine, but thought I'd mention it.

::: browser/components/preferences/in-content/extensionControlled.js:68
(Diff revision 1)
> +  return function() {
> +    if (timer) {
> +      clearTimeout(timer);
> +    }
> +
> +    let args = arguments;

nit: You should be able to make this `function(...args)` instead of pulling them here.

::: browser/components/preferences/in-content/extensionControlled.js:72
(Diff revision 1)
> +
> +    let args = arguments;
> +    let context = this;
> +    timer = setTimeout(() => {
> +      timer = null;
> +      func.apply(context, args);

nit: You should be able to use `func.apply(this, args)` here since this is in an arrow function.

It also looks like the case where this is used `this` isn't needed. It may make sense to keep this in case it needs `this` later but I think it could also be removed.

::: browser/components/preferences/in-content/extensionControlled.js:206
(Diff revision 1)
> +      if (API_PROXY_PREFS.includes(data)) {
> +        container.updateProxySettingsUI();
> +      }
> +    },
> +  };
> +  proxyObserver.observe = debounce(proxyObserver.observe, 1);

I think this would be clearer if it was inline in `proxyObserver`.

```
let observe = debounce(function (subject, topic, data) {
  if (API_PROXY_PREFS.includes(data)) {
    container.updateProxySettingsUI();
  }
}, 1);
let proxyObserver = {observe};
// or
let proxyObserver = {
  observe: debounce(function (subject, topic, data) {
    if (API_PROXY_PREFS.includes(data)) {
      container.updateProxySettingsUI();
    }
  }, 1),
};
```

::: browser/components/preferences/in-content/tests/browser_extension_controlled.js:648
(Diff revision 1)
> +    }
> +
> +    if (isPanel) {
> +      let controlGroup = doc.getElementById("networkProxyType");
> +      let controlState = isControlled ? "disabled" : "enabled";
> +      for (let element of controlGroup.querySelectorAll(":scope > radio")) {

I guess these should be updated to match what is in connection.js. It's really too bad there doesn't appear to be a better way to find these elements.

Comment 10

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944832 [details]
Bug 1429593 - Part 1: Extract functions for dealing with extensions into a separate file

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

Comment 11

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

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

Comment 12

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

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

Comment 13

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944832 [details]
Bug 1429593 - Part 1: Extract functions for dealing with extensions into a separate file

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

Comment 14

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

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

Comment 15

[Markus Jaritz [:designakt] (UX)]

(In reply to Bob Silverberg [:bsilverberg] from comment #4)
> Created attachment 8944716 [details]
> Connections panel with extension controlled message
> 
> This is a screenshot of the panel that appears when a user clicks
> "Settings..." in the "Network Proxy" section of about:preferences. The
> standard extension controlled message appears at the top of the panel, along
> with a button to disable the extension. All of the controls in the panel are
> disabled. I left the OK and Cancel buttons enabled, but maybe the OK button
> should be disabled too.
> 
> Markus, please take a look at this and the other screenshot I attached and
> let me know what changes you'd like to see.

Looks good to me. Please also disable the headline in the overlay. Thanks.

Comment 16

[Bob Silverberg [:bsilverberg]]

(In reply to Markus Jaritz [:designakt] (UX) from comment #15)
> (In reply to Bob Silverberg [:bsilverberg] from comment #4)
> > Created attachment 8944716 [details]
> > Connections panel with extension controlled message
> > 
> > This is a screenshot of the panel that appears when a user clicks
> > "Settings..." in the "Network Proxy" section of about:preferences. The
> > standard extension controlled message appears at the top of the panel, along
> > with a button to disable the extension. All of the controls in the panel are
> > disabled. I left the OK and Cancel buttons enabled, but maybe the OK button
> > should be disabled too.
> > 
> > Markus, please take a look at this and the other screenshot I attached and
> > let me know what changes you'd like to see.
> 
> Looks good to me. Please also disable the headline in the overlay. Thanks.

Thanks Markus. This patch also includes a new translation string. The message displayed when an extension is controlling proxy settings is:

"An extension, %S, is controlling your proxy settings.", where %S is the name of the extension. Who needs to sign off on this string?

Comment 17

[Markus Jaritz [:designakt] (UX)]

Meridel, can you please review the string for this override.

> "An extension, <icon> <insert name of extension>, is controlling your proxy settings."

For context, I added a list of other messages we have:

> "An extension, <icon> <insert name of extension>, is controlling tracking protection."
> "An extension, <icon> <insert name of extension>, controls your New Tab page."
> "An extension, <icon> <insert name of extension>, controls your Home page."

> "An extension, <icon> <insert name of extension>, controls how cookies are handled." (WIP)

Bob, can you please verify that list of messages, and update/extend if necessary.

Comment 18

[Bob Silverberg [:bsilverberg]]

(In reply to Markus Jaritz [:designakt] (UX) from comment #17)
> Meridel, can you please review the string for this override.
> 
> > "An extension, <icon> <insert name of extension>, is controlling your proxy settings."
> 
> For context, I added a list of other messages we have:
> 
> > "An extension, <icon> <insert name of extension>, is controlling tracking protection."
> > "An extension, <icon> <insert name of extension>, controls your New Tab page."
> > "An extension, <icon> <insert name of extension>, controls your Home page."
> 
> > "An extension, <icon> <insert name of extension>, controls how cookies are handled." (WIP)
> 
> Bob, can you please verify that list of messages, and update/extend if
> necessary.

Thanks Markus. It looks like the only existing one you missed is:

"An extension, <icon> <insert name of extension>, has set your default search engine."

Comment 19

[Markus Jaritz [:designakt] (UX)]

Here is another string that I forgot about:

> "An extension, <icon> <insert name of extension>, requires Container Tabs."

Comment 20

[Meridel [:meridel]]

Draft in progress here -- https://docs.google.com/document/d/1ia8KW95jSB8SAAAN8vYtz8INDsbuNWs0c2WZvbxH0NM/edit?usp=sharing

I will review and discuss with Michelle on Monday as she worked on the earlier strings.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8944832 [details]
Bug 1429593 - Part 1: Extract functions for dealing with extensions into a separate file

https://reviewboard.mozilla.org/r/214968/#review222098

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

https://reviewboard.mozilla.org/r/214970/#review222102

::: browser/components/preferences/connection.js:244
(Diff revision 4)
> +    let isLocked = API_PROXY_PREFS.some(
> +      pref => Services.prefs.prefIsLocked(pref));

Should we propose adding a new prefIsControlled API? Then we could have a function that checks for either prefIsLocked or prefIsControlled.

::: browser/components/preferences/in-content/extensionControlled.js:208
(Diff revision 4)
> +  let proxyObserver = {
> +    observe: debounce((subject, topic, data) => {
> +      if (API_PROXY_PREFS.includes(data)) {
> +        container.updateProxySettingsUI();
> +      }
> +    }, 1),

A debounce rate of 1ms is too low. Can you update the debounce function to use idle callbacks instead of setTimeout?

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:303
(Diff revision 4)
>  # This string is shown to notify the user that their tracking protection preferences are being controlled by an extension.
>  extensionControlled.websites.trackingProtectionMode = An extension, %S, is controlling tracking protection.
>  
> +# LOCALIZATION NOTE (extensionControlled.proxyConfig):
> +# This string is shown to notify the user that their proxy configuration preferences are being controlled by an extension.
> +extensionControlled.proxyConfig = An extension, %S, is controlling your proxy settings.

I can't see where this string is used.

Comment 23

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

https://reviewboard.mozilla.org/r/214970/#review220622

> It looks like you could inline this. I don't see `setEventListener` used elsewhere and it doesn't seem to need `gConnectionsDialog`.
> 
> ```
> document
>   .getElementById("disableProxyExtension")
>   .addEventListener(
>     "command", makeDisableControllingExtension(PREF_SETTING_TYPE, PROXY_KEY));
> ```

Thanks. It does need `gConnectionsDialog` but I can just bind it inline.

Comment 24

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

https://reviewboard.mozilla.org/r/214970/#review222102

> Should we propose adding a new prefIsControlled API? Then we could have a function that checks for either prefIsLocked or prefIsControlled.

I'm not sure. I see a couple of issues with this:
1. Determining whether a pref is controlled would require interacting with the ExtensionSettingsStore, and I'm not sure we want to add code to the prefs service that does that (or maybe you aren't suggesting adding it to the prefs service, but rather to the extension settings store?). It's also not a straightforward operation, as the data in the settings store doesn't tell us directly which prefs are being controlled, it only tells us which settings are being controlled, so we can't simply ask "Is this pref controlled?". Well, we could, but it would require a bit more code to execute.
2. I don't think we only need to know the combination of prefIsLocked or prefIsControlled - we also need to know whether a pref is controlled or not, independent of whether it's locked, so we can update the UI. So I'm not sure what this would buy us. Could you be a bit more explicit about exactly what code you think we could replace if we have a prefIsControlled API?

> A debounce rate of 1ms is too low. Can you update the debounce function to use idle callbacks instead of setTimeout?

The reason I'm using 1ms in this case is because all of the prefs that are changed when a setting changes happen synchonously, so I assume they'd happen in the same tick and therefore 1ms would be sufficient. I'm not trying to protect against firing multiple times for pref changes if they happen as a result of multiple settings being changed, I only want to make sure that if a setting is changed, which results in multiple prefs being changed (which would happen synchonously) that the observer is only fired once for that complete set of changes. Does that make sense, or am I not understanding the issue?

> I can't see where this string is used.

It's in extensionControlled.js lines 139-140:

```
let msg = document.getElementById("bundlePreferences")
                    .getString(`extensionControlled.${settingName}`);
```

Comment 25

[Bob Silverberg [:bsilverberg]]

Thanks for the review, Jared. I have responded to your issues with some questions/responses of my own.

Comment 26

[Meridel [:meridel]]

Michelle approved the text (https://docs.google.com/document/d/1ia8KW95jSB8SAAAN8vYtz8INDsbuNWs0c2WZvbxH0NM/edit?usp=sharing) but is confirming with legal today what form of the verb (gerund of 'controlling' or not) we should use for all of these extension strings. 

In the meantime, please let me know if what I have proposed in the document will work -- the user will see one statement if he/she if the proxy is installed and a different statement if the proxy is not installed.

Comment 27

[Bob Silverberg [:bsilverberg]]

Looks like the wrong needinfo was removed.

Comment 28

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

https://reviewboard.mozilla.org/r/214970/#review222102

> I'm not sure. I see a couple of issues with this:
> 1. Determining whether a pref is controlled would require interacting with the ExtensionSettingsStore, and I'm not sure we want to add code to the prefs service that does that (or maybe you aren't suggesting adding it to the prefs service, but rather to the extension settings store?). It's also not a straightforward operation, as the data in the settings store doesn't tell us directly which prefs are being controlled, it only tells us which settings are being controlled, so we can't simply ask "Is this pref controlled?". Well, we could, but it would require a bit more code to execute.
> 2. I don't think we only need to know the combination of prefIsLocked or prefIsControlled - we also need to know whether a pref is controlled or not, independent of whether it's locked, so we can update the UI. So I'm not sure what this would buy us. Could you be a bit more explicit about exactly what code you think we could replace if we have a prefIsControlled API?

Much of this is already solved by the new extensionControlled.js file, so it's not as big of a consolidation as it otherwise would be.

> The reason I'm using 1ms in this case is because all of the prefs that are changed when a setting changes happen synchonously, so I assume they'd happen in the same tick and therefore 1ms would be sufficient. I'm not trying to protect against firing multiple times for pref changes if they happen as a result of multiple settings being changed, I only want to make sure that if a setting is changed, which results in multiple prefs being changed (which would happen synchonously) that the observer is only fired once for that complete set of changes. Does that make sense, or am I not understanding the issue?

You could use `await Promise.resolve` like is done at https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/toolkit/components/payments/res/PaymentsStore.js#36-70 to wait for all synchronous calls to bunch up first.

> It's in extensionControlled.js lines 139-140:
> 
> ```
> let msg = document.getElementById("bundlePreferences")
>                     .getString(`extensionControlled.${settingName}`);
> ```

Thanks

Comment 29

[Meridel [:meridel]]

Hi Michelle, did Mika have a response as to whether we should be using the gerund or not for our extension strings?


"An extension, <icon> <insert name of extension>, is controlling tracking protection."
"An extension, <icon> <insert name of extension>, controls your New Tab page."
"An extension, <icon> <insert name of extension>, controls your Home page."
"An extension, <icon> <insert name of extension>, controls how cookies are handled." (WIP)
"An extension, <icon> <insert name of extension>, has set your default search engine."
"An extension, <icon> <insert name of extension>, requires Container Tabs."
"An extension, <icon> <insert name of extension>, controls how Nightly connects to the internet."

Comment 30

[Bob Silverberg [:bsilverberg]]

Attachment: Main page without extension control

This is the new version of the main page when an extension is not in control.

Comment 31

[Bob Silverberg [:bsilverberg]]

Attachment: Main page with extension controlled message

This is the new version of the main page when an extension is in control.

Comment 32

[Bob Silverberg [:bsilverberg]]

Attachment: Connections panel with extension controlled message

This is the new version of the panel with an extension in control.

Markus, please take a look at this and the other two new screenshots, which conform to Meridel's requests in the Google doc, and let me know if they are okay.

Comment 33

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944832 [details]
Bug 1429593 - Part 1: Extract functions for dealing with extensions into a separate file

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

Comment 34

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

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

Comment 35

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

Re-requesting review as the changes requested by UX resulted in significant changes to the code. Sorry, and thanks.

Comment 36

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

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

Comment 37

[Meridel [:meridel]]

Comment on attachment 8947589 [details]
Connections panel with extension controlled message

Thanks, Bob. This looks good. Reviewed with Markus and here are our two edits, please:

1. Possible to move the "Learn more" link up on to the same line as the strings, rather than placing the link on a separate line beneath? The strings would look like this:

Configure how Nightly connects to the internet. Learn more          

An extension, <icon> <insert name of extension>, controls how Nightly connects to the internet. Learn more


2. On the "Connection Settings" screen, can we top-align the Disable Extension button with the string?

Comment 38

[Mark Striemer [:mstriemer]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

https://reviewboard.mozilla.org/r/214970/#review223362

Still looks good. A few minor nits.

::: browser/components/preferences/connection.js:247
(Diff revision 6)
> +  // proxy settings.
> +  async updateProxySettingsUI() {
> +    let isLocked = API_PROXY_PREFS.some(
> +      pref => Services.prefs.prefIsLocked(pref));
> +
> +    function getProxyControls() {

nit: You could move this function definition out of this function. I think it would make this read a bit better

::: browser/components/preferences/connection.js:271
(Diff revision 6)
> +    if (isLocked) {
> +      // An extension can't control this setting if any pref is locked.
> +      hideControllingExtension(PROXY_KEY);
> +      setInputsDisabledState(false);
> +    } else {
> +      handleControllingExtension(

nit: put the first function call on one line

::: browser/components/preferences/in-content/extensionControlled.js:136
(Diff revision 6)
> +  image.classList.add("extension-controlled-icon");
> +  let addonBit = document.createDocumentFragment();
> +  addonBit.appendChild(image);
> +  addonBit.appendChild(document.createTextNode(" " + addon.name));
> +  let fragmentArgs = [document, msg, addonBit, ...extraArgs];
> +  return BrowserUtils.getLocalizedFragment(...fragmentArgs);

nit: fragmentArgs isn't needed

```
return BrowserUtils.getLocalizedFragment(document, msg, addonBit, ...extraArgs);
```

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

https://reviewboard.mozilla.org/r/214970/#review223426

::: browser/components/preferences/in-content/extensionControlled.js:207
(Diff revision 6)
> +function initializeProxyUI(container) {
> +  let deferredUpdate = new DeferredTask(() => {
> +    container.updateProxySettingsUI();
> +  }, 1);

Just a reminder that DeferredTask does not debounce by default when you call arm(); like this - check out this documentation:

https://searchfox.org/mozilla-central/rev/eeb7190f9ad6f1a846cd6df09986325b3f2c3117/toolkit/modules/DeferredTask.jsm#30-32

Are you attempting to debounce here? If so, what I'd suggest you do instead is to do:

```js
deferredUpdate.disarm();
deferredUpdate.arm();
```

Inside your observer, and perhaps stretch out your delay a bit. Maybe 100ms?

Comment 40

[Bob Silverberg [:bsilverberg]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #39)
> Comment on attachment 8944833 [details]
> Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config
> setting
> 
> https://reviewboard.mozilla.org/r/214970/#review223426
> 
> ::: browser/components/preferences/in-content/extensionControlled.js:207
> (Diff revision 6)
> > +function initializeProxyUI(container) {
> > +  let deferredUpdate = new DeferredTask(() => {
> > +    container.updateProxySettingsUI();
> > +  }, 1);
> 
> Just a reminder that DeferredTask does not debounce by default when you call
> arm(); like this - check out this documentation:
> 
> https://searchfox.org/mozilla-central/rev/
> eeb7190f9ad6f1a846cd6df09986325b3f2c3117/toolkit/modules/DeferredTask.jsm#30-
> 32
> 
> Are you attempting to debounce here? If so, what I'd suggest you do instead
> is to do:
> 
> ```js
> deferredUpdate.disarm();
> deferredUpdate.arm();
> ```
> 
> Inside your observer, and perhaps stretch out your delay a bit. Maybe 100ms?

Thanks for the feedback, Mike. Perhaps I'm not trying to debounce in the traditional meaning of the term. I'm just trying to protect against the updateProxySettingsUI() being called multiple times when one batch of preferences is updated. This "batch update" is done synchonously, and I only want to fire the function once per batch, but if a second batch of changes was to occur within a very short period of time, then it would be fine for the function to fire a second time. This is why it seemed to me that simply using `arm` would work, and also why 1ms would be sufficient. But perhaps I'm misunderstanding. Given my use case, do you still think the changes you've requested above are required?

Comment 41

[Bob Silverberg [:bsilverberg]]

Attachment: Main page without extension control

Comment 42

[Bob Silverberg [:bsilverberg]]

Attachment: Main page with extension controlled message

Comment 43

[Bob Silverberg [:bsilverberg]]

Attachment: Connections panel with extension controlled message

Comment 44

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944832 [details]
Bug 1429593 - Part 1: Extract functions for dealing with extensions into a separate file

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

Comment 45

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

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

Comment 46

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

https://reviewboard.mozilla.org/r/214970/#review223362

Thanks Mark. Nits addressed.

Comment 47

[Meridel [:meridel]]

Comment on attachment 8948381 [details]
Main page with extension controlled message

Thank you! This looks good except that the button needs more space around it -- would this be possible? "Learn more" is running into "Settings..."

Comment 48

[Meridel [:meridel]]

Comment on attachment 8948380 [details]
Main page without extension control

If the string becomes longer once translated, I worry "Learn more" will run into the button. Possible to build space around the button?

Comment 49

[Meridel [:meridel]]

Comment on attachment 8948382 [details]
Connections panel with extension controlled message

This looks good, thank you. Do we have more space built around the button here? It looks like it.

Comment 50

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

https://reviewboard.mozilla.org/r/214970/#review223692

::: browser/components/preferences/in-content/extensionControlled.js:209
(Diff revision 7)
>  }
> +
> +function initializeProxyUI(container) {
> +  let deferredUpdate = new DeferredTask(() => {
> +    container.updateProxySettingsUI();
> +  }, 1);

r=me if you change this to 10ms. There should be no visible difference to the human eye and it will allow an order of magntitude more coalescing.

Comment 51

[Bob Silverberg [:bsilverberg]]

Attachment: Main page with extension controlled message

I added a separator between the Learn more link and the button, which should deal with this on the main page, both when controlled and when uncontrolled. Attached is a new screenshot.

Comment 52

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944832 [details]
Bug 1429593 - Part 1: Extract functions for dealing with extensions into a separate file

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

Comment 53

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

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

Comment 54

[Bob Silverberg [:bsilverberg]]

(In reply to Meridel from comment #29)
> Hi Michelle, did Mika have a response as to whether we should be using the
> gerund or not for our extension strings?
> 
> 
> "An extension, <icon> <insert name of extension>, is controlling tracking
> protection."
> "An extension, <icon> <insert name of extension>, controls your New Tab
> page."
> "An extension, <icon> <insert name of extension>, controls your Home page."
> "An extension, <icon> <insert name of extension>, controls how cookies are
> handled." (WIP)
> "An extension, <icon> <insert name of extension>, has set your default
> search engine."
> "An extension, <icon> <insert name of extension>, requires Container Tabs."
> "An extension, <icon> <insert name of extension>, controls how Nightly
> connects to the internet."

Meridel, I think this question is the only thing blocking this from landing now. Do you think we can get an answer, or, failing that, could we land this as is and submit a follow-up bug to change whatever strings end up needing to be changed?

Comment 55

[Mike Conley (:mconley) (:⚙️)]

(In reply to Bob Silverberg [:bsilverberg] from comment #40)
> I'm just trying to protect against the
> updateProxySettingsUI() being called multiple times when one batch of
> preferences is updated. This "batch update" is done synchonously, and I only
> want to fire the function once per batch, but if a second batch of changes
> was to occur within a very short period of time, then it would be fine for
> the function to fire a second time. 

Oh, I see. So this is less about the prefs changing rapidly over several ticks of the event loop, and more about the prefs changing rapidly over the _same_ tick of the event loop. I see.

In that case, yes, I think I understand what's happening here. I don't see the harm in taking jaws' suggestion of 10ms to cover the event that these prefs _are_ changed several times over several ticks, though it sounds like that'd just be a bonus.

(Also, sorry for wading in here uninvited - I just had been looking at DeferredTask recently for an unrelated bug)

Comment 56

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

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

Comment 57

[Meridel [:meridel]]

(In reply to Bob Silverberg [:bsilverberg] from comment #54)
> (In reply to Meridel from comment #29)
> > Hi Michelle, did Mika have a response as to whether we should be using the
> > gerund or not for our extension strings?
> > 
> > 
> > "An extension, <icon> <insert name of extension>, is controlling tracking
> > protection."
> > "An extension, <icon> <insert name of extension>, controls your New Tab
> > page."
> > "An extension, <icon> <insert name of extension>, controls your Home page."
> > "An extension, <icon> <insert name of extension>, controls how cookies are
> > handled." (WIP)
> > "An extension, <icon> <insert name of extension>, has set your default
> > search engine."
> > "An extension, <icon> <insert name of extension>, requires Container Tabs."
> > "An extension, <icon> <insert name of extension>, controls how Nightly
> > connects to the internet."
> 
> Meridel, I think this question is the only thing blocking this from landing
> now. Do you think we can get an answer, or, failing that, could we land this
> as is and submit a follow-up bug to change whatever strings end up needing
> to be changed?

I confirmed with Michelle that we can use the gerund. I'd like the strings to all be consistent. Can we edit each of the strings to the following? Do I need to file a separate bug on this?

"An extension, <icon> <insert name of extension>, is controlling your New Tab page."
"An extension, <icon> <insert name of extension>, is controlling your Home page."
"An extension, <icon> <insert name of extension>, is controlling how cookies are handled." (WIP)
"An extension, <icon> <insert name of extension>, is controlling how Nightly connects to the internet."

Comment 58

[Meridel [:meridel]]

(In reply to Bob Silverberg [:bsilverberg] from comment #51)
> Created attachment 8948485 [details]
> Main page with extension controlled message
> 
> I added a separator between the Learn more link and the button, which should
> deal with this on the main page, both when controlled and when uncontrolled.
> Attached is a new screenshot.

I think this looks good. Markus - do you agree?

Comment 59

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

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

Comment 60

[Markus Jaritz [:designakt] (UX)]

(In reply to Meridel from comment #58)
> (In reply to Bob Silverberg [:bsilverberg] from comment #51)
> > Created attachment 8948485 [details]
> > Main page with extension controlled message
> > 
> > I added a separator between the Learn more link and the button, which should
> > deal with this on the main page, both when controlled and when uncontrolled.
> > Attached is a new screenshot.
> 
> I think this looks good. Markus - do you agree?

Looks great. Thanks.

Also clearing the needinfo for Michelle, as Meridel answered for her.

Comment 61

[Bob Silverberg [:bsilverberg]]

(In reply to Meridel from comment #57)
> (In reply to Bob Silverberg [:bsilverberg] from comment #54)
> > (In reply to Meridel from comment #29)
> > > Hi Michelle, did Mika have a response as to whether we should be using the
> > > gerund or not for our extension strings?
> > > 
> > > 
> > > "An extension, <icon> <insert name of extension>, is controlling tracking
> > > protection."
> > > "An extension, <icon> <insert name of extension>, controls your New Tab
> > > page."
> > > "An extension, <icon> <insert name of extension>, controls your Home page."
> > > "An extension, <icon> <insert name of extension>, controls how cookies are
> > > handled." (WIP)
> > > "An extension, <icon> <insert name of extension>, has set your default
> > > search engine."
> > > "An extension, <icon> <insert name of extension>, requires Container Tabs."
> > > "An extension, <icon> <insert name of extension>, controls how Nightly
> > > connects to the internet."
> > 
> > Meridel, I think this question is the only thing blocking this from landing
> > now. Do you think we can get an answer, or, failing that, could we land this
> > as is and submit a follow-up bug to change whatever strings end up needing
> > to be changed?
> 
> I confirmed with Michelle that we can use the gerund. I'd like the strings
> to all be consistent. Can we edit each of the strings to the following? Do I
> need to file a separate bug on this?
> 
> "An extension, <icon> <insert name of extension>, is controlling your New
> Tab page."
> "An extension, <icon> <insert name of extension>, is controlling your Home
> page."
> "An extension, <icon> <insert name of extension>, is controlling how cookies
> are handled." (WIP)
> "An extension, <icon> <insert name of extension>, is controlling how Nightly
> connects to the internet."

Thanks Meridel. I have updated all of these (well, not the cookies one as it's not landed yet) in the patch attached to this bug.

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20829d727fdf
Part 1: Extract functions for dealing with extensions into a separate file, r=jaws,mstriemer
https://hg.mozilla.org/integration/autoland/rev/9dfaa0724a42
Part 2: Show that a WebExtension is managing the proxy config setting, r=jaws,mstriemer

Comment 63

[Meridel [:meridel]]

(In reply to Bob Silverberg [:bsilverberg] from comment #61)
> (In reply to Meridel from comment #57)
> > (In reply to Bob Silverberg [:bsilverberg] from comment #54)
> > > (In reply to Meridel from comment #29)
> > > > Hi Michelle, did Mika have a response as to whether we should be using the
> > > > gerund or not for our extension strings?
> > > > 
> > > > 
> > > > "An extension, <icon> <insert name of extension>, is controlling tracking
> > > > protection."
> > > > "An extension, <icon> <insert name of extension>, controls your New Tab
> > > > page."
> > > > "An extension, <icon> <insert name of extension>, controls your Home page."
> > > > "An extension, <icon> <insert name of extension>, controls how cookies are
> > > > handled." (WIP)
> > > > "An extension, <icon> <insert name of extension>, has set your default
> > > > search engine."
> > > > "An extension, <icon> <insert name of extension>, requires Container Tabs."
> > > > "An extension, <icon> <insert name of extension>, controls how Nightly
> > > > connects to the internet."
> > > 
> > > Meridel, I think this question is the only thing blocking this from landing
> > > now. Do you think we can get an answer, or, failing that, could we land this
> > > as is and submit a follow-up bug to change whatever strings end up needing
> > > to be changed?
> > 
> > I confirmed with Michelle that we can use the gerund. I'd like the strings
> > to all be consistent. Can we edit each of the strings to the following? Do I
> > need to file a separate bug on this?
> > 
> > "An extension, <icon> <insert name of extension>, is controlling your New
> > Tab page."
> > "An extension, <icon> <insert name of extension>, is controlling your Home
> > page."
> > "An extension, <icon> <insert name of extension>, is controlling how cookies
> > are handled." (WIP)
> > "An extension, <icon> <insert name of extension>, is controlling how Nightly
> > connects to the internet."
> 
> Thanks Meridel. I have updated all of these (well, not the cookies one as
> it's not landed yet) in the patch attached to this bug.

Great, thank you!

Comment 64

[Francesco Lodolo [:flod]]

I've asked for a back-out, since one of the patches changes strings without a new ID.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

The change is very subtle ("controls" -> "is controlling"), to the point that it might have been OK for an existing standalone string, but it's not the case for these alerts. 

We have two strings using "controls" and already translated across languages: the new one is going to be translated as "is controlling", while the change to the other 2 strings will remain unnoticed, leading to inconsistent messages in localized builds.

Comment 65

[Raul Gurzau (:RaulG)]

Backed out 2 changesets (bug 1429593) for l10n issues

Backout link: https://hg.mozilla.org/integration/autoland/rev/87b9333134d1ed8833dda6514d729de3930dc54b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9dfaa0724a424dfa146159d388f8fbd0a70bce28

Comment 66

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944832 [details]
Bug 1429593 - Part 1: Extract functions for dealing with extensions into a separate file

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

Comment 67

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

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

Comment 68

[Bob Silverberg [:bsilverberg]]

I reverted the changes to the existing strings and will try to reland. Meridel, I will open a follow-up bug to make the changes to the two existing strings.

Comment 69

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s fc9554bf80125f99bc963b18788d0c4263e62a39 -d 8932202ed0b9: rebasing 446172:fc9554bf8012 "Bug 1429593 - Part 1: Extract functions for dealing with extensions into a separate file, r=jaws,mstriemer"
merging browser/components/preferences/in-content/jar.mn
merging browser/components/preferences/in-content/main.js
merging browser/components/preferences/in-content/preferences.js
merging browser/components/preferences/in-content/preferences.xul
merging browser/components/preferences/in-content/search.js
warning: conflicts while merging browser/components/preferences/in-content/preferences.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 70

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944832 [details]
Bug 1429593 - Part 1: Extract functions for dealing with extensions into a separate file

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

Comment 71

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

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

Comment 72

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa845d221e7a
Part 1: Extract functions for dealing with extensions into a separate file, r=jaws,mstriemer
https://hg.mozilla.org/integration/autoland/rev/9ce1b89f8744
Part 2: Show that a WebExtension is managing the proxy config setting, r=jaws,mstriemer

Comment 73

[Cristina Coroiu [:ccoroiu]]

Backed out 2 changesets (bug 1429593) for for mass failing browser-chrome, e.g. browser/components/preferences/in-content/tests/browser_extension_controlled.js on a CLOSED TREE

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9ce1b89f874463196844012b121aada56d56f8a8

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=160838131&repo=autoland

Backout: https://hg.mozilla.org/integration/autoland/rev/814985e63c0e0fbea2996af50c2a0e8add8005b1

Comment 74

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8944833 [details]
Bug 1429593 - Part 2: Show that a WebExtension is managing the proxy config setting

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214970/diff/12-13/

Comment 75

[Bob Silverberg [:bsilverberg]]

Sorry about that. This time I submitted the changes to try again before landing. Hopefully all will be fine this time.

Comment 76

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46bac1deee39
Part 1: Extract functions for dealing with extensions into a separate file, r=jaws,mstriemer
https://hg.mozilla.org/integration/autoland/rev/bb1b856328ab
Part 2: Show that a WebExtension is managing the proxy config setting, r=jaws,mstriemer

Comment 77

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/46bac1deee39
https://hg.mozilla.org/mozilla-central/rev/bb1b856328ab

Comment 78

[marius.santa]

Please provide an extension to be able to test this, I was unable to reproduce this scenario with other existing Proxy addons.

Comment 79

[Bob Silverberg [:bsilverberg]]

Attachment: An extension to test using browserSettings.proxyConfig

Comment 80

[marius.santa]

Attachment: proxy notifier.gif

Reproduced issue on Firefox 59.0a1 (20180110221942).
Retested and verified in Firefox 60.0a1 (20180308100121) on Windows 10 64Bit, Mac OS 10.13.2.