Store the "browser autorestarted, last session must be restored" information in a preference.

Store the "browser autorestarted, last session must be restored" information in a preference.

This way ChromiumOS can run the same session restore code as Chrome.

BUG=1870
TEST=NONE


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

[marja, 2011-11-30 15:46:13]

Hi Finnur,

I'm working on a CL (1) for restoring session cookies after a restart / crash,
but it won't work as is in ChromiumOS. In ChromiumOS, a full logout (2) is
performed and the new instance is started without the kRestoreLastSession
command line flag.

(1) http://codereview.chromium.org/8533013

(2)
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/bro...

In this CL I add a preference for storing this "we have just restarted"
information. Sadly it seems that I cannot remove the command line flag
kRestoreLastSession totally, because of this Windows restart manager (3).

(3)
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chrome...

This is a bit of duplicate effort (we need to check both the flag and the
command line switch during startup), but is there an alternative? Or do you have
other ideas on how this might work?

[Finnur, 2011-11-30 18:32:04]

LGTM, with nits and some general comments: 

Update the bug number for the CL, it points to a duplicate.

Description comment: "This way ChromiumOS can run the same session restore code
than Chrome." 
Change "than" -> "as"

As for your questions:

> In this CL I add a preference for storing this "we have just restarted"
> information. Sadly it seems that I cannot remove the command line flag
> kRestoreLastSession totally, because of this Windows restart manager (3).

I don't see why you would want to remove the kRestoreLastSession usage here. I
think there is a case for the pref being used and a case for the command line
argument being used. This seems like a valid case to use the command line
argument and the wrong case for the pref. Bear in mind that my "restart CL"
didn't add the kRestoreLastSession param, if that is why you think it needs to
be removed. :)

If you still think it should be removed (and I would notify sky that you are
planning on it), then I'm not sure how to help you with the restart manager
case. I would suggest asking msw, who implemented the restart manager for
Chrome.

> This is a bit of duplicate effort (we need to check both the flag and the
> command line switch during startup), but is there an alternative? Or do you 
> have other ideas on how this might work?

Nope. I think checking both is probably the right thing to do.

http://codereview.chromium.org/8745015/diff/1/chrome/browser/ui/browser_list.cc
File chrome/browser/ui/browser_list.cc (left):

http://codereview.chromium.org/8745015/diff/1/chrome/browser/ui/browser_list....
chrome/browser/ui/browser_list.cc:521: 
nit: shouldn't we keep this line break?
nit: can you remove the braces on line 517, 519 also?

http://codereview.chromium.org/8745015/diff/1/chrome/browser/ui/browser_list.h
File chrome/browser/ui/browser_list.h (right):

http://codereview.chromium.org/8745015/diff/1/chrome/browser/ui/browser_list....
chrome/browser/ui/browser_list.h:118: static void AttemptUserExit(bool restart);
nit: maybe document the |restart| param (also below).

http://codereview.chromium.org/8745015/diff/1/chrome/browser/ui/browser_list....
chrome/browser/ui/browser_list.h:132: //  Note that he exit process may be
interrupted by download or
nit: extra space at the front.

http://codereview.chromium.org/8745015/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/8745015/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:1200: const char kWasRestarted[] = "was.restarted";
Shouldn't this variable be named kWasAutoRestarted, with value
"was.autorestarted"?

[marja, 2011-12-02 09:23:41]

Hi & thanks for comments.

Ok, I guess we need both, the command line flag and the preference. In this CL,
I removed setting the command line flag from browser_shutdown.cc, since the
preference is set anyway, is that OK?

http://codereview.chromium.org/8745015/diff/1/chrome/browser/ui/browser_list.cc
File chrome/browser/ui/browser_list.cc (left):

http://codereview.chromium.org/8745015/diff/1/chrome/browser/ui/browser_list....
chrome/browser/ui/browser_list.cc:521: 
On 2011/11/30 18:32:04, Finnur wrote:
> nit: shouldn't we keep this line break?
> nit: can you remove the braces on line 517, 519 also?

Done. Braces removed in the not-yet-committed CL (8533013) this is based on,
line break kept.

http://codereview.chromium.org/8745015/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/8745015/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:1200: const char kWasRestarted[] = "was.restarted";
On 2011/11/30 18:32:04, Finnur wrote:
> Shouldn't this variable be named kWasAutoRestarted, with value
> "was.autorestarted"?

It's not set only when chrome autorestarts, but also when the user selects
"Update chrome" from the menu and chrome restarts. That cannot be called
"autorestart", right?

[Finnur, 2011-12-02 16:48:50]

> I removed setting the command line flag from browser_shutdown.cc, since the
> preference is set anyway, is that OK?

Yes, I'm fine with that.

> It's not set only when chrome autorestarts, but also when the user selects
> "Update chrome" from the menu and chrome restarts. That cannot be called
> "autorestart", right?

Fair enough.

Still LGTM.

[commit-bot: I haz the power, 2011-12-05 11:28:46]

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

[commit-bot: I haz the power, 2011-12-05 11:28:48]

Can't process patch for file chrome/browser/ui/browser.cc.
File's status is None, patchset upload is incomplete.

[commit-bot: I haz the power, 2011-12-05 11:48:13]

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

[commit-bot: I haz the power, 2011-12-05 11:48:19]

Presubmit check for 8745015-12014 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/profiles/profile_impl.cc

Presubmit checks took 2.2s to calculate.

[marja, 2011-12-05 11:56:24]

Hi mirandac, could you review chrome/browser/profiles/* of this CL? Thanks!

[Miranda Callahan, 2011-12-05 14:30:28]

LGTM with change.

http://codereview.chromium.org/8745015/diff/12014/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8745015/diff/12014/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:379: PrefService* pref_service =
g_browser_process->local_state();
You actually already have this as a local variable in line 318; you can get rid
of this declaration.

http://codereview.chromium.org/8745015/diff/12014/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:385:
pref_service->GetBoolean(prefs::kWasRestarted));
after removing the above declaration:

local_State->GetBoolean(prefs::kWasRestarted));

[marja, 2011-12-05 14:44:01]

Ah, thanks for pointing that out.

http://codereview.chromium.org/8745015/diff/12014/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/8745015/diff/12014/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:379: PrefService* pref_service =
g_browser_process->local_state();
On 2011/12/05 14:30:28, Miranda Callahan wrote:
> You actually already have this as a local variable in line 318; you can get
rid
> of this declaration.

Done.

http://codereview.chromium.org/8745015/diff/12014/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:385:
pref_service->GetBoolean(prefs::kWasRestarted));
On 2011/12/05 14:30:28, Miranda Callahan wrote:
> after removing the above declaration:
> 
> local_State->GetBoolean(prefs::kWasRestarted));

Done.

[commit-bot: I haz the power, 2011-12-05 14:46:44]

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

[commit-bot: I haz the power, 2011-12-05 15:10:27]

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

[commit-bot: I haz the power, 2011-12-05 16:30:00]

Change committed as 112985