Closed
Bug 943998
Opened 6 years ago
Closed 6 years ago
Need debug and uri pref for FirefoxAccounts
Categories
(Firefox OS Graveyard :: General, defect)
Firefox OS Graveyard
General
Not set
Tracking
(Not tracked)
RESOLVED
FIXED
1.3 C1/1.4 S1(20dec)
People
(Reporter: edwong, Assigned: ferjm)
References
Details
(Whiteboard: [qa+])
Attachments
(1 file, 2 obsolete files)
20.79 KB,
patch
|
markh
:
review+
|
Details | Diff | Splinter Review |
We need a way to set the server uri and verbose debugging on device. For Persona we have: user_pref("toolkit.identity.uri", "https://firefoxos.anosrep.org"); user_pref("toolkit.identity.debug", true); Maybe we could have user_pref("toolkit.fxaccounts.uri", "https://stage.fxa.org"); user_pref("toolkit.fxaccounts.debug", true);
Updated•6 years ago
|
Whiteboard: [qa+]
Comment 1•6 years ago
|
||
Bug 935232 adds this pref: +// The URL of the Firefox Accounts auth server backend +pref("identity.fxaccounts.auth.uri", "https://api-accounts.dev.lcip.org/v1");
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → ferjmoreno
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8341053 -
Flags: review?(mhammond)
Assignee | ||
Comment 3•6 years ago
|
||
The attached patch adds the "identity.fxaccounts.debug" preference to enable/disable debug messages.
Comment 4•6 years ago
|
||
Comment on attachment 8341053 [details] [diff] [review] v1 Review of attachment 8341053 [details] [diff] [review]: ----------------------------------------------------------------- Is there any reason we can't use Log.jsm instead, similar to how sync does? Then a pref can control the log level and we will not be restricted to dump() output.
Attachment #8341053 -
Flags: review?(mhammond)
Comment 5•6 years ago
|
||
(In reply to Mark Hammond [:markh] from comment #4) > Is there any reason we can't use Log.jsm instead, similar to how sync does? > Then a pref can control the log level and we will not be restricted to > dump() output. See also bug 909967 - this introduces use of Log.jsm, but it currently hard-code a log-level of debug. Maybe this bug could extend its scope a little to provide a consistent logging interface - eg, using prefs to set each log's level?
Assignee | ||
Comment 6•6 years ago
|
||
Using Log.jsm sounds good to me. I'll update the patch soon. Thanks Mark!
Assignee | ||
Comment 7•6 years ago
|
||
With this patch, we add an "identity.fxaccounts.loglevel" preference that takes one of the string values at [1]. We will be logging in the console and if no preference is provided we will only log error messages. Mark, is this what you were referring to? I still need to rebase on top of bug 909967, check all the log messages and probably add three different levels (error, debug, info), but I would love to hear your feedback about the general approach (basically the changes in FxAccountsCommon.js). Thanks! [1] https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Log.jsm#45
Attachment #8341053 -
Attachment is obsolete: true
Attachment #8342480 -
Flags: feedback?(mhammond)
Comment 8•6 years ago
|
||
Comment on attachment 8342480 [details] [diff] [review] v2 Review of attachment 8342480 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/FxAccounts.jsm @@ +101,5 @@ > return d.promise; > }, > > getCertificate: function(data, keyPair, mustBeValidUntil) { > + log.debug("getCertificate " + internal.signedInUserStorage); also note the logger takes a second param, and object that will be JSON.stringified and appended to the end of the message - so something like: log.debug("getCertificate:", {signedInUserStorage: internal.signedInUserStorage}); might be appropriate in some cases. ::: services/fxaccounts/FxAccountsCommon.js @@ +9,5 @@ > + > +// loglevel preference should be one of: "FATAL", "ERROR", "WARN", "INFO", > +// "CONFIG", "DEBUG", "TRACE" or "ALL". We will be logging error messages by > +// default. > +const PREF_LOG_LEVEL = "identity.fxaccounts.loglevel"; you probably shouldn't use LOG_LEVEL - allcaps implies a const, which this isn't. Also, I'm a little surprised this works - I thought the level needed to be an integer. Sync does: this._log.level = Log.Level[pref_value]; which relies on the Log module's 'Level' object which maps strings to values. But in general, yeah, this is what I meant.
Attachment #8342480 -
Flags: feedback?(mhammond) → feedback+
Comment 9•6 years ago
|
||
Comment on attachment 8342480 [details] [diff] [review] v2 Review of attachment 8342480 [details] [diff] [review]: ----------------------------------------------------------------- ::: services/fxaccounts/FxAccountsCommon.js @@ +11,5 @@ > +// "CONFIG", "DEBUG", "TRACE" or "ALL". We will be logging error messages by > +// default. > +const PREF_LOG_LEVEL = "identity.fxaccounts.loglevel"; > +try { > + this.LOG_LEVEL = actually, another thought is that maybe this could go into a: XPCOMUtils.defineLazyModuleGetter(this, 'log', function() { // read the pref, create the log, set the level and create the formatter }); which should mean we don't pay any penalty for the preference read etc at import time, but only the first time we try and log something.
Assignee | ||
Comment 10•6 years ago
|
||
Sorry for the late update on this. I've been traveling and busy with other issues.
Attachment #8342480 -
Attachment is obsolete: true
Attachment #8346093 -
Flags: review?(mhammond)
Assignee | ||
Comment 11•6 years ago
|
||
Thanks again for the feedback Mark :) (In reply to Mark Hammond [:markh] from comment #8) > Comment on attachment 8342480 [details] [diff] [review] > v2 > > Review of attachment 8342480 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: services/fxaccounts/FxAccounts.jsm > @@ +101,5 @@ > > return d.promise; > > }, > > > > getCertificate: function(data, keyPair, mustBeValidUntil) { > > + log.debug("getCertificate " + internal.signedInUserStorage); > > also note the logger takes a second param, and object that will be > JSON.stringified and appended to the end of the message - so something like: > > log.debug("getCertificate:", {signedInUserStorage: > internal.signedInUserStorage}); > > might be appropriate in some cases. > For some reason, I couldn't make this work, so I had to manually do the JSON.stringify.
FWIW, I had the same problem with log and the second json arg.
Comment 13•6 years ago
|
||
Comment on attachment 8346093 [details] [diff] [review] v2 Review of attachment 8346093 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine to me. I suspect the logs might end up too noisy and might later need finer granularity, but we can deal with that as needed. [Turns out that second param to the log functions is only used when a StructuredFormatter is used, which seems a little dumb, but there you have it]
Attachment #8346093 -
Flags: review?(mhammond) → review+
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 14•6 years ago
|
||
Thanks Mark! https://hg.mozilla.org/integration/b2g-inbound/rev/b3d70eda988d
Comment 15•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b3d70eda988d
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C1/1.4 S1(20dec)
You need to log in
before you can comment on or make changes to this bug.
Description
•