Preferences.jsm does not need to guard against clearUserPref throwing

RESOLVED FIXED in mozilla24

Status

()

Toolkit
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Gavin, Assigned: riadh.chtara)

Tracking

Trunk
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 4 obsolete attachments)

I noticed one minor issue with the code being moved into Preferences.jsm in toolkit in bug 848519.

I believe it was written before bug 487059, so its _reset method specifically swallows NS_ERROR_UNEXPECTED exceptions. This isn't necessary anymore since clearUserPref now never throws.
Whiteboard: [good first bug]
(Assignee)

Comment 1

4 years ago
hey,
I will fix it.
(Assignee)

Comment 2

4 years ago
Created attachment 751206 [details] [diff] [review]
delete non needed guard

delete non needed guard
Assignee: nobody → riadh.chtara
Status: NEW → ASSIGNED
Comment on attachment 751206 [details] [diff] [review]
delete non needed guard

Review of attachment 751206 [details] [diff] [review]:
-----------------------------------------------------------------

If _reset is only this one line, it's probably better to remove this function and change it's callers to just call clearUserPref.
(Assignee)

Comment 4

4 years ago
Created attachment 751721 [details] [diff] [review]
deleting _reset
Attachment #751721 - Flags: review+
Attachment #751721 - Flags: feedback+
(Assignee)

Comment 5

4 years ago
Thanks for the review
I updated
Comment on attachment 751721 [details] [diff] [review]
deleting _reset

I didn't spend enough time on my previous drive-by feedback to know if this is good or not. Please don't set a review+ or feedback+ unless someone has already granted you the approval.
Attachment #751721 - Flags: review+
Attachment #751721 - Flags: feedback+
Attachment #751206 - Attachment is obsolete: true
(Assignee)

Comment 7

4 years ago
Sorry for that
Comment on attachment 751721 [details] [diff] [review]
deleting _reset

Thanks!

This is already covered by a test:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_Preferences.js#178
Attachment #751721 - Flags: review+
Riadh, can you please update your patch to have the necessary header information so someone else can check it in for you?

You can follow the steps here, https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
(Assignee)

Comment 10

4 years ago
Created attachment 751781 [details] [diff] [review]
patch with  header information
Attachment #751781 - Flags: review?
(Assignee)

Updated

4 years ago
Attachment #751781 - Flags: review? → review?(dietrich)
(Assignee)

Comment 11

4 years ago
@Gavin Sharp: you are welcome

@jaws: I just done that
Attachment #751781 - Flags: review?(dietrich) → review+
Comment on attachment 751781 [details] [diff] [review]
patch with  header information

># HG changeset patch
># Parent 0c1663454e490c4215e9df3cd6eef1f8025893d8
># User Riadh Chtara <riadh.chtara@gmail.com>
>Bug 861546 - Preferences.jsm is changed so it does not need to guard against clearUserPref throwing; r=reviewers

For the bug summary, this should be r=gavin instead of r=reviewers.
Attachment #751781 - Flags: review+
Attachment #751721 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 751788 [details] [diff] [review]
patch with header information
Attachment #751781 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
wait a second
(Assignee)

Comment 15

4 years ago
Created attachment 751792 [details] [diff] [review]
patch with  header information
Attachment #751788 - Attachment is obsolete: true
Attachment #751792 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2bcdf9b6a2a
Keywords: checkin-needed
(Assignee)

Comment 17

4 years ago
That s awesome
https://hg.mozilla.org/mozilla-central/rev/a2bcdf9b6a2a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.