Closed Bug 1410412 Opened 3 years ago Closed 1 year ago

Implement BrowserSetting.onChange

Categories

(WebExtensions :: General, enhancement, P2)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(firefox57 wontfix, firefox72 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla72
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox72 --- fixed

People

(Reporter: freddy, Assigned: mixedpuppy)

References

(Blocks 3 open bugs,
URL
)

Details

(Keywords: dev-doc-complete, parity-chrome, Whiteboard: [browserSetting])

Attachments

(1 file)

Bug 1410412 implement browser setting onChange event
47 bytes, text/x-phabricator-request
Details | Review 
imho it's pretty hard to write an extension that changes a pref when it can't properly monitor for other changes and update its UI accordingly.

One could use setInterval to poll though.
status-firefox57: --- → wontfix
Component: WebExtensions: Untriaged → WebExtensions: General
Priority: -- → P3
Whiteboard: [browserSetting]
Summary: Implement BrowserSetting.onChanged → Implement BrowserSetting.onChange
Product: Toolkit → WebExtensions

bumping priority here, secure-proxy would need this to get out of experimental api land.

Blocks: secure-proxy
Priority: P3 → P2
Keywords: parity-chrome

Changing to corp confidential temporarily (till after Sept 10) on Tony and elan's request.

Group: mozilla-employee-confidential

Can we open this up again? Is this still needed for secure-proxy?

Flags: needinfo?(tcinotto)

Baku thoughts here?

Flags: needinfo?(tcinotto) → needinfo?(amarchesini)

@Fallen, This should have been done from the very start, any extension using any number of settings should be (or be able to be) aware of when those settings change underneath them.

It would be nice to have browserSettings.onChanged listener. Currently, secure-proxy uses an experimental API to monitor these 2 prefs: network.proxy.type and network.proxy.no_proxies_on, but if we had browser.Settings.onChanged, we can simply listen for proxyType and passthrough changes. See: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/proxy/settings

Flags: needinfo?(amarchesini)
Group: mozilla-employee-confidential
Blocks: 1578508
Blocks: 1592706
No longer blocks: 1592706
Blocks: 1592704

I have a patch that introduces the settings.onChange api. The problem that this presents is, for those settings that make changes to multiple preferences, multiple notifications may happen. As well, the setting data received in any one of those may not be the correct "final" value.

I don't see a good way around this issue, we have no way to coalesce the preference changes. So I've a couple thoughts on how to change this so we can be a little more compatible, and a little more flexible.

Idea 1:

By default we could only notify when the setting is changed via the settings api. This would allow us to be "correct" for the setting onChange notification. This however will not provide notifications if something in the system takes over those values.

We have a need for some extensions to actually know if the underlying pref itself has changed. We could have an extra param for onChange.addListener where an extension provides a flag to use the preference observers.

Idea 2:

Many settings are a single pref, in which case the above is not an issue. We could add a "completed" flag on the details sent to the onChange listener. If completed is true, they would know that it is a reliable change. We would then turn this into a documentation issue and leave extensions to deal with the problem somehow.

Idea 3:

Combine parts of 1 and 2, make the details flag a "system" flag. For events emitted for changes through the settings api, system is false. For changes emitted via a pref observer, system is true.

Idea 4:

Essentially the same as 3, but we have two event managers, one for onChange and one for onSystemChange. onChange only fires for changes done via the api, onSystemChange hooks into the preference observers.

Looking for feedback and thoughts.

Flags: needinfo?(tomica)
Flags: needinfo?(rob)
Flags: needinfo?(philipp)
Flags: needinfo?(lgreco)

This doesn't look like a new problem, but something that's already present with reading the settings value if the corresponding prefs are changed outside of this api.

I don't think we should over-complicate this, so one of two simple options might be enough:

  1. Don't include the new value with the event, in the hope that by the time the extension asks for it, the whole group of prefs is already changed
  2. Just debounce onChange events for some arbitrary amount of time, say half a second.
Flags: needinfo?(tomica)

As Tomislav I've been also thinking that debounce the events may be a reasonable way to deal with it.

Nevertheless, I'm wondering if that approach could be problematic for some particular settings ([1]), when knowing sooner that the settings has been changed may actually make a difference (e.g. if the preference been set has an impact on some privacy-related behaviors, and the extension monitoring that pref needs to know about that change as soon as possible to change some of its behavior accordingly).

If the scenario describe above is actual realistic ([2]), I think it may be reasonable to treat those settings differently, e.g.

  • fire those events without any debounce, even if the settings value that the extension would get isn't going to change
  • or cache the last value fired and don't fire it again if the "settings computed value" is not going to be different

[1]: I think that for most of them it shouldn't make any big difference, e.g. a little latency in emitting an changed event for "newTabPosition" shouldn't be really a problem
[2]: and at a first glance it seems it is, given that one of the extensions that wants to use this API is the secure-proxy extension

Flags: needinfo?(lgreco)

Not related to the needinfo, but still related to this bugzilla issue:

we should call the new API event browserSettings.onChanged (instead of onChange) to make it consistent with the naming conventions used by other similar events we already have (e.g. the sessions API has a session.onChanged API event).

(In reply to :Tomislav Jovanovic :zombie from comment #9)

This doesn't look like a new problem, but something that's already present with reading the settings value if the corresponding prefs are changed outside of this api.

I don't think we should over-complicate this, so one of two simple options might be enough:

  1. Don't include the new value with the event, in the hope that by the time the extension asks for it, the whole group of prefs is already changed

I suppose we could do that, it would be incompatible with chrome, but that may not be such a big deal, we haven't had this api all this time.

  1. Just debounce onChange events for some arbitrary amount of time, say half a second.

I'm hoping to avoid that, I could see it causing weird problems that are hard to understand.

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #11)

we should call the new API event browserSettings.onChanged

The chrome compatible api is setting.onChange.

https://developer.chrome.com/extensions/types#ChromeSetting

I'm now inclined to just document the behavior, and suggest that extensions rely on getting the value again rather than what is received via the event. This really only effects those settings that use more than one pref, which is in the minority.

(In reply to Shane Caraveo (:mixedpuppy) from comment #12)

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #11)

we should call the new API event browserSettings.onChanged

The chrome compatible api is setting.onChange.

https://developer.chrome.com/extensions/types#ChromeSetting

sigh :-(
yeah, we need to name it onChange then, we could have it as an alias (as we did for menus and contextMenus), but it is really not worth it.

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #10)

(e.g. if the preference been set has an impact on some privacy-related behaviors, and the extension monitoring that pref needs to know about that change as soon as possible to change some of its behavior accordingly).

We already sorta "debounce" all the webRequest.onX events by up to 250ms, so things (especially privacy-related things) can already get out of sync, and it hasn't been an issue.

  • fire those events without any debounce, even if the settings value that the extension would get isn't going to change

I'm afraid that may end up in extensions wars over some settings, and debounce could at least slow that down somewhat. :shrug:

(In reply to :Tomislav Jovanovic :zombie from comment #14)

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #10)

(e.g. if the preference been set has an impact on some privacy-related behaviors, and the extension monitoring that pref needs to know about that change as soon as possible to change some of its behavior accordingly).

We already sorta "debounce" all the webRequest.onX events by up to 250ms, so things (especially privacy-related things) can already get out of sync, and it hasn't been an issue.

We would need to coalesce the onChange events, but the problem is that we don't know how many we'd get for a particular setting. That the reason behind Idea 1.

  • fire those events without any debounce, even if the settings value that the extension would get isn't going to change

I'm afraid that may end up in extensions wars over some settings, and debounce could at least slow that down somewhat. :shrug:

I'm not sure I follow here. Precedence is by install time. So the potential for changing is minimal. Once a "newer" extension takes control of the setting, the "older" extension cannot take control back. The "newer" extension can clear the setting, which would remove itself from the precedence list and control is relinquished to the prior extension in the precedence list.

We would need to coalesce the onChange events, but the problem is that we don't know how many we'd get for a particular setting. That the reason behind Idea 1.

We don't need to if we use a time-based debounce, just like we don't know how many webRequests events are coming.

I'm not sure I follow here. Precedence is by install time.

Ah, I missed that, much better.

I suppose that onPrefsChanged cannot be used because it only detects changes via ExtensionPreferencesManager, right?

I hope that there is no need to debounce. onChange events should only fire if the extension-observable value changes (see my comment at https://phabricator.services.mozilla.com/D51324#1566719 ).
If the setting's value may be unstable due to expected changes in preference value, then it could be implemented in the callback themselves - https://searchfox.org/mozilla-central/rev/35873cfc312a6285f54aa5e4ec2d4ab911157522/toolkit/components/extensions/ExtensionPreferencesManager.jsm#441

Flags: needinfo?(rob)
Assignee: nobody → mixedpuppy

I've been thinking about this a bunch, and I think that the right path to go is to only have notifications happen when EPM does the change.

  • about:preferences should be blocking changes to any prefs already, if it is extension controlled. So if the user "changes" it, they've disabled or uninstalled, situations where we would get the update. This alone will allow an extension to know a newer extension has taken control.
  • I need to revive bug 1548700 which would allow for selecting a specific extension (or back to default/user values) to control a pref set, in which case it would also go through EPM.
  • as part of that bug or another followup, we can add something to push a notification that the setting has changed if the user chooses/changes default/user-set values. That should catch extensions that are just watching for changes, but not using the setting.

That leaves about:config changes, which are already a non-starter.

So this patch can take care of the first bullet, the others will come later.

Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d9b0de6a3abc
implement browser setting onChange event r=zombie
Backout by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fdd07df83c87
Backed out changeset d9b0de6a3abc for failing at browser_extension_controlled.js on a CLOSED TREE.

Backed out changeset d9b0de6a3abc (bug 1410412) for failing at browser_extension_controlled.js on a CLOSED TREE.

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=d9b0de6a3abc950bccbe20bb4eff14baffd5f830&selectedJob=276868666

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=276868666&repo=autoland&lineNumber=2446

Log snippet:

[task 2019-11-18T23:17:55.449Z] 23:17:55 INFO - Console message: [JavaScript Error: "Error: An unexpected error occurred" {file: "moz-extension://fe719452-0f64-43ec-a56a-92cff3e4df5a/%7B442ee706-a772-4624-a2fa-bb7891fcf7a1%7D.js" line: 2}]
[task 2019-11-18T23:17:55.450Z] 23:17:55 INFO - Buffered messages finished
[task 2019-11-18T23:17:55.451Z] 23:17:55 INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_extension_controlled.js | Test timed out -
[task 2019-11-18T23:17:55.451Z] 23:17:55 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-11-18T23:17:55.452Z] 23:17:55 INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_extension_controlled.js | Extension left running at test shutdown -
[task 2019-11-18T23:17:55.452Z] 23:17:55 INFO - Stack trace:
[task 2019-11-18T23:17:55.452Z] 23:17:55 INFO - chrome://mochikit/content/browser-test.js:test_ok:1299
[task 2019-11-18T23:17:55.453Z] 23:17:55 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:109
[task 2019-11-18T23:17:55.453Z] 23:17:55 INFO - chrome://mochikit/content/browser-test.js:nextTest:577
[task 2019-11-18T23:17:55.453Z] 23:17:55 INFO - chrome://mochikit/content/browser-test.js:timeoutFn:1190
[task 2019-11-18T23:17:55.454Z] 23:17:55 INFO - setTimeout handler*chrome://mochikit/content/browser-test.js:Tester_execTest:1137
[task 2019-11-18T23:17:55.454Z] 23:17:55 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:932
[task 2019-11-18T23:17:55.455Z] 23:17:55 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:805
[task 2019-11-18T23:17:55.456Z] 23:17:55 INFO - GECKO(2006) | MEMORY STAT | vsize 20975883MB | residentFast 1853MB
[task 2019-11-18T23:17:55.456Z] 23:17:55 INFO - TEST-OK | browser/components/preferences/in-content/tests/browser_extension_controlled.js | took 90299ms
[task 2019-11-18T23:17:55.456Z] 23:17:55 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-11-18T23:17:55.457Z] 23:17:55 INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_extension_controlled.js | Found a tab after previous test timed out: about:preferences#privacy -
[task 2019-11-18T23:17:55.457Z] 23:17:55 INFO - checking window state

Flags: needinfo?(mixedpuppy)
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d11ccc12529
implement browser setting onChange event r=zombie
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf30ec111af9
implement browser setting onChange event r=zombie
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a8770b8e4b9
implement browser setting onChange event r=zombie

https://hg.mozilla.org/mozilla-central/rev/4a8770b8e4b9

Status: NEW → RESOLVED
Closed: 1 year ago
status-firefox72: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

This is reported as unsupported, it just needs to update to being supported in 72.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/types/BrowserSetting/onChange

Flags: needinfo?(mixedpuppy)
Keywords: dev-doc-needed

Hello,

Will this fix require manual validation? If yes, please provide some steps to reproduce in order to correctly test it and also, please set the "qe-verify+" flag. Otherwise, could the "qe-verify-" flag be added? Thanks!

Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy) → qe-verify-
Flags: needinfo?(philipp)

Compatibility data changes merged and details added to the release note. Any other changes needed?

Flags: needinfo?(mixedpuppy)

thanks!

Flags: needinfo?(mixedpuppy)

I was about to open a bug report because I though this feature didn’t work, but I had the presence of mind to 1) see in which version this has been added on MDN, 2) open the Firefox 72 for Developers page and find the link to Bugzilla about this feature, 3) read a lot of comments to finally understand that this feature doesn’t do what the documentation is saying.

