Mac: don't restore explicitly closed windows.

Mac: don't restore explicitly closed windows.

With "On startup"/"Restore the pages that were open last" set (in Preferences), a window which was explicitly closed (using the red button, Cmd-Shift-W, the menu item -- but not due to quit) should not be restored on startup. This is the behaviour of Firefox and Camino (Safari doesn't appear to implement this feature).

Note about this patch: Depending on how we decide things will work for App mode, much more code could be disabled on Mac.

BUG=13341
TEST=Set the browser to restore pages on start (see above). Navigate somewhere interesting; close window; quit; restart; should open a browser with default window (NTP or homepage or whatever). Navigate somewhere; quit; restart; should open previously opened stuff. Make sure that session restore (also crash recovery) still work as expected.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=31164

[sky, 2009-11-05 16:50:08]

http://codereview.chromium.org/362016/diff/1/2
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/362016/diff/1/2#newcode241
Line 241: #if defined(OS_MACOSX)
Rather than an ifdef, could you add a boolean to chrome/browser/defaults that
describes the behavior, and use that boolean here. That way you only have the
ifdef in defaults and not here.

http://codereview.chromium.org/362016/diff/1/2#newcode250
Line 250: if (!has_open_trackable_browsers_ && BrowserList::size() > 1)
How come the > 1 check here?

http://codereview.chromium.org/362016/diff/1/2#newcode274
Line 274: pending_window_close_ids_.insert(window_id.id());
Do you also need to modify this?

[viettrungluu, 2009-11-05 21:34:50]

Thanks, I've updated the patch.

http://codereview.chromium.org/362016/diff/1/2
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/362016/diff/1/2#newcode241
Line 241: #if defined(OS_MACOSX)
On 2009/11/05 16:50:08, sky wrote:
> Rather than an ifdef, could you add a boolean to chrome/browser/defaults that
> describes the behavior, and use that boolean here. That way you only have the
> ifdef in defaults and not here.

Done. I also put the entire check into a method.

http://codereview.chromium.org/362016/diff/1/2#newcode250
Line 250: if (!has_open_trackable_browsers_ && BrowserList::size() > 1)
On 2009/11/05 16:50:08, sky wrote:
> How come the > 1 check here?

The "1" is the window which is being closed, so this says "if there are no open
trackable browsers and there are other windows".

After some discussion with Glen, I decided that closing the last non-app window
on Mac should probably be like quitting. (The principle should be that an app
window should act as much like its own application as possible.) But of course
App mode is not implemented on Mac yet, nor even designed....

http://codereview.chromium.org/362016/diff/1/2#newcode274
Line 274: pending_window_close_ids_.insert(window_id.id());
On 2009/11/05 16:50:08, sky wrote:
> Do you also need to modify this?

Yes. Done.

[sky, 2009-11-05 22:16:19]

LGTM with the following change.

http://codereview.chromium.org/362016/diff/3001/7
File chrome/browser/sessions/session_service.h (right):

http://codereview.chromium.org/362016/diff/3001/7#newcode332
Line 332: BrowserList::size() > 1);
Can you add a comment that at the time this invoked the browser hasn't been
removed, hence 1 and not 0.

[Ben Goodger (Google), 2009-11-05 22:23:57]

What do you want from me here? I'm happy if sky is.

-Ben

On Wed, Nov 4, 2009 at 5:53 PM,  <viettrungluu@chromium.org> wrote:
> Reviewers: pink, sky, Ben Goodger,
>
> Description:
> Mac: don't restore explicitly closed windows.
>
> With "On startup"/"Restore the pages that were open last" set (in
> Preferences),
> a window which was explicitly closed (using the red button, Cmd-Shift-W, =
the
> menu item -- but not due to quit) should not be restored on startup. This=
 is
> the
> behaviour of Firefox and Camino (Safari doesn't appear to implement this
> feature).
>
> Note about this patch: Depending on how we decide things will work for Ap=
p
> mode,
> much more code could be disabled on Mac (see comment in code).
>
> BUG=3D13341
> TEST=3DSet the browser to restore pages on start (see above). Navigate
> somewhere
> interesting; close window; quit; restart; should open a browser with defa=
ult
> window (NTP or homepage or whatever). Navigate somewhere; quit; restart;
> should
> open previously opened stuff. Make sure that session restore (also crash
> recovery) still work as expected.
>
> Please review this at http://codereview.chromium.org/362016
>
> Affected files:
> =A0M chrome/browser/sessions/session_service.cc
>
>
> Index: chrome/browser/sessions/session_service.cc
> diff --git a/chrome/browser/sessions/session_service.cc
> b/chrome/browser/sessions/session_service.cc
> index
> 4682ca380269dc3180a96be2469d35c05cd595fe..305e23453b8056e511a0aecd5c75c98=
ead7b6e20
> 100644
> --- a/chrome/browser/sessions/session_service.cc
> +++ b/chrome/browser/sessions/session_service.cc
> @@ -238,8 +238,21 @@ void SessionService::WindowClosing(const SessionID&
> window_id) {
> =A0 =A0 // false to true, so only update it if already true.
> =A0 =A0 has_open_trackable_browsers_ =3D HasOpenTrackableBrowsers(window_=
id);
> =A0 }
> +#if defined(OS_MACOSX)
> + =A0// On Mac, we record an explicit window close even if it's the last
> window
> + =A0// (since the browser stays open; quitting takes a different code pa=
th).
> The
> + =A0// condition below would record a window close as pending if, e.g., =
an
> App
> + =A0// mode window were still open, even though App mode is not yet
> implemented on
> + =A0// Mac.
> + =A0// TODO(viettrungluu): Determine behaviour for App mode. If the beha=
viour
> + =A0// isn't that described above, all the |pending_window_...| (and pro=
bably
> + =A0// |pending_tab_...|) code can be removed from the Mac build.
> + =A0if (!has_open_trackable_browsers_ && BrowserList::size() > 1)
> + =A0 =A0pending_window_close_ids_.insert(window_id.id());
> +#else
> =A0 if (!has_open_trackable_browsers_)
> =A0 =A0 pending_window_close_ids_.insert(window_id.id());
> +#endif
> =A0 else
> =A0 =A0 window_closing_ids_.insert(window_id.id());
> =A0}
>
>
>

[viettrungluu, 2009-11-05 23:32:22]

Thanks sky and Ben. I'm good to go.

On 2009/11/05 22:23:57, Ben Goodger wrote:
> What do you want from me here? I'm happy if sky is.
> 
> -Ben
> 
> On Wed, Nov 4, 2009 at 5:53 PM,  <mailto:viettrungluu@chromium.org> wrote:
> > Reviewers: pink, sky, Ben Goodger,
> >
> > Description:
> > Mac: don't restore explicitly closed windows.
> >
> > With "On startup"/"Restore the pages that were open last" set (in
> > Preferences),
> > a window which was explicitly closed (using the red button, Cmd-Shift-W, =
> the
> > menu item -- but not due to quit) should not be restored on startup. This=
>  is
> > the
> > behaviour of Firefox and Camino (Safari doesn't appear to implement this
> > feature).
> >
> > Note about this patch: Depending on how we decide things will work for Ap=
> p
> > mode,
> > much more code could be disabled on Mac (see comment in code).
> >
> > BUG=3D13341
> > TEST=3DSet the browser to restore pages on start (see above). Navigate
> > somewhere
> > interesting; close window; quit; restart; should open a browser with defa=
> ult
> > window (NTP or homepage or whatever). Navigate somewhere; quit; restart;
> > should
> > open previously opened stuff. Make sure that session restore (also crash
> > recovery) still work as expected.
> >
> > Please review this at http://codereview.chromium.org/362016
> >
> > Affected files:
> > =A0M chrome/browser/sessions/session_service.cc
> >
> >
> > Index: chrome/browser/sessions/session_service.cc
> > diff --git a/chrome/browser/sessions/session_service.cc
> > b/chrome/browser/sessions/session_service.cc
> > index
> > 4682ca380269dc3180a96be2469d35c05cd595fe..305e23453b8056e511a0aecd5c75c98=
> ead7b6e20
> > 100644
> > --- a/chrome/browser/sessions/session_service.cc
> > +++ b/chrome/browser/sessions/session_service.cc
> > @@ -238,8 +238,21 @@ void SessionService::WindowClosing(const SessionID&
> > window_id) {
> > =A0 =A0 // false to true, so only update it if already true.
> > =A0 =A0 has_open_trackable_browsers_ =3D HasOpenTrackableBrowsers(window_=
> id);
> > =A0 }
> > +#if defined(OS_MACOSX)
> > + =A0// On Mac, we record an explicit window close even if it's the last
> > window
> > + =A0// (since the browser stays open; quitting takes a different code pa=
> th).
> > The
> > + =A0// condition below would record a window close as pending if, e.g., =
> an
> > App
> > + =A0// mode window were still open, even though App mode is not yet
> > implemented on
> > + =A0// Mac.
> > + =A0// TODO(viettrungluu): Determine behaviour for App mode. If the beha=
> viour
> > + =A0// isn't that described above, all the |pending_window_...| (and pro=
> bably
> > + =A0// |pending_tab_...|) code can be removed from the Mac build.
> > + =A0if (!has_open_trackable_browsers_ && BrowserList::size() > 1)
> > + =A0 =A0pending_window_close_ids_.insert(window_id.id());
> > +#else
> > =A0 if (!has_open_trackable_browsers_)
> > =A0 =A0 pending_window_close_ids_.insert(window_id.id());
> > +#endif
> > =A0 else
> > =A0 =A0 window_closing_ids_.insert(window_id.id());
> > =A0}
> >
> >
> >