723003 - nsLoginManager.js uses global Private Browsing state to make decisions

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Josh Matthews [:jdm]]

Global PB state is going away. Instead, it should query the window's docshell.

Comment 1

[Justin Dolske [:Dolske]]

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...

Comment 2

[Josh Matthews [:jdm]]

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?

Comment 3

[Saurabh Anand [:sawrubh]]

@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 ?

Comment 4

[Josh Matthews [:jdm]]

No, singletons to instances won't work, since this is a component implementing the nsILoginManager interface.

Comment 5

[(no longer active)]

(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.

Comment 6

[Saurabh Anand [:sawrubh]]

I'm interested in working on this.

Comment 7

[(no longer active)]

Cool!

Comment 8

[Saurabh Anand [:sawrubh]]

Attachment: Patch v1

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.

Comment 9

[Josh Matthews [:jdm]]

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.

Comment 10

[Saurabh Anand [:sawrubh]]

Attachment: Patch v2

Passes all the concerned tests.

Comment 11

[(no longer active)]

Comment on attachment 643821 [details] [diff] [review]
Patch v2

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

Looks great, thanks!

Comment 12

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2376910844ba

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2376910844ba