Closed Bug 1429593 Opened 2 years ago Closed 2 years ago

Show that a WebExtension is managing the proxy config setting

Categories

(WebExtensions :: Frontend, enhancement, P3)

enhancement

Tracking

(firefox60 verified)

VERIFIED FIXED
mozilla60
Tracking Status
firefox60 --- verified

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 7 obsolete files)

59 bytes, text/x-review-board-request
mstriemer
: review+
jaws
: review+
Details
59 bytes, text/x-review-board-request
jaws
: review+
mstriemer
: review+
Details
39.32 KB, image/png
mwalkington
: review+
Details
260.56 KB, image/png
mwalkington
: review+
Details
43.23 KB, image/png
mwalkington
: review+
Details
2.01 KB, application/zip
Details
160.19 KB, image/gif
Details
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.
Depends on: 1425535
No longer depends on: 1390161
Assignee: nobody → bob.silverberg
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.
Flags: needinfo?(mjaritz)
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.
Flags: needinfo?(mjaritz)
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.
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.
Flags: needinfo?(mjaritz)
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
Attachment #8944832 - Flags: review?(mstriemer) → review+
This is a new screenshot of the panel, which shows the latest version, which fixes the problems with the socks version radio buttons.
Attachment #8944716 - Attachment is obsolete: true
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.
Attachment #8944833 - Flags: review?(mstriemer) → review+
(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.
Flags: needinfo?(mjaritz)
(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?
Flags: needinfo?(mjaritz)
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.
Flags: needinfo?(mwalkington)
Flags: needinfo?(mjaritz)
Flags: needinfo?(bob.silverberg)
(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."
Flags: needinfo?(bob.silverberg)
Here is another string that I forgot about:

> "An extension, <icon> <insert name of extension>, requires Container Tabs."
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 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
Attachment #8944832 - Flags: review?(jaws) → review+
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.
Attachment #8944833 - Flags: review?(jaws) → review-
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 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}`);
```
Thanks for the review, Jared. I have responded to your issues with some questions/responses of my own.
Flags: needinfo?(jaws)
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.
Flags: needinfo?(jaws)
Looks like the wrong needinfo was removed.
Flags: needinfo?(mwalkington) → needinfo?(jaws)
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
Flags: needinfo?(jaws)
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."
Flags: needinfo?(mheubusch)
Attached image Main page without extension control (obsolete) —
This is the new version of the main page when an extension is not in control.
Attachment #8944715 - Attachment is obsolete: true
Attachment #8944839 - Attachment is obsolete: true
This is the new version of the main page when an extension is in control.
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.
Attachment #8947589 - Flags: review?(mjaritz)
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.
Attachment #8944833 - Flags: review+ → review?(mstriemer)
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?
Attachment #8947589 - Flags: review?(mjaritz) → review-
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);
```
Attachment #8944833 - Flags: review+
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?
(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?
Flags: needinfo?(mconley)
Attachment #8947586 - Attachment is obsolete: true
Attachment #8948380 - Flags: review?(mwalkington)
Attachment #8947587 - Attachment is obsolete: true
Attachment #8948381 - Flags: review?(mwalkington)
Attachment #8947589 - Attachment is obsolete: true
Attachment #8948382 - Flags: review?(mwalkington)
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 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 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 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 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.
Attachment #8944833 - Flags: review?(jaws) → review+
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.
Attachment #8948381 - Attachment is obsolete: true
Attachment #8948381 - Flags: review?(mwalkington)
Attachment #8948485 - Flags: review?(mwalkington)
(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?
Flags: needinfo?(mwalkington)
(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)
Flags: needinfo?(mconley)
(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."
Flags: needinfo?(mwalkington)
(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?
Flags: needinfo?(mjaritz)
(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.
Flags: needinfo?(mjaritz)
Flags: needinfo?(mheubusch)
(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.
Pushed by bsilverberg@mozilla.com:
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
(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!
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.
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.
Flags: needinfo?(bob.silverberg)
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)
Pushed by bsilverberg@mozilla.com:
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
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
Flags: needinfo?(bob.silverberg)
Sorry about that. This time I submitted the changes to try again before landing. Hopefully all will be fine this time.
Flags: needinfo?(bob.silverberg)
Pushed by bsilverberg@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/46bac1deee39
https://hg.mozilla.org/mozilla-central/rev/bb1b856328ab
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Attachment #8948380 - Flags: review?(mwalkington) → review+
Attachment #8948382 - Flags: review?(mwalkington) → review+
Attachment #8948485 - Flags: review?(mwalkington) → review+
Depends on: 1437335
Please provide an extension to be able to test this, I was unable to reproduce this scenario with other existing Proxy addons.
Flags: needinfo?(bob.silverberg)
Flags: needinfo?(bob.silverberg)
Attached image 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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
Depends on: 1505330
Regressions: 1548856
You need to log in before you can comment on or make changes to this bug.