722987 - Places UI makes decisions based on global Private Browsing state

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Josh Matthews [:jdm]]

The global state is going away. Either the UI should not change based on the status, and the importing (http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/places.js#1405) should happen in such a way that no information is leaked, or there needs to be some window-level status that reflects the presence of PB docshells in a window.

Comment 1

[Josh Matthews [:jdm]]

http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/controller.js also uses PlacesUIUtils.privateBrowsing for these sorts of decisions.

Comment 2

[Josh Matthews [:jdm]]

Marco, do you know why we hide "Forget About This Site" and "Import From Another Browser" when in PB mode? Do you think we could just always allow those operations?

Comment 3

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #2)
> Marco, do you know why we hide "Forget About This Site" and "Import From
> Another Browser" when in PB mode? Do you think we could just always allow
> those operations?

See bug 463607 for the former, and bug 464736 for the latter.  Basically we disabled these UIs because there was no easy way to make them work in the global private browsing mode.  But they can both be re-enabled when we switch to per-window PB.

The thing which sucks here is that we can't land a patch here until everything related is ready, which makes me think whether we should add an --enable-per-winodw-pb build time option and hide this kind of stuff behind that (and turn it on by default when per-window PB is ready).

Comment 4

[Marco Bonardo [:mak]]

yes, anything that removes information and doesn't store information automatically can be re-enabled once we don't use anymore the global PB status.

Comment 5

[(no longer active)]

We don't need any special handling here, so the fix is really simple, just take all of the mentions of gPrivateBrowsingListener plus its definition and put them behind #ifndef MOZ_PER_WINDOW_PRIVATE_BROWSING chunks.

Comment 6

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Comment 7

[(no longer active)]

Comment on attachment 674798 [details] [diff] [review]
Patch v1

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

Thanks!

Comment 8

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f22d98c98b0

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0f22d98c98b0