So, on the BrowserSetting.onChange page:

The BrowserSetting.onChange event is fired when the setting is changed.

And on the browserSettings.closeTabsByDoubleClick. The setting can be changed by the user in about:config.) page:

By default, closeTabsByDoubleClick is false. The setting can be changed by the user in about:config.

So obviously, I think that it’s possible to react to change mades through about:config (but really I have users that change the keys corresponding to newTabPosition in about:config and I was eager to be able to react immediately, cleanly to the change).

And as developer, it’s so tempting to quickly change a value in about:config to test if my code works. So it kind of sounds obvious that if it didn’t worked, it would be precised. And it’s not obvious that «about:config changes […] are already a non-starter».

So here it is, I added the information so that people trying to use this feature don’t fall into the same trap.

It’s not the first time I’ve been bitten by the doc being incomplete, omitting important details or missing to clarify things that aren’t obviously.

This bug report I opened because the documentation wasn’t clear isn’t even closed. I had to sit for two months, nobody giving me the obvious answer, until my curiosity led me to read the documentation and find the answer by mistake, I then fixed the bug myself by improving the documentation (edit 1, edit 2).

Please close bug 1574025 and don’t forget to to write down important limitations of the API in the docs. I don’t mind improving the docs, but I would have preferred to avoid all this wasted time and frustration in the first place.

