Login manager should send observer notifications on various events

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Password Manager
P2
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: thunder, Assigned: Dolske)

Tracking

({dev-doc-complete, fixed1.9.1})

unspecified
mozilla1.9.2a1
dev-doc-complete, fixed1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
It would be useful if the login manager sent events on login added/removed/modified.

Currently a workaround is to implement a storage module shim that sits in the middle, passing along all API calls to the regular storage module.  It's lengthy to write, though, and does not work if more than one party wants to do it (since there can only be one storage module).
(Assignee)

Updated

10 years ago
Blocks: 395681
(Assignee)

Comment 1

10 years ago
Created attachment 361875 [details] [diff] [review]
Patch v.1 (WIP)

This also rolls in the changes from bug 477917, because a fair chunk of the code added there ends up rewritten here. [All the "let metainfo = {}" stuff.]
Assignee: nobody → dolske
(Assignee)

Updated

10 years ago
Flags: blocking1.9.1?
OK, so should bug 477917 be duped off to this or marked as [fixed by 475634]? My same questions from bug 477917 comment 3 apply here, too.
(In reply to comment #2)
> OK, so should bug 477917 be duped off to this or marked as [fixed by 475634]?

Nope, these are two different (but related bugs). Bug 477917 is about allowing us to retrieve the GUID from login objects, while this one deals with making the login manager give Observer notifications whenever events like add/remove/modify on login objects occur (Weave uses this to decide when to sync)
(Reporter)

Comment 4

10 years ago
Mike, you're right that the attached patch has fixes for both bug 477917 and this one.  But as Anant explained, they are not the same.
(Reporter)

Comment 5

10 years ago
And compat risk here is minimal.  The only case I can think of is that we'd break anyone who is using the same observer notification topic implemented here, but that is extremely unlikely.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
(Assignee)

Comment 6

10 years ago
(In reply to comment #2)
> OK, so should bug 477917 be duped off to this or marked as [fixed by 475634]?

The latter, probably. The bugs are independent, except that the implementation happened to touch the same places. So the code I added in 477917 was promptly rewritten by this bug. :) I suppose I could also look at just fixing 477917 in a forwards-looking way...
Whiteboard: [wip patch]
(Assignee)

Comment 7

10 years ago
Created attachment 363009 [details] [diff] [review]
Patch v.2 (WIP)

Updated to apply on top of bug 477917 changes. (No longer rolled into this patch, so it's simpler now.)
Attachment #361875 - Attachment is obsolete: true
(Assignee)

Comment 8

10 years ago
Created attachment 364010 [details] [diff] [review]
Patch v.3
Attachment #363009 - Attachment is obsolete: true
Attachment #364010 - Flags: review?(gavin.sharp)
(Assignee)

Updated

10 years ago
Whiteboard: [wip patch] → [needs review gavin]
(Assignee)

Updated

10 years ago
Keywords: dev-doc-complete

Updated

10 years ago
Attachment #364010 - Flags: review?(gavin.sharp) → review+

Updated

10 years ago
Whiteboard: [needs review gavin] → [has patch][has reviews]
(Assignee)

Comment 9

10 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/f31b9c9fb785
Flags: in-testsuite+
Whiteboard: [has patch][has reviews]
Target Milestone: --- → mozilla1.9.2a1

Updated

10 years ago
Whiteboard: [has patch][has reviews][ready to land]
(Assignee)

Comment 10

10 years ago
Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/426a9c885d64
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.9.1
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews][ready to land]
(Assignee)

Comment 11

9 years ago
Huh, bug history shows I added dev-doc-complete, when I meant to add dev-doc-needed. :-(
Keywords: dev-doc-complete → dev-doc-needed
(Assignee)

Comment 12

9 years ago
Created attachment 369927 [details] [diff] [review]
Extra tests

Added some additional testing, since I noticed the tests were not checking all of the details of the notifications.

Pushed: http://hg.mozilla.org/mozilla-central/rev/1e32bd840e03
The documentation here has been updated:

https://developer.mozilla.org/En/Observer_Notifications
https://developer.mozilla.org/en/Using_nsILoginManager#Login_Manager_notifications
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.