Implement a support action which takes an internal pref name and resets that pref to the factory default. Note that this intentionally will not support setting prefs to an arbitrary value.

I'm not sure what a "support action" means, but isn't this what we want? https://mxr.mozilla.org/mozilla-central/source/modules/libpref/prefapi.h#140 This API is exposed in JS, and even used in some places throughout our code.

Have you read the context in bug 1031506 and the google doc specification? This bug is specifically about exposing this functionality through the support API, MozSelfSupport.webidl

(In reply to Benjamin Smedberg [:bsmedberg] from comment #2) > Have you read the context in bug 1031506 and the google doc specification? > This bug is specifically about exposing this functionality through the > support API, MozSelfSupport.webidl Yes, I read both. Though I more or less understand the purpose of webidl, this is something I still have to learn about. For instance I am wondering if it's possible/enough to create an identifier in MozSelfSupport.webidl and connect it with the existing C PREF_ClearUserPref() function?

You'd be connecting it with Preferences::ClearUser (or the JS equivalent of that, since this is implemented in JS). But yes.

WIP

WIP

WIP

Test OK, but considering still WIP.

Launched try build/test : https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b457e3ddb98

Try closed during above build because of bug 1147853. Launching again : https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f32aa9d15d7

Comment on attachment 8582379 [details] [diff] [review] bugzilla-1075160-5.patch Review of attachment 8582379 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/selfsupport/test/browser_1075160.js @@ +1,1 @@ > +const PREF_TO_TEST = "security.sandbox.macos.content"; Lets give this test a more descriptive name, maybe browser_selfsupportAPI.js? @@ +1,3 @@ > +const PREF_TO_TEST = "security.sandbox.macos.content"; > +const PREF_NEW_VALUE = 3; > +const PREF_UNDEF_VALUE = -1; You don't use the constants outside of the test() function, can we just move them into it? @@ +2,5 @@ > +const PREF_NEW_VALUE = 3; > +const PREF_UNDEF_VALUE = -1; > + > +function test() { > + let level_original = PREF_UNDEF_VALUE; We use camelCase. If you use Preferences.jsm you avoid the try-catch juggle: let originalLevel = Preferences.get(<prefName>, <fallbackValue>); We should make sure to always reset the pref using: registerCleanupFunction(() => ... I also think we need to cover more cases here, see the list in the webidl comment. ::: browser/experiments/Experiments.jsm @@ +320,5 @@ > let payload = yield reporter.collectAndObtainJSONPayload(); > return payload; > }); > }, > + Unneeded whitespace change. ::: dom/webidl/MozSelfSupport.webidl @@ +38,5 @@ > * @return Promise<Object> > * Resolved when the FHR payload data has been collected. > */ > Promise<object> getHealthReportPayload(); > + Nit: Trailing whitespace. @@ +39,5 @@ > * Resolved when the FHR payload data has been collected. > */ > Promise<object> getHealthReportPayload(); > + > + void clearUserPref(DOMString name); Add documentation like the above example. What is the expected behavior, can this throw? I assume that we need to handle this being called for: * prefs that we ship a default for, which may be user-set or not * prefs that were added as a user pref * prefs that don't exist

Georg, I think I replied to all your comments on my previous patch.

Comment on attachment 8590745 [details] [diff] [review] bugzilla-1075160-6.patch Review of attachment 8590745 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/selfsupport/moz.build @@ +5,5 @@ > # file, You can obtain one at http://mozilla.org/MPL/2.0/. > > EXTRA_COMPONENTS += [ > 'SelfSupportService.js', > + 'SelfSupportService.manifest' This change is unneeded? It seems actually nice to use trailing commas for less diff noise when adding entries, any issue with that? ::: browser/components/selfsupport/test/browser_selfsupportAPI.js @@ +5,5 @@ > + const PREF_NEW_VALUE = 3; > + const PREF_UNDEF_VALUE = -1; > + > + registerCleanupFunction(function() { > + Services.prefs.clearUserPref(PREF_TO_TEST); In general we can't use clearUserPref() here. The test harness may have set the pref to a specific value, which will be a user-set value. We have to reset it to the value it had at the test-start (or reset if it was not present). @@ +7,5 @@ > + > + registerCleanupFunction(function() { > + Services.prefs.clearUserPref(PREF_TO_TEST); > + }); > + let level_original = Preferences.get(PREF_TO_TEST, PREF_UNDEF_VALUE); Nit: camelCase here and below please. For proper test-coverage we should specifically test the cases mentioned before: * prefs that we ship a default for (which are not user-test, we can just assert that) * prefs that were added as a user pref (we can use a test-specific pref here) * prefs that don't exist (some test-specific pref-name too) Comments on what we're testing for in which section would help with test readability and maintenance. ::: dom/webidl/MozSelfSupport.webidl @@ +43,5 @@ > + /** > + * Resets a named pref to its original value. > + * > + * Refer to nsPrefBranch::ClearUserPref in > + * source/modules/libpref/nsPrefBranch.cpp for more information. We shouldn't point to the implementation for the doc comment (the implementation may change, but the interface behavior should stay consistent). Can you specify this methods behavior in the documentation here? I'd also add a @param and documentation.

(In reply to Georg Fritzsche [:gfritzsche] from comment #14) > For proper test-coverage we should specifically test the cases mentioned > before: > * prefs that we ship a default for (which are not user-test, we can just > assert that) ... "not user-set" is what i mean't. I think "extensions.hotfix.id" looks like a good candidate. We can just Assert for prefHasUserValue() being false to be sure.

The pref I test has a default value of 1 in a real browser, as per bug 1083344. But in the test environment it is undefined. I'm fine adding another pref to test (for the case it IS set in the test environment).

Comment on attachment 8591663 [details] [diff] [review] bugzilla-1075160-7.patch Review of attachment 8591663 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/selfsupport/test/browser_selfsupportAPI.js @@ +6,5 @@ > + const prefValueUndefined = -1; > + > + let prefValueOriginal = Preferences.get(prefToTest, prefValueUndefined); > + registerCleanupFunction(function() { > + if (prefValueOriginal == prefValueUndefined) We should find a pref that is expected to have a default value. Then we can just do something like (adding some comments so the test gets easier to read): Assert.ok(Preferences.has(DEFAULT_TEST_PREF)); registerCleanupFunction(() => /* reset pref */); // Test that resetting default pref doesn't change its value ... // Test that resetting that pref after it was user-set works. ... // Test resetting with a user-set pref that has no default value. ... // Test resetting a non-existant pref.

Factored the test function and call it: 1. for a pref which is set (found "browser.search.log" to be set in mochitest environment) 2. for a pref which is unset Added comments to ease maintenance.

Comment on attachment 8592182 [details] [diff] [review] bugzilla-1075160-8.patch Review of attachment 8592182 [details] [diff] [review]: ----------------------------------------------------------------- In general this looks much clearer now thanks to the comments and structuring. I think we still need to work on covering the different test cases though. ::: browser/components/selfsupport/test/browser_selfsupportAPI.js @@ +9,5 @@ > + // Record the original value > + let prefValueOriginal = Preferences.get(prefToTest, prefValueUndefined); > + > + // Register cleanup to restore orignal value > + registerCleanupFunction(function() { Let's move this and the preceding lines out of this helper function. If we keep it in the top-level function we can actually assert that the pref is set as expected and avoid the conditional juggles. I believe that is much clearer to read than factoring it out (it's just a test, so clarity should trump). @@ +31,5 @@ > + let prefValueCleared = Preferences.get(prefToTest, prefValueUndefined); > + Assert.equal(prefValueCleared, prefValueOriginal, "failed to reset pref value: " + prefToTest); > +} > + > +function test() { So far this test only covers two of the four cases mentioned in comment 17. I think we can't factor out all the four cases in one common helper function, so it's probably easier to just write things out in this test function. @@ +33,5 @@ > +} > + > +function test() { > + // Test changing a pref which is set in the mochitest context > + testOnePref(true, "browser.search.log", true); Is this a pref that has a default value and no user-set value? If yes let's assert that as suggested in comment 17 (in case someone changes that in the future we want clear failures). @@ +36,5 @@ > + // Test changing a pref which is set in the mochitest context > + testOnePref(true, "browser.search.log", true); > + > + // Test setting a pref which is unset in the mochitest context > + testOnePref(false, "thisPrefDoesntExist", 50); I don't think that we want to test setting a non-existing pref to a value. We want to test that calling MozSelfSupport.clearUserPref() makes no changes at all for a pref that doesn't exist, i.e.: * check pref doesnt exist * clearUserPref * check pref still doesnt exist

Ok, I see your point. I still think I can and should factor to some degree in order to ease reading. Basically I can cover the 4 cases you talk about in comment 17 by 1. check pref exists (or doesn't exist) 2. call clearUserPref 3. check we're consistent with 1 4. change (or set) the pref to a new value 5. call clearUserPref 6. check we're consistent with 1 And to the above with: 1. a pref which has a default value 2. a pref which has no default value Is my understanding right?

(In reply to André Reinald from comment #20) > I still think I can and should factor to some degree in order to ease > reading. > Basically I can cover the 4 cases you talk about in comment 17 by > 1. check pref exists (or doesn't exist) > 2. call clearUserPref > 3. check we're consistent with 1 > 4. change (or set) the pref to a new value > 5. call clearUserPref > 6. check we're consistent with 1 > > And to the above with: > 1. a pref which has a default value > 2. a pref which has no default value You don't need 1-3 for the pref with no default value and 6 will differ. I'd personally rather see less conditionals for this (pretty short) test so it's easier to understand from logs what happened. If we can do that and also factor it out, nice, otherwise it seems simpler to just write it out here.

WIP In browser_selfsupportAPI.js, assertions on lines 57 and 59 fail. My understanding is that a deleted pref is not restored by calling Services.prefs.clearUserPref() Is there something we can do about this?

Comment on attachment 8592354 [details] [diff] [review] bugzilla-1075160-9.patch Review of attachment 8592354 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/selfsupport/test/browser_selfsupportAPI.js @@ +2,5 @@ > + > +function test() { > + const prefValueUndefined = -1; > + > + let prefNewName = "browser.newpref.fake"; Let's make that `const prefNewName`. @@ +3,5 @@ > +function test() { > + const prefValueUndefined = -1; > + > + let prefNewName = "browser.newpref.fake"; > + Assert.ok(!Preferences.has(prefNewName)); A general point: Lets also add the additional parameter with a description here and below (e.g. "Should not have that pref yet") @@ +4,5 @@ > + const prefValueUndefined = -1; > + > + let prefNewName = "browser.newpref.fake"; > + Assert.ok(!Preferences.has(prefNewName)); > + Nit: Trailing whitespace. @@ +5,5 @@ > + > + let prefNewName = "browser.newpref.fake"; > + Assert.ok(!Preferences.has(prefNewName)); > + > + let prefExistingName = "extensions.hotfix.id"; const @@ +6,5 @@ > + let prefNewName = "browser.newpref.fake"; > + Assert.ok(!Preferences.has(prefNewName)); > + > + let prefExistingName = "extensions.hotfix.id"; > + Assert.ok(Preferences.has(prefExistingName)); Let's also assert that it is not user-set, i.e. `!Preference.isSet()` @@ +7,5 @@ > + Assert.ok(!Preferences.has(prefNewName)); > + > + let prefExistingName = "extensions.hotfix.id"; > + Assert.ok(Preferences.has(prefExistingName)); > + let prefExistingOriginal = Preferences.get(prefExistingName, prefValueUndefined); We made sure that the pref exists, so we can get rid of prefValueUndefined. prefExistingOriginalValue might be more clear? @@ +11,5 @@ > + let prefExistingOriginal = Preferences.get(prefExistingName, prefValueUndefined); > + > + registerCleanupFunction(function() { > + Preferences.set(prefExistingName, prefExistingOriginal); > + Services.prefs.deleteBranch(prefNewName); clearUserPref should be enough? @@ +18,5 @@ > + // 1. do nothing on an inexistent pref > + MozSelfSupport.clearUserPref(prefNewName); > + Assert.ok(!Preferences.has(prefNewName)); > + > + Nit: Only one newline. Dito the ones below. @@ +23,5 @@ > + // 2. creation of a new pref > + Preferences.set(prefNewName, 10); > + Assert.ok(Preferences.has(prefNewName)); > + let prefValueModified = Preferences.get(prefNewName, prefValueUndefined); > + Assert.equal(prefValueModified, 10, "failed to set new pref value"); We just set it, so i don't think we need to test the existence and the value here. (The pref system itself should have test coverage elsewhere) @@ +33,5 @@ > + // 3. do nothing on an unchanged existing pref > + MozSelfSupport.clearUserPref(prefExistingName); > + Assert.ok(Preferences.has(prefExistingName)); > + let prefValueCleared = Preferences.get(prefExistingName, prefValueUndefined); > + Assert.equal(prefValueCleared, prefExistingOriginal, "failed to reset to existing value"); We already assert that it exists, so we could just shorten to: Assert.equal(Preferences.get(prefExistingName), prefExistingOriginal, ... @@ +40,5 @@ > + // 4. change the value of an existing pref > + Preferences.set(prefExistingName, "anyone@mozilla.org"); > + Assert.ok(Preferences.has(prefExistingName)); > + prefValueModified = Preferences.get(prefExistingName, prefValueUndefined); > + Assert.equal(prefValueModified, "anyone@mozilla.org", "failed to set existing pref value"); Same here, i think we don't need the checks after setting the pref. @@ +45,5 @@ > + > + MozSelfSupport.clearUserPref(prefExistingName); > + Assert.ok(Preferences.has(prefExistingName)); > + prefValueCleared = Preferences.get(prefExistingName, prefValueUndefined); > + Assert.equal(prefValueCleared, prefExistingOriginal, "failed to reset to existing value"); `Assert.equal(Preferences.get(prefExistingName), ...` ? @@ +48,5 @@ > + prefValueCleared = Preferences.get(prefExistingName, prefValueUndefined); > + Assert.equal(prefValueCleared, prefExistingOriginal, "failed to reset to existing value"); > + > + > + // 5. delete an existing pref Per IRC, lets drop this part.

One last point: I think MozSelfSupport.resetPref() would be better than MozSelfSupport.clearUserPref().

I commented out the "case 5". I still think it would make sense to have it covered by clearUserPref(). But maybe not worth opening a bug?

Comment on attachment 8592761 [details] [diff] [review] bugzilla-1075160-10.patch Review of attachment 8592761 [details] [diff] [review]: ----------------------------------------------------------------- This looks close now. We only have minor points now and need to clear up the question on test-case #5. ::: browser/components/selfsupport/test/browser_selfsupportAPI.js @@ +3,5 @@ > +function test() { > + const prefValueUndefined = -1; > + > + const prefNewName = "browser.newpref.fake"; > + Assert.ok(!Preferences.has(prefNewName), "pref should not be defined"); "pref should not exist" @@ +6,5 @@ > + const prefNewName = "browser.newpref.fake"; > + Assert.ok(!Preferences.has(prefNewName), "pref should not be defined"); > + > + const prefExistingName = "extensions.hotfix.id"; > + Assert.ok(Preferences.has(prefExistingName), "pref should be defined"); "pref should not exist" @@ +7,5 @@ > + Assert.ok(!Preferences.has(prefNewName), "pref should not be defined"); > + > + const prefExistingName = "extensions.hotfix.id"; > + Assert.ok(Preferences.has(prefExistingName), "pref should be defined"); > + Assert.ok(!Preferences.isSet(prefExistingName), "pref should not be set"); Nit: "pref should not be user-set" @@ +17,5 @@ > + }); > + > + // 1. do nothing on an inexistent pref > + MozSelfSupport.clearUserPref(prefNewName); > + Assert.ok(!Preferences.has(prefNewName), "pref should not be defined"); "pref should still not exist"? And so on with s/defined/exist/ for the assert messages below? @@ +19,5 @@ > + // 1. do nothing on an inexistent pref > + MozSelfSupport.clearUserPref(prefNewName); > + Assert.ok(!Preferences.has(prefNewName), "pref should not be defined"); > + > + You still have 2 empty lines here and below; one is enough. @@ +23,5 @@ > + > + // 2. creation of a new pref > + Preferences.set(prefNewName, 10); > + Assert.ok(Preferences.has(prefNewName), "pref should be defined"); > + Assert.equal(Preferences.get(prefNewName), 10, "failed to set new pref value"); We have the assert message describe the expected state, not failure messages. Same for the "failed" messages below. @@ +32,5 @@ > + > + // 3. do nothing on an unchanged existing pref > + MozSelfSupport.clearUserPref(prefExistingName); > + Assert.ok(Preferences.has(prefExistingName), "pref should be defined"); > + Assert.equal(Preferences.get(prefExistingName), prefExistingOriginalValue, "failed to reset to existing value"); "should still have the same value"? ::: dom/webidl/MozSelfSupport.webidl @@ +42,5 @@ > + > + /** > + * Resets a named pref: > + * - to its default value if it has one, > + * - delete the named pref it it has no default value. This should still have @param documentation. Also, it should document whether it can throw. @@ +44,5 @@ > + * Resets a named pref: > + * - to its default value if it has one, > + * - delete the named pref it it has no default value. > + */ > + void clearUserPref(DOMString name); It is not a very big deal, but i now think that resetPref() is a better name.

(In reply to André Reinald from comment #25) > I still think it would make sense to have it covered by clearUserPref(). > But maybe not worth opening a bug? The open question here is on test-case #5 in the browser test. clearUserPref() doesn't reset a Services.prefs.deleteBranch(defaultPref) - do we care for the purposes of this API?

I can't see why NEEDINFO was set for me. Is there a question I need to answer here?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #29) > I can't see why NEEDINFO was set for me. Is there a question I need to > answer here? Yes, question is stated in comment 27.

I still don't understand what you're asking me to decide.

If you delete a default pref via say Services.prefs.deleteBranch(), the implementation here (using clearUserPref()), doesn't restore it until browser restart. Is that ok for the API behavior? If not, what do you know how to properly restore those prefs?

That sounds like a footgun API. I think we should probably remove that API (or make it reset a branch instead of removing it entirely), but I don't think it needs to be important for this API.

Comment on attachment 8592777 [details] [diff] [review] bugzilla-1075160-11.patch Review of attachment 8592777 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty good now, we should do a try-run on this. ::: browser/components/selfsupport/test/browser_selfsupportAPI.js @@ +41,5 @@ > + MozSelfSupport.resetPref(prefExistingName); > + Assert.ok(Preferences.has(prefExistingName), "pref should still exist"); > + Assert.equal(Preferences.get(prefExistingName), prefExistingOriginalValue, "pref value should be the same as original"); > + > + // 5. delete an existing pref Let's remove this per the preceding discussion. ::: dom/webidl/MozSelfSupport.webidl @@ +42,5 @@ > + > + /** > + * Resets a named pref: > + * - to its default value if it has one, > + * - delete the named pref it it has no default value. Does this throw? Please document this.

Comment on attachment 8593363 [details] [diff] [review] bugzilla-1075160-12.patch Review of attachment 8593363 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me now. One minor fix needed below and we should check that try results come back green. After that this should be good to go. ::: browser/components/selfsupport/test/browser_selfsupportAPI.js @@ +1,4 @@ > +Cu.import("resource://gre/modules/Preferences.jsm"); > + > +function test() { > + const prefValueUndefined = -1; This is unused, let's remove that line.

Removed prefValueUndefined. Not relaunching another try after that minor change.

This needs review from a DOM peer for the webidl change.

Georg, anyone to suggest for the webidl change?

Comment on attachment 8593390 [details] [diff] [review] bugzilla-1075160-13.patch Asking for review for the webidl change. Thanks.

Comment on attachment 8593390 [details] [diff] [review] bugzilla-1075160-13.patch Review of attachment 8593390 [details] [diff] [review]: ----------------------------------------------------------------- Seems fine given that this is [ChromeOnly], but we'll need a proper security review if we ever expose this to other things, as bsmedberg tells me we plan to do.