Implement BrowserSetting.onChange
Categories
(WebExtensions :: General, enhancement, P2)
Tracking
(firefox57 wontfix, firefox72 fixed)
People
(Reporter: freddyb, Assigned: mixedpuppy, NeedInfo)
References
(Blocks 3 open bugs, )
Details
(Keywords: dev-doc-needed, parity-chrome, Whiteboard: [browserSetting])
Attachments
(1 file)
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.
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Assignee | ||
Comment 1•5 months ago
|
||
bumping priority here, secure-proxy would need this to get out of experimental api land.
Assignee | ||
Updated•5 months ago
|
Comment 2•4 months ago
|
||
Changing to corp confidential temporarily (till after Sept 10) on Tony and elan's request.
Comment 3•2 months ago
|
||
Can we open this up again? Is this still needed for secure-proxy?
Assignee | ||
Comment 5•2 months ago
|
||
@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.
Comment 6•2 months ago
|
||
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
Updated•Last month
|
Assignee | ||
Comment 7•Last month
|
||
Assignee | ||
Comment 8•Last month
|
||
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.
Comment 9•Last month
|
||
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:
- 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
- Just debounce onChange events for some arbitrary amount of time, say half a second.
Comment 10•Last month
|
||
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
Comment 11•Last month
|
||
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).
Assignee | ||
Comment 12•Last month
|
||
(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:
- 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.
- 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.
Comment 13•Last month
|
||
(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.
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.
Comment 14•Last month
|
||
(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:
Assignee | ||
Comment 15•Last month
|
||
(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.
Comment 16•Last month
|
||
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.
Comment 17•Last month
|
||
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
Assignee | ||
Updated•Last month
|
Assignee | ||
Comment 18•29 days ago
|
||
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.
Comment 19•18 days ago
|
||
Pushed by scaraveo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d9b0de6a3abc implement browser setting onChange event r=zombie
Comment 20•18 days ago
|
||
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.
Comment 21•18 days ago
|
||
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
Comment 22•18 days ago
|
||
Pushed by scaraveo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4d11ccc12529 implement browser setting onChange event r=zombie
Comment 23•18 days ago
|
||
Backed out changeset 4d11ccc12529 (Bug 1410412) on mixedpuppy's request
Backout link: https://hg.mozilla.org/integration/autoland/rev/fbf7d8f9ba38924f9a71fd5ffed8441ab97bcbf0
Comment 24•17 days ago
|
||
Pushed by scaraveo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cf30ec111af9 implement browser setting onChange event r=zombie
Comment 25•17 days ago
|
||
Backed out for failing browser_ext_chrome_settings_overrides_home.js
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=277076370&repo=autoland&lineNumber=5383
Backout: https://hg.mozilla.org/integration/autoland/rev/5e463f98aabbc479360d7a67a3127ddbf1cda4f0
Assignee | ||
Comment 26•17 days ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a06e916d347c36ae3b63feeda7ba759f93359907
Comment 27•16 days ago
|
||
Pushed by scaraveo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4a8770b8e4b9 implement browser setting onChange event r=zombie
Comment 28•15 days ago
|
||
bugherder |
Assignee | ||
Comment 29•10 days ago
|
||
This is reported as unsupported, it just needs to update to being supported in 72.
Comment 30•8 days ago
|
||
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!
Assignee | ||
Updated•8 days ago
|
Description
•