847959 - Consider making RecentWindow return private windows even when public is desired in perma-PB mode

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Josh Matthews [:jdm]]

This would sidestep any future issues where we only want public windows when the public/private distinction actually matters.

Comment 1

[(no longer active)]

Yeah I think we should do this.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Make RecentWindow return any browser window regardless of requested privacy state when perma-PB mode is enabled.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 722257 [details] [diff] [review]
Make RecentWindow return any browser window regardless of requested privacy state when perma-PB mode is enabled.

>     function isSuitableBrowserWindow(win) {
>       return (!win.closed &&
>               win.toolbar.visible &&
>-              (!checkPrivacy ||
>+              (PrivateBrowsingUtils.permanentPrivateBrowsing ||
>+               !checkPrivacy ||
>                PrivateBrowsingUtils.isWindowPrivate(win) == aOptions.private));

make this:

  !checkPrivacy ||
  PrivateBrowsingUtils.permanentPrivateBrowsing ||
  ...

i.e. put the cheapest check first

Comment 4

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a60d1c61af47

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a60d1c61af47

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 722257 [details] [diff] [review]
Make RecentWindow return any browser window regardless of requested privacy state when perma-PB mode is enabled.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Potential for more undiscovered edge-case bugs when users use permanent private browsing mode.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): No risk.
String or UUID changes made by this patch: None

Comment 7

[Alex Keybl [:akeybl]]

(In reply to Josh Matthews [:jdm] from comment #6)
> User impact if declined: Potential for more undiscovered edge-case bugs when
> users use permanent private browsing mode.

We're trying to suss out a known user impact here - is an example that within private browsing, recently closed tabs wouldn't be listed?

Comment 8

[Josh Matthews [:jdm]]

No. This patch does not fix any identified open issues at this point; it is a belt and suspenders patch that would have avoided bug 843247.

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 722257 [details] [diff] [review]
Make RecentWindow return any browser window regardless of requested privacy state when perma-PB mode is enabled.

We'll take a bit of risk now rather than a black eye post-release, given your risk evaluation. Thanks jdm.

Comment 10

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8b9d035c60cf
https://hg.mozilla.org/releases/mozilla-beta/rev/889df8e0a193