Thanks for the update to the docs.(In reply to ariasuni from comment #33)

So obviously, I think that it’s possible to react to change mades through about:config (but really I have users that change the keys corresponding to newTabPosition in about:config and I was eager to be able to react immediately, cleanly to the change).

And as developer, it’s so tempting to quickly change a value in about:config to test if my code works. So it kind of sounds obvious that if it didn’t worked, it would be precised. And it’s not obvious that «about:config changes […] are already a non-starter».

The current backend for the BrowserSettings API does not account for changes through about:config. There is an existing bug about showing the effect of extensions on the preference in bug 1595865, but I didn't find a separate bug about onChanged for externally modified settings.

It’s not the first time I’ve been bitten by the doc being incomplete, omitting important details or missing to clarify things that aren’t obviously.

This bug report I opened because the documentation wasn’t clear isn’t even closed. I had to sit for two months, nobody giving me the obvious answer, until my curiosity led me to read the documentation and find the answer by mistake, I then fixed the bug myself by improving the documentation (edit 1, edit 2).

We regularly triage unprioritized bug reports, but after triaging we do usually not look at bugs again until it comes up again. In your specific case, the bug report had little detail to go on, and I am not surprised that the existing API was not identified because very few people would recognize the existence of the requested feature based on that description.

In the future, if you have support questions, ask us a question on Matrix, or open a thread on Discourse. See https://wiki.mozilla.org/Add-ons/Community/

See Also: → 1595865

OK, thanks for the answer, I’ll try to think about using these other channels next time.

My comment was ranty because it’s sometimes frustrating, but I appreciate the work that’s done here and hope my feedback is constructive.

You need to log in before you can comment on or make changes to this bug.