Last Comment Bug 861546 - Preferences.jsm does not need to guard against clearUserPref throwing
: Preferences.jsm does not need to guard against clearUserPref throwing
Status: RESOLVED FIXED
[good first bug]
:
Product: Toolkit
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla24
Assigned To: riadh.chtara
:
:
Mentors:
Depends on: 848519
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-13 13:02 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2013-05-22 00:03 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
delete non needed guard (1.24 KB, patch)
2013-05-17 13:56 PDT, riadh.chtara
no flags Details | Diff | Splinter Review
deleting _reset (1.37 KB, patch)
2013-05-20 09:20 PDT, riadh.chtara
gavin.sharp: review+
Details | Diff | Splinter Review
patch with header information (1.59 KB, patch)
2013-05-20 12:04 PDT, riadh.chtara
no flags Details | Diff | Splinter Review
patch with header information (227 bytes, patch)
2013-05-20 12:17 PDT, riadh.chtara
no flags Details | Diff | Splinter Review
patch with header information (1.59 KB, patch)
2013-05-20 12:21 PDT, riadh.chtara
jaws: review+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2013-04-13 13:02:24 PDT
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.
Comment 1 riadh.chtara 2013-05-17 13:21:52 PDT
hey,
I will fix it.
Comment 2 riadh.chtara 2013-05-17 13:56:46 PDT
Created attachment 751206 [details] [diff] [review]
delete non needed guard

delete non needed guard
Comment 3 Jared Wein [:jaws] (please needinfo? me) 2013-05-20 08:54:44 PDT
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.
Comment 4 riadh.chtara 2013-05-20 09:20:25 PDT
Created attachment 751721 [details] [diff] [review]
deleting _reset
Comment 5 riadh.chtara 2013-05-20 09:20:51 PDT
Thanks for the review
I updated
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2013-05-20 09:26:22 PDT
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.
Comment 7 riadh.chtara 2013-05-20 09:46:57 PDT
Sorry for that
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-05-20 10:24:10 PDT
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
Comment 9 Jared Wein [:jaws] (please needinfo? me) 2013-05-20 10:50:36 PDT
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
Comment 10 riadh.chtara 2013-05-20 12:04:19 PDT
Created attachment 751781 [details] [diff] [review]
patch with  header information
Comment 11 riadh.chtara 2013-05-20 12:08:55 PDT
@Gavin Sharp: you are welcome

@jaws: I just done that
Comment 12 Jared Wein [:jaws] (please needinfo? me) 2013-05-20 12:13:25 PDT
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.
Comment 13 riadh.chtara 2013-05-20 12:17:51 PDT
Created attachment 751788 [details] [diff] [review]
patch with header information
Comment 14 riadh.chtara 2013-05-20 12:19:07 PDT
wait a second
Comment 15 riadh.chtara 2013-05-20 12:21:13 PDT
Created attachment 751792 [details] [diff] [review]
patch with  header information
Comment 16 Ryan VanderMeulen [:RyanVM] 2013-05-20 13:59:13 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2bcdf9b6a2a
Comment 17 riadh.chtara 2013-05-20 14:13:02 PDT
That s awesome
Comment 18 Ryan VanderMeulen [:RyanVM] 2013-05-21 10:47:46 PDT
https://hg.mozilla.org/mozilla-central/rev/a2bcdf9b6a2a

Note You need to log in before you can comment on or make changes to this bug.