Closed
Bug 861546
Opened 11 years ago
Closed 11 years ago
Preferences.jsm does not need to guard against clearUserPref throwing
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: Gavin, Assigned: riadh.chtara)
References
Details
(Whiteboard: [good first bug])
Attachments
(1 file, 4 obsolete files)
|
1.59 KB,
patch
|
jaws
:
review+
|
Details | Diff | Splinter Review |
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.
|
||
Updated•11 years ago
|
Whiteboard: [good first bug]
|
Assignee | |
Comment 1•11 years ago
|
||
hey, I will fix it.
|
|
Assignee | |
Comment 2•11 years ago
|
||
delete non needed guard
|
||
Updated•11 years ago
|
Assignee: nobody → riadh.chtara
Status: NEW → ASSIGNED
|
|
||
Comment 3•11 years ago
|
||
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•11 years ago
|
||
Attachment #751721 -
Flags: review+
Attachment #751721 -
Flags: feedback+
|
|
Assignee | |
Comment 5•11 years ago
|
||
Thanks for the review I updated
|
|
||
Comment 6•11 years ago
|
||
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+
|
|
||
Updated•11 years ago
|
Attachment #751206 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 7•11 years ago
|
||
Sorry for that
|
Reporter | |
Comment 8•11 years ago
|
||
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+
|
|
||
Comment 9•11 years ago
|
||
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•11 years ago
|
||
Attachment #751781 -
Flags: review?
|
|
Assignee | |
Updated•11 years ago
|
Attachment #751781 -
Flags: review? → review?(dietrich)
|
|
Assignee | |
Comment 11•11 years ago
|
||
@Gavin Sharp: you are welcome @jaws: I just done that
|
|
||
Updated•11 years ago
|
Attachment #751781 -
Flags: review?(dietrich) → review+
|
|
||
Comment 12•11 years ago
|
||
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+
|
|
||
Updated•11 years ago
|
Attachment #751721 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 13•11 years ago
|
||
Attachment #751781 -
Attachment is obsolete: true
|
|
Assignee | |
Comment 14•11 years ago
|
||
wait a second
|
|
Assignee | |
Comment 15•11 years ago
|
||
Attachment #751788 -
Attachment is obsolete: true
|
|
||
Updated•11 years ago
|
Attachment #751792 -
Flags: review+
|
|
||
Updated•11 years ago
|
Keywords: checkin-needed
|
||
Comment 16•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2bcdf9b6a2a
Keywords: checkin-needed
|
|
Assignee | |
Comment 17•11 years ago
|
||
That s awesome
|
|
||
Comment 18•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a2bcdf9b6a2a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in
before you can comment on or make changes to this bug.
Description
•