Closed Bug 847955 Opened 7 years ago Closed 6 years ago

SessionStore.jsm's _getMostRecentBrowserWindow should utilize RecentWindow.getMostRecentBrowserWindow

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: jdm, Assigned: dao)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

No description provided.
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-
This looks like it should enable the desired behaviour while retaining similar semantics to the old code.
Attachment #733020 - Flags: review?(ttaubert)
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.
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
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
Attached patch patchSplinter Review
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.