Password manager code simplification

RESOLVED FIXED

Status

()

--
minor
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: neil, Assigned: neil)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

12 years ago
* 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
(Assignee)

Comment 1

12 years ago
Created attachment 274761 [details] [diff] [review]
diff -uw
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #274761 - Flags: review?(dolske)
(Assignee)

Comment 2

12 years ago
Created attachment 274762 [details] [diff] [review]
diff -up
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.
Attachment #274761 - Flags: review?(dolske) → review-
(Assignee)

Comment 4

12 years ago
(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.
(Assignee)

Comment 5

12 years ago
Created attachment 275871 [details] [diff] [review]
Remaining differences
Attachment #275871 - Flags: review?(dolske)
Comment on attachment 275871 [details] [diff] [review]
Remaining differences

Looks good.
Attachment #275871 - Flags: review?(dolske) → review+
(Assignee)

Comment 7

12 years ago
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.