Closed Bug 1108019 Opened 6 years ago Closed 5 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+
https://hg.mozilla.org/mozilla-central/rev/e775f1fed03a
Status: NEW → RESOLVED
Closed: 5 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.