SessionStore.jsm's _getMostRecentBrowserWindow should utilize RecentWindow.getMostRecentBrowserWindow

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Session Restore
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jdm, Assigned: dao)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
Created attachment 722302 [details] [diff] [review]
Make sessionstore discriminate between recent windows based on privacy status.
Attachment #722302 - Flags: review?(ttaubert)
Isn't some of the rest of the logic in _getMostRecentBrowserWindow duplicated in RecentWindow? Seems like we could clean this up further.
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 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.
Attachment #722302 - Flags: review?(ttaubert) → review-
(Reporter)

Comment 6

5 years ago
Created attachment 733020 [details] [diff] [review]
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.
Attachment #733020 - Flags: review?(ttaubert)
(Reporter)

Updated

5 years ago
Attachment #722302 - Attachment is obsolete: true
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.
Attachment #733020 - Flags: review?(ttaubert) → review-
(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.
(Assignee)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
Summary: SessionStore.jsm's _getMostRecentBrowserWindow should restrict its search to public windows → SessionStore.jsm's _getMostRecentBrowserWindow should utilize RecentWindow.getMostRecentBrowserWindow
(Assignee)

Comment 9

5 years ago
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.
(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.
(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.
Depends on: 860621
(Assignee)

Comment 12

5 years ago
Created attachment 8346511 [details] [diff] [review]
patch
Assignee: nobody → dao
Attachment #733020 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8346511 - Flags: review?(ttaubert)
Comment on attachment 8346511 [details] [diff] [review]
patch

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

Thanks!
Attachment #8346511 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/mozilla-central/rev/61bcbf08ca46
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.