Closed
Bug 1108019
Opened 10 years ago
Closed 9 years ago
safePrefGetter triggers exceptions (and catches them), which is noisy for debuggers
Categories
(Toolkit :: Places, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: WeirdAl, Assigned: tranj23, Mentored)
Details
(Whiteboard: debugging-noise [good first bug][lang=js])
Attachments
(1 file, 3 obsolete files)
1.42 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
The attached patch is currently untested. I will test it very soon.
Comment 1•10 years ago
|
||
To me looks like some of the prefs are just set wrong?
Do you have an example of a pref that causes the noise and the event when that happens?
Reporter | ||
Comment 2•10 years ago
|
||
(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #1)
> To me looks like some of the prefs are just set wrong?
> Do you have an example of a pref that causes the noise and the event when
> that happens?
This is actually something I hit when using Firefox --app to run a XULRunner-based app, which didn't have the preferences defined.
Reporter | ||
Comment 3•10 years ago
|
||
This patch doesn't have syntax errors and works. :)
Attachment #8532626 -
Attachment is obsolete: true
Attachment #8532671 -
Flags: review?(mak77)
Reporter | ||
Comment 4•10 years ago
|
||
Actually, using Preferences.jsm would be better: less duplication.
Comment 5•10 years ago
|
||
(In reply to Alex Vincent [:WeirdAl] from comment #4)
> Actually, using Preferences.jsm would be better: less duplication.
the new code uses Preferences.jsm indeed, it's not worth fixing the old code.
Comment 6•10 years ago
|
||
Comment on attachment 8532671 [details] [diff] [review]
safePrefGetter.diff
Review of attachment 8532671 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +197,2 @@
> // If the pref isn't set, we want to use the default.
> + if (aPrefBranch.getPrefType(aName) != typeCheck) {
I think you don't need typeCheck at all.
you could leave the code as it was and here just do
if (aPrefBranch.getPrefType(aName) == Ci.nsIPrefBranch.PREF_INVALID)
return aDefault;
Attachment #8532671 -
Flags: review?(mak77) → feedback+
Reporter | ||
Comment 8•10 years ago
|
||
(In reply to Marco Bonardo [::mak] from comment #7)
> Do you still plan to complete this?
Go ahead and finish it off, Marco. I've been extraordinarily busy the last few months (which is why I haven't signed up for the commit privileges yet).
Flags: needinfo?(ajvincent)
Comment 9•10 years ago
|
||
Ok, let's make this a mentored bug, we just need to fix the first if using "in" and implement comment 6 (no need to change the "map" object).
Assignee: ajvincent → nobody
Mentor: mak77
Whiteboard: debugging-noise → debugging-noise [good first bug][lang=js]
Updated•9 years ago
|
Priority: -- → P2
Updated•9 years ago
|
Flags: needinfo?(mak77)
Assignee | ||
Comment 10•9 years ago
|
||
Hi, I made a new patch based on the previous patch and comment 6
Attachment #8532671 -
Attachment is obsolete: true
Attachment #8713480 -
Flags: review?(mak77)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → tranj23
Comment 11•9 years ago
|
||
Comment on attachment 8713480 [details] [diff] [review]
safePrefGetter.patch
Review of attachment 8713480 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +195,3 @@
> throw "Unknown type!";
> }
> + let [type, typeCheck] = types[defType];
I think all of the changes above this point can be removed (apart from the typo fix in the javadoc)
Indeed, by using PREF_INVALID we don't need anymore typeCheck
Attachment #8713480 -
Flags: review?(mak77)
Updated•9 years ago
|
Flags: needinfo?(mak77) → needinfo?(tranj23)
Assignee | ||
Comment 12•9 years ago
|
||
Ok, I undid some changes from previous patch
Attachment #8713480 -
Attachment is obsolete: true
Flags: needinfo?(tranj23)
Attachment #8716373 -
Flags: review?(mak77)
Comment 13•9 years ago
|
||
Comment on attachment 8716373 [details] [diff] [review]
rev2 - Reverted some changes from previous patch
Review of attachment 8716373 [details] [diff] [review]:
-----------------------------------------------------------------
r=me with the commit message changed to
Bug 1108019 - Avoid console exception spew from nsPlacesAutoComplete.js. r=mak
and the following comments addressed:
::: toolkit/components/places/nsPlacesAutoComplete.js
@@ +194,5 @@
> if (!type) {
> throw "Unknown type!";
> }
> + let [type, typeCheck] = types[defType];
> +
thse 2 lines should go away, nothing is using type nor typeCheck.
Attachment #8716373 -
Flags: review?(mak77) → review+
Comment 14•9 years ago
|
||
Comment 16•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Reporter | ||
Comment 17•9 years ago
|
||
Jeffrey, Marco, I just want to say "thank you" for getting this finished. This was a pain point in debugging, and those go unnoticed by non-developers... but when you're in the debugger, it's really hard to avoid. :)
You need to log in
before you can comment on or make changes to this bug.
Description
•