390447 - Password manager code simplification

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[[e-mail]]

* Merge nsLoginManager with its observers and listeners
  (this simplifies a lot of cross-references)
* Make all void functions return; instead of return 0;
* Make getAll* function pass their out count parameter to storage
* Simplify initialisation of an internal object

Comment 1

[[e-mail]]

Attachment: diff -uw

Comment 2

[[e-mail]]

Attachment: diff -up

Comment 3

[Justin Dolske [:Dolske]]

Comment on attachment 274761 [details] [diff] [review]
diff -uw


>     QueryInterface : XPCOMUtils.generateQI([Ci.nsILoginManager,
>+                                            Ci.nsIObserver,
>+                                            Ci.nsIFormSubmitObserver,
>+                                            Ci.nsIWebProgressListener,
>+                                            Ci.nsIDOMEventListener,
>                                             Ci.nsISupportsWeakReference]),

I'm not so sure about the removal of the utility objects... I implemented it that way deliberately to make things more readable by splitting the extra interfaces into their own implementations, so it was more clear what was going on. It seemed to me the original implementation was confusing in part because it wasn't clear what was going on when things got blobbed together. It there a compelling reason to change this?


>+    // nsIWebProgressListener
>         onStateChange : function (aWebProgress, aRequest,
>                                   aStateFlags,  aStatus) {
> 
>+        if (!this._remember)
>+            return;
>+
>             // STATE_START is too early, doc is still the old page.
>             if (!(aStateFlags & Ci.nsIWebProgressListener.STATE_TRANSFERRING))
>-                return 0;
>-
>-            if (!this._pwmgr._remember)
>-                return 0;
>+            return;

The common case here is that passwords are being remembered (enabled by default), but we get many onStateChange() notifications we want to ignore. Checking _remember after the state change is accepted will save a few cycles.


>     getAllLogins : function (count) {
>         this.log("Getting a list of all logins");
>-        var logins = this._storage.getAllLogins({});
>-        count.value = logins.length;
>-        return logins;
>+        return this._storage.getAllLogins(count);
>     },

Is that correct? The |count| arg is from XPCOM sillyness... I thought I remembered testing this when first testing how returning arrays worked (and finding that setting |count| was needed), but I might be wrong.


All the other little changes looked good.

Comment 4

[[e-mail]]

(In reply to comment #3)
>I'm not so sure about the removal of the utility objects... I implemented it
>that way deliberately to make things more readable by splitting the extra
>interfaces into their own implementations, so it was more clear what was going
>on. It seemed to me the original implementation was confusing in part because
>it wasn't clear what was going on when things got blobbed together.
* The methods are basically the same, the idea is to avoid those ugly extra links between your utility objects
* If you really wanted to split the implementations you would have a separate utility object for each observer notification (including form submit).

>The common case here is that passwords are being remembered (enabled by
>default), but we get many onStateChange() notifications we want to ignore.
>Checking _remember after the state change is accepted will save a few cycles.
Fair enough.

>>+        return this._storage.getAllLogins(count);
>Is that correct? The |count| arg is from XPCOM sillyness... I thought I
>remembered testing this when first testing how returning arrays worked (and
>finding that setting |count| was needed), but I might be wrong.
nsILoginManagerStorage's getAllLogins has the same function signature, and is therefore required to fill in count.value with the array length.

Comment 5

[[e-mail]]

Attachment: Remaining differences

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 275871 [details] [diff] [review]
Remaining differences

Looks good.

Comment 7

[[e-mail]]

Checked in.