If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Support action: reset a pref to the default setting

RESOLVED FIXED in Firefox 40

Status

Firefox Health Report
Client: Desktop
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Benjamin Smedberg, Assigned: André Reinald)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

31 Branch
Firefox 40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 12 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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.
Flags: needinfo?(benjamin)
(Reporter)

Comment 2

3 years ago
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
Flags: needinfo?(benjamin)
(Assignee)

Comment 3

3 years ago
(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?
(Assignee)

Updated

3 years ago
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Comment 4

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

Updated

3 years ago
Assignee: nobody → areinald
(Assignee)

Comment 5

3 years ago
Created attachment 8571427 [details] [diff] [review]
bugzilla-1075160-1.patch

WIP
Attachment #8571427 - Flags: feedback?
(Assignee)

Updated

3 years ago
Attachment #8571427 - Flags: feedback?
(Assignee)

Comment 6

3 years ago
Created attachment 8571441 [details] [diff] [review]
bugzilla-1075160-2.patch

WIP
Attachment #8571427 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Created attachment 8581718 [details] [diff] [review]
bugzilla-1075160-3.patch

WIP
Attachment #8571441 - Attachment is obsolete: true
(Assignee)

Comment 8

3 years ago
Created attachment 8582365 [details] [diff] [review]
bugzilla-1075160-4.patch

Test OK, but considering still WIP.
Attachment #8581718 - Attachment is obsolete: true
(Assignee)

Comment 9

3 years ago
Created attachment 8582379 [details] [diff] [review]
bugzilla-1075160-5.patch
Attachment #8582365 - Attachment is obsolete: true
Attachment #8582379 - Flags: review?(gfritzsche)
(Assignee)

Comment 10

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

Comment 11

3 years ago
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
Attachment #8582379 - Flags: review?(gfritzsche)
(Assignee)

Comment 13

3 years ago
Created attachment 8590745 [details] [diff] [review]
bugzilla-1075160-6.patch

Georg, I think I replied to all your comments on my previous patch.
Attachment #8582379 - Attachment is obsolete: true
Attachment #8590745 - Flags: review?(gfritzsche)
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.
Attachment #8590745 - Flags: review?(gfritzsche)
(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.
(Assignee)

Comment 16

3 years ago
Created attachment 8591663 [details] [diff] [review]
bugzilla-1075160-7.patch

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).
Attachment #8590745 - Attachment is obsolete: true
Attachment #8591663 - Flags: review?(gfritzsche)
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.
Attachment #8591663 - Flags: review?(gfritzsche)
(Assignee)

Comment 18

3 years ago
Created attachment 8592182 [details] [diff] [review]
bugzilla-1075160-8.patch

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.
Attachment #8591663 - Attachment is obsolete: true
Attachment #8592182 - Flags: review?(gfritzsche)
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
Attachment #8592182 - Flags: review?(gfritzsche)
(Assignee)

Comment 20

3 years ago
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.
(Assignee)

Comment 22

3 years ago
Created attachment 8592354 [details] [diff] [review]
bugzilla-1075160-9.patch

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?
Attachment #8592182 - Attachment is obsolete: true
Attachment #8592354 - Flags: feedback?(gfritzsche)
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.
Attachment #8592354 - Flags: feedback?(gfritzsche) → feedback+
One last point:
I think MozSelfSupport.resetPref() would be better than MozSelfSupport.clearUserPref().
(Assignee)

Comment 25

3 years ago
Created attachment 8592761 [details] [diff] [review]
bugzilla-1075160-10.patch

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?
Attachment #8592354 - Attachment is obsolete: true
Flags: needinfo?(benjamin)
Attachment #8592761 - Flags: review?(gfritzsche)
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.
Attachment #8592761 - Flags: review?(gfritzsche)
(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?
(Assignee)

Comment 28

3 years ago
Created attachment 8592777 [details] [diff] [review]
bugzilla-1075160-11.patch
Attachment #8592761 - Attachment is obsolete: true
Attachment #8592777 - Flags: review?(gfritzsche)
(Reporter)

Comment 29

3 years ago
I can't see why NEEDINFO was set for me. Is there a question I need to answer here?
Flags: needinfo?(benjamin)
(Assignee)

Comment 30

3 years ago
(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.
(Reporter)

Comment 31

3 years ago
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?
(Reporter)

Comment 33

3 years ago
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.
Attachment #8592777 - Flags: review?(gfritzsche) → feedback+
(Assignee)

Comment 35

3 years ago
Created attachment 8593363 [details] [diff] [review]
bugzilla-1075160-12.patch
Attachment #8592777 - Attachment is obsolete: true
Attachment #8593363 - Flags: review?(gfritzsche)
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.
Attachment #8593363 - Flags: review?(gfritzsche) → feedback+
Status: NEW → ASSIGNED
(Assignee)

Comment 37

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ec347bd9a22
(Assignee)

Comment 38

3 years ago
Created attachment 8593390 [details] [diff] [review]
bugzilla-1075160-13.patch

Removed prefValueUndefined. Not relaunching another try after that minor change.
Attachment #8593363 - Attachment is obsolete: true
Attachment #8593390 - Flags: review?(gfritzsche)
Attachment #8593390 - Flags: review?(gfritzsche) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
This needs review from a DOM peer for the webidl change.
Flags: needinfo?(areinald)
Keywords: checkin-needed
(Assignee)

Comment 40

3 years ago
Georg, anyone to suggest for the webidl change?
Flags: needinfo?(areinald) → needinfo?(gfritzsche)
(Assignee)

Comment 41

3 years ago
Comment on attachment 8593390 [details] [diff] [review]
bugzilla-1075160-13.patch

Asking for review for the webidl change. Thanks.
Flags: needinfo?(gfritzsche)
Attachment #8593390 - Flags: review?(bobbyholley)
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.
Attachment #8593390 - Flags: review?(bobbyholley) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/ee8544241cd6
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ee8544241cd6
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 40
You need to log in before you can comment on or make changes to this bug.