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)

x86
All
defect

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)

Attached patch safePrefGetter.diff (obsolete) — Splinter Review
The attached patch is currently untested. I will test it very soon.
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?
(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.
Attached patch safePrefGetter.diff (obsolete) — Splinter Review
This patch doesn't have syntax errors and works. :)
Attachment #8532626 - Attachment is obsolete: true
Attachment #8532671 - Flags: review?(mak77)
Actually, using Preferences.jsm would be better: less duplication.
(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 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+
Do you still plan to complete this?
Flags: needinfo?(ajvincent)
(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)
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]
Priority: -- → P2
Flags: needinfo?(mak77)
Attached patch safePrefGetter.patch (obsolete) — Splinter Review
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: nobody → tranj23
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)
Flags: needinfo?(mak77) → needinfo?(tranj23)
Ok, I undid some changes from previous patch
Attachment #8713480 - Attachment is obsolete: true
Flags: needinfo?(tranj23)
Attachment #8716373 - Flags: review?(mak77)
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+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
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.

Attachment

General

Created:
Updated:
Size: