Closed Bug 723003 Opened 8 years ago Closed 7 years ago

nsLoginManager.js uses global Private Browsing state to make decisions

Categories

(Toolkit :: Password Manager, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: jdm, Assigned: sawrubh)

References

()

Details

Attachments

(1 file, 1 obsolete file)

Global PB state is going away. Instead, it should query the window's docshell.
As noted in bug 723004 comment 5... What should we do when there's no obvious window at hand? This is a component, after all, and the APIs here don't have a way to allow a caller to pass in a window reference...
APIs are being changed for other components such as the Download Manager to allow for a context, or default to non-private if none is provided. Can we do the same here?
Depends on: 723353
@Josh,@Ehsan,@Justin Is there an agreement upon what needs to be done then ? Wouldn't the normal solution (of converting singletons to instances) work in this case ?
No, singletons to instances won't work, since this is a component implementing the nsILoginManager interface.
(In reply to Saurabh Anand [:sawrubh] from comment #3)
> @Josh,@Ehsan,@Justin Is there an agreement upon what needs to be done then ?
> Wouldn't the normal solution (of converting singletons to instances) work in
> this case ?

What Josh said in comment 2.
I'm interested in working on this.
Cool!
Assignee: nobody → saurabhanandiit
Attached patch Patch v1 (obsolete) — Splinter Review
From the comments of Justin and Josh, I think we wouldn't have a window, then how am I supposed to QI to get the nsILoadContext ?

I have still done some QI'ing in this patch, using nsILoadContext. Can you guys tell me whether this is somewhat correct or not.
Attachment #638602 - Flags: feedback?(josh)
You should turn the getter into a method that takes a window argument. In the two places that use _inPrivateBrowsing, one can be moved down after the definition of the win variable, and the other can pass doc.defaultView.
Attachment #638602 - Flags: feedback?(josh)
Attached patch Patch v2Splinter Review
Passes all the concerned tests.
Attachment #638602 - Attachment is obsolete: true
Attachment #643821 - Flags: review?(ehsan)
Comment on attachment 643821 [details] [diff] [review]
Patch v2

Review of attachment 643821 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks!
Attachment #643821 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/2376910844ba
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.