847955 - SessionStore.jsm's _getMostRecentBrowserWindow should utilize RecentWindow.getMostRecentBrowserWindow

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Comment 1

[Josh Matthews [:jdm]]

Attachment: Make sessionstore discriminate between recent windows based on privacy status.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Isn't some of the rest of the logic in _getMostRecentBrowserWindow duplicated in RecentWindow? Seems like we could clean this up further.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Er, also RecentWindow.getMostRecentWindow doesn't exist (only RecentWindow.getMostRecentBrowserWindow, which doesn't take a "type" option). Is this on top of some other patch?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Ah, I found bug 847952.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 722302 [details] [diff] [review]
Make sessionstore discriminate between recent windows based on privacy status.

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +3775,5 @@
>     * @returns Window reference
>     */
>    _getMostRecentBrowserWindow: function ssi_getMostRecentBrowserWindow() {
> +    var win = RecentWindow.getMostRecentWindow({type: "navigator:browser",
> +                                                private: true});

This restricts the search to private windows only, right? Isn't this bug about only public windows?

Also, like Gavin said, there is only a getMostRecentBrowserWindow() function which does the same as SS._getMostRecentBrowserWindow() and we should probably remove a lot of the duplicated code.

Comment 6

[Josh Matthews [:jdm]]

Attachment: Make sessionstore discriminate between recent windows based on privacy status.

This looks like it should enable the desired behaviour while retaining similar semantics to the old code.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 733020 [details] [diff] [review]
Make sessionstore discriminate between recent windows based on privacy status.

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

RecentWindow.getMostRecentBrowserWindow's logic is a little off:

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

We need to remove |win.toolbar.visible| so that |allowPopups| actually has any effect.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +3774,5 @@
>     * Returns most recent window
>     * @returns Window reference
>     */
>    _getMostRecentBrowserWindow: function ssi_getMostRecentBrowserWindow() {
> +    return RecentWindow.getMostRecentBrowserWindow({private: false,

Looks like we need to consider private windows, too. browser_819510_perwindowpb.js starts failing otherwise. We expect ss.getBrowserState() to correctly return with selectedWindow being a possibly private window.

There might be places where the _getMostRecentBrowserWindow() caller doesn't actually want/need a private window but that requires a little more investigation.

I like that we got rid of some duplicate code but seems we can't restrict the search to public windows easily here.

@@ +3775,5 @@
>     * @returns Window reference
>     */
>    _getMostRecentBrowserWindow: function ssi_getMostRecentBrowserWindow() {
> +    return RecentWindow.getMostRecentBrowserWindow({private: false,
> +                                                    skipPopups: false});

This should be 'allowPopups'. And we actually do want popups here or else tests start failing.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #7)
> > +    return RecentWindow.getMostRecentBrowserWindow({private: false,
> > +                                                    skipPopups: false});
> 
> And we actually do want popups here or else tests start failing.

Sorry, I overlooked that your patch does NOT SKIP popups which is the right thing to do.

Comment 9

[Dão Gottwald [:dao]]

You can also remove the BROKEN_WM_Z_ORDER #define.

(In reply to Tim Taubert [:ttaubert] from comment #7)
> RecentWindow.getMostRecentBrowserWindow's logic is a little off:

This sounds like something that should be uplifted and have its own bug.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #9)
> You can also remove the BROKEN_WM_Z_ORDER #define.

Good catch, thanks.

> (In reply to Tim Taubert [:ttaubert] from comment #7)
> > RecentWindow.getMostRecentBrowserWindow's logic is a little off:
> 
> This sounds like something that should be uplifted and have its own bug.

Yes, I agree, didn't think about uplifting.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to Tim Taubert [:ttaubert] from comment #7)
> > RecentWindow.getMostRecentBrowserWindow's logic is a little off:
> 
> This sounds like something that should be uplifted and have its own bug.

Filed bug 860621.

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch

Comment 13

[Dão Gottwald [:dao]]

https://tbpl.mozilla.org/?tree=Try&rev=c94d48983b12

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8346511 [details] [diff] [review]
patch

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

Thanks!

Comment 15

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/61bcbf08ca46

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/61bcbf08ca46