1036661 - Access to seenToS preference for the first time triggers an error message in the console

Status: RESOLVED FIXED

(Hello (Loop) :: General, defect)

Description

[Andrei Oprea [mention me on [e-mail]]]

seenToS preference needs a default otherwise on first access it will log an error message in the console

Comment 1

[Mark Banner (:standard8)]

Attachment: Avoid dumping a console error message for missing loop prefs when we expect them to be missing.

This adds a default value option, so that we can specify a default value for those cases where we expect the preference to be missing.

Justin, if you can review the non-content parts, and Dan can review the content ones. Thanks.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Do we actually need this functionality?  If not, it seems like it would be less code and tests to maintain to just add a default line for loop.seenToS to firefox.js...

Comment 3

[Mark Banner (:standard8)]

I'd rather not be adding default lines for every pref we want to store, just to avoid a warning. Whilst the ToS one could be defaulted in firefox.js, I don't think it would be reasonable to do the same for the default camera/microphone preferences that we're going to want real soon now.

Hence, whilst its nice to keep code to a minimum, I'm expecting that we're going to want this for multiple places.

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 8453753 [details] [diff] [review]
Avoid dumping a console error message for missing loop prefs when we expect them to be missing.

Review of attachment 8453753 [details] [diff] [review]:
-----------------------------------------------------------------

I would also have just added a default pref for this. :) That's generally encouraged as good-practice (as opposed to having hidden prefs for things), although for cases like this that isn't so relevant since you're really using this pref as a way to store some internal state.

But note that the way you've implemented this opens up a potential footgun -- if the pref has been set, but is the wrong type, the Services.pref.getCharPref() will throw and you'll get your API-specified default value instead. E.G. if someone (or somecode) sets loop.seenToS as a boolean true/false (which seems pretty natural, given it's used as such!), these calls will always return the default. And someone's going to be very confused when trying to debug what's happening. :)

I'd suggest making use of toolkit/modules/Preferences.jsm, or perhaps steal its error-checking for cases like this.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8453753 [details] [diff] [review]
Avoid dumping a console error message for missing loop prefs when we expect them to be missing.

Review of attachment 8453753 [details] [diff] [review]:
-----------------------------------------------------------------

I'll wait for a new patch for review.

My recollection is that I tried to use Preferences.jsm originally but failed for some reason (I _think_ it was libxul-linkage related), and feel back to the bad old API.

Comment 6

[Mark Banner (:standard8)]

Attachment: Accessing the loop panel for the first time triggers an error message in the console.

I think we'll just go with defaulting the preference for now. We might change how some of this is saved in the future anyway (e.g. when we get indexeddb in our special panels we could move some of these prefs across to that), so I think this is fine for now.

Comment 7

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7271b1984ca3

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7271b1984ca3

Comment 9

[u279076]

I'm untracking this for QE verification. Please needinfo me to request manual testing.