Last Comment Bug 551851 - Add Services.jsm goodness to login manager
: Add Services.jsm goodness to login manager
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Password Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.3a4
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-11 16:55 PST by Justin Dolske [:Dolske]
Modified: 2010-03-18 15:28 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.1 (26.86 KB, patch)
2010-03-11 16:55 PST, Justin Dolske [:Dolske]
paul: review+
Details | Diff | Splinter Review
Patch v.2 (26.86 KB, patch)
2010-03-11 18:25 PST, Justin Dolske [:Dolske]
vladimir: review+
Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2010-03-11 16:55:30 PST
Created attachment 432024 [details] [diff] [review]
Patch v.1

Bug 512784 added Services.jsm, we should use this goodness in login manager code.

Untested (waiting for my build to finish) but is simple enough.
Comment 1 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-03-11 17:26:02 PST
Comment on attachment 432024 [details] [diff] [review]
Patch v.1

r=zpao on the password manager part. I assume somebody else needs to give the nod to adding console to services...
Comment 2 Justin Dolske [:Dolske] 2010-03-11 18:25:24 PST
Created attachment 432040 [details] [diff] [review]
Patch v.2

Oops. Took out a QueryInterface I shouldn't have. xpcshell and mochitests now all pass.

Over to vlad for the change to Services.jsm (bottom of patch).
Comment 3 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-03-17 18:48:19 PDT
Comment on attachment 432040 [details] [diff] [review]
Patch v.2

>diff --git a/toolkit/components/passwordmgr/src/crypto-SDR.js b/toolkit/components/passwordmgr/src/crypto-SDR.js

>     init : function () {
>         // Connect to the correct preferences branch.
>         this._prefBranch = Cc["@mozilla.org/preferences-service;1"].
>                            getService(Ci.nsIPrefService);
>         this._prefBranch = this._prefBranch.getBranch("signon.");

Missed this before - Services.prefs this one too.
Comment 4 Justin Dolske [:Dolske] 2010-03-17 18:57:23 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/b264a7e3c0f5

(with extra change from comment 3)
Comment 5 Dão Gottwald [:dao] 2010-03-18 01:40:51 PDT
(In reply to comment #2)
> Took out a QueryInterface I shouldn't have.

Why not? Services.jsm seems to do it already...
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-03-18 12:03:24 PDT
(In reply to comment #5)
> (In reply to comment #2)
> > Took out a QueryInterface I shouldn't have.
> 
> Why not? Services.jsm seems to do it already...

Services.jsm QIs Services.prefs to nsIPrefBranch2, but getBranch returns an nsIPrefBranch, so that needs to be QIed.

Why does Services.jsm even QI to nsIPrefBranch2? I don't think that should do anything, let alone work...
Comment 7 Dão Gottwald [:dao] 2010-03-18 12:14:30 PDT
(In reply to comment #6)
> Services.jsm QIs Services.prefs to nsIPrefBranch2, but getBranch returns an
> nsIPrefBranch, so that needs to be QIed.

Right, I missed the getBranch...

> Why does Services.jsm even QI to nsIPrefBranch2? I don't think that should do
> anything, let alone work...

It's there to enable Services.prefs.add/removeObserver, which works fine.
Comment 8 Justin Dolske [:Dolske] 2010-03-18 15:28:06 PDT
(In reply to comment #7)

> Right, I missed the getBranch...

I'm glad I wasn't the only one to make this mistake. ;)

Note You need to log in before you can comment on or make changes to this bug.