Honor session content settings even if "continue where I left off" is selected.

Honor the "clear on exit" setting even if "continue where I left off" is selected.

This also cancels the session restore UI changes except renaming the "reopen last pages" to "continue where I left off".

BUG=121072
TEST=Manual


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

[marja, 2012-03-30 12:31:12]

Could you review the following parts?
bauerb: all
sail: chrome/browser/profiles/*
estade: chrome/browser/resources/options2/* &
chrome/browser/ui/webui/options2/*

(crbug.com/121072 contains more information about this change of plans...)

[Bernhard Bauer, 2012-03-30 12:40:05]

http://codereview.chromium.org/9965012/diff/1006/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/9965012/diff/1006/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:375: SessionStartupPref::Type
startup_pref_type =
Nit: Indent

http://codereview.chromium.org/9965012/diff/1006/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:389: !clear_local_state_on_exit_ &&
Can you indent this line and the next one by one space (so it lines up with
"startup_pref_type == ...")...

http://codereview.chromium.org/9965012/diff/1006/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:391: CONTENT_SETTING_SESSION_ONLY);
...and this line by five spaces (so it's indented four spaces with respect to
the previous line)?

http://codereview.chromium.org/9965012/diff/1006/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:579: if (clear_local_state_on_exit_) {
Nit: Braces unnecessary.

[marja, 2012-03-30 12:51:37]

Thanks for comments!

http://codereview.chromium.org/9965012/diff/1006/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/9965012/diff/1006/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:375: SessionStartupPref::Type
startup_pref_type =
On 2012/03/30 12:40:05, Bernhard Bauer wrote:
> Nit: Indent

Done.

http://codereview.chromium.org/9965012/diff/1006/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:389: !clear_local_state_on_exit_ &&
On 2012/03/30 12:40:05, Bernhard Bauer wrote:
> Can you indent this line and the next one by one space (so it lines up with
> "startup_pref_type == ...")...

Done.

http://codereview.chromium.org/9965012/diff/1006/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:391: CONTENT_SETTING_SESSION_ONLY);
On 2012/03/30 12:40:05, Bernhard Bauer wrote:
> ...and this line by five spaces (so it's indented four spaces with respect to
> the previous line)?

Done.

http://codereview.chromium.org/9965012/diff/1006/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:579: if (clear_local_state_on_exit_) {
On 2012/03/30 12:40:05, Bernhard Bauer wrote:
> Nit: Braces unnecessary.

Done.

[Bernhard Bauer, 2012-03-30 12:58:21]

http://codereview.chromium.org/9965012/diff/5005/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (left):

http://codereview.chromium.org/9965012/diff/5005/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:921:
io_data->session_startup_pref()->GetValue());
Do we still need |io_data->session_startup_pref()|? If not, you could add a TODO
to clean it up (no need to do it in this CL though, to keep it more easily
mergeable).

http://codereview.chromium.org/9965012/diff/5005/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (left):

http://codereview.chromium.org/9965012/diff/5005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:293:
switches::kDisableRestoreSessionState));
I don't completely get this part. This effectively means that we always say
"continue where I left off", right? Why do we do this change here?

[marja, 2012-03-30 13:08:32]

http://codereview.chromium.org/9965012/diff/5005/chrome/browser/chrome_conten...
File chrome/browser/chrome_content_browser_client.cc (left):

http://codereview.chromium.org/9965012/diff/5005/chrome/browser/chrome_conten...
chrome/browser/chrome_content_browser_client.cc:921:
io_data->session_startup_pref()->GetValue());
On 2012/03/30 12:58:21, Bernhard Bauer wrote:
> Do we still need |io_data->session_startup_pref()|? If not, you could add a
TODO
> to clean it up (no need to do it in this CL though, to keep it more easily
> mergeable).

Yes, it won't be needed if we go this way (honor the content settings)
permanently. I added a TODO in profile_io_data.h.

http://codereview.chromium.org/9965012/diff/5005/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options2/content_settings_handler2.cc (left):

http://codereview.chromium.org/9965012/diff/5005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options2/content_settings_handler2.cc:293:
switches::kDisableRestoreSessionState));
On 2012/03/30 12:58:21, Bernhard Bauer wrote:
> I don't completely get this part. This effectively means that we always say
> "continue where I left off", right? Why do we do this change here?

This change disables all the UI changes (displaying the info dialog, disabling
settings..), the string change is done in startup_section.html.

[Bernhard Bauer, 2012-03-30 13:37:57]

On 2012/03/30 13:08:32, marja wrote:
>
http://codereview.chromium.org/9965012/diff/5005/chrome/browser/chrome_conten...
> File chrome/browser/chrome_content_browser_client.cc (left):
> 
>
http://codereview.chromium.org/9965012/diff/5005/chrome/browser/chrome_conten...
> chrome/browser/chrome_content_browser_client.cc:921:
> io_data->session_startup_pref()->GetValue());
> On 2012/03/30 12:58:21, Bernhard Bauer wrote:
> > Do we still need |io_data->session_startup_pref()|? If not, you could add a
> TODO
> > to clean it up (no need to do it in this CL though, to keep it more easily
> > mergeable).
> 
> Yes, it won't be needed if we go this way (honor the content settings)
> permanently. I added a TODO in profile_io_data.h.
> 
>
http://codereview.chromium.org/9965012/diff/5005/chrome/browser/ui/webui/opti...
> File chrome/browser/ui/webui/options2/content_settings_handler2.cc (left):
> 
>
http://codereview.chromium.org/9965012/diff/5005/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options2/content_settings_handler2.cc:293:
> switches::kDisableRestoreSessionState));
> On 2012/03/30 12:58:21, Bernhard Bauer wrote:
> > I don't completely get this part. This effectively means that we always say
> > "continue where I left off", right? Why do we do this change here?
> 
> This change disables all the UI changes (displaying the info dialog, disabling
> settings..), the string change is done in startup_section.html.

Do these UI changes consist only of the stuff about warning the user that we
don't delete stuff at shutdown etc.?

[marja, 2012-03-30 13:41:57]

On 2012/03/30 13:37:57, Bernhard Bauer wrote:
> On 2012/03/30 13:08:32, marja wrote:
http://codereview.chromium.org/9965012/diff/5005/chrome/browser/ui/webui/opti...
> > File chrome/browser/ui/webui/options2/content_settings_handler2.cc (left):
> > 
> >
>
http://codereview.chromium.org/9965012/diff/5005/chrome/browser/ui/webui/opti...
> > chrome/browser/ui/webui/options2/content_settings_handler2.cc:293:
> > switches::kDisableRestoreSessionState));
> > On 2012/03/30 12:58:21, Bernhard Bauer wrote:
> > > I don't completely get this part. This effectively means that we always
say
> > > "continue where I left off", right? Why do we do this change here?
> > 
> > This change disables all the UI changes (displaying the info dialog,
disabling
> > settings..), the string change is done in startup_section.html.
> 
> Do these UI changes consist only of the stuff about warning the user that we
> don't delete stuff at shutdown etc.?

The UI changes disabled by this are:
- disabling the "cookies... session only" radio button and the "clear cookies
... on exit" checkbox
- displaying the session restore info dialog

[Bernhard Bauer, 2012-03-30 13:42:49]

On 2012/03/30 13:41:57, marja wrote:
> On 2012/03/30 13:37:57, Bernhard Bauer wrote:
> > On 2012/03/30 13:08:32, marja wrote:
>
http://codereview.chromium.org/9965012/diff/5005/chrome/browser/ui/webui/opti...
> > > File chrome/browser/ui/webui/options2/content_settings_handler2.cc (left):
> > > 
> > >
> >
>
http://codereview.chromium.org/9965012/diff/5005/chrome/browser/ui/webui/opti...
> > > chrome/browser/ui/webui/options2/content_settings_handler2.cc:293:
> > > switches::kDisableRestoreSessionState));
> > > On 2012/03/30 12:58:21, Bernhard Bauer wrote:
> > > > I don't completely get this part. This effectively means that we always
> say
> > > > "continue where I left off", right? Why do we do this change here?
> > > 
> > > This change disables all the UI changes (displaying the info dialog,
> disabling
> > > settings..), the string change is done in startup_section.html.
> > 
> > Do these UI changes consist only of the stuff about warning the user that we
> > don't delete stuff at shutdown etc.?
> 
> The UI changes disabled by this are:
> - disabling the "cookies... session only" radio button and the "clear cookies
> ... on exit" checkbox
> - displaying the session restore info dialog

OK then. Just wanted to make sure we're not disabling more functionality than
necessary.

LGTM!

[sail, 2012-03-30 15:50:01]

http://codereview.chromium.org/9965012/diff/6009/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/9965012/diff/6009/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:365: bool restore_old_session_cookies =
false;
Hi Marja. Since this is getting a bit complicated can you move it into its own
function. Personally I find the following format easier to read:
bool xzy() {
  if (!session_restore_enabled_)
    return false;

  // Restore old session cookies after crashes.
  if (!DidLastSessionExitCleanly())
    return true.

  // Also restore after updates.
  if (BrowserInit::WasRestarted())
    return true;

  ...
}

[marja, 2012-03-30 16:26:42]

Oops, change of plans: we only honor the "clear on exit" content setting, not
the "session-only cookies" content setting.

http://codereview.chromium.org/9965012/diff/6009/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/9965012/diff/6009/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:365: bool restore_old_session_cookies =
false;
On 2012/03/30 15:50:01, sail wrote:
> Hi Marja. Since this is getting a bit complicated can you move it into its own
> function. Personally I find the following format easier to read:
> bool xzy() {
>   if (!session_restore_enabled_)
>     return false;
> 
>   // Restore old session cookies after crashes.
>   if (!DidLastSessionExitCleanly())
>     return true.
> 
>   // Also restore after updates.
>   if (BrowserInit::WasRestarted())
>     return true;
> 
>   ...
> }

This got reverted back to the original version.

[sail, 2012-03-30 16:28:58]

lgtm

[Evan Stade, 2012-03-30 20:21:57]

lgtm

[commit-bot: I haz the power, 2012-03-30 20:42:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/9965012/7008

[commit-bot: I haz the power, 2012-03-31 01:55:02]

Change committed as 130025