Fixes for the kWasRestarted pref.

Fixes for the kWasRestarted pref.

Reset the kWasRestarted pref upon successful restart. Set it only when restarting. Write
the persistent preferences after setting it.

The preference was introduced in http://codereview.chromium.org/8745015.

BUG=106948
TEST=see bug


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

[marja, 2011-12-13 10:02:34]

Hi,

I got the kWasRestarted preference adding badly wrong in
http://codereview.chromium.org/8745015 and this CL tries to fix it.

Could you review the following parts:
finnur: all
mirandac: chrome/browser/profiles/*

Thanks!

[Finnur, 2011-12-13 10:53:03]

This LGTM but I think you should probably consult someone who knows the shutdown
sequence better than I do and the possible paths it can take. Joi has been
working in that area recently, in case you need someone in the European
timezone... sky is probably another good candidate.

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

http://codereview.chromium.org/8937001/diff/1/chrome/browser/ui/browser_init....
chrome/browser/ui/browser_init.h:77: static bool was_restarted();
Document this one. Specifically, it should be made pretty clear that this has a
side-effect of setting the variable to false. Also, this should be WasRestarted,
not was_restarted (or have a name that reflects the reset functionality).

[Miranda Callahan, 2011-12-13 10:59:15]

profiles/* LGTM w/comment nit.

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

http://codereview.chromium.org/8937001/diff/1/chrome/browser/ui/browser_init....
chrome/browser/ui/browser_init.h:255: static bool was_restarted_read_;
could you add a comment here to explain the difference between these two bools?

[marja, 2011-12-13 11:09:10]

Thanks for comments!

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

http://codereview.chromium.org/8937001/diff/1/chrome/browser/ui/browser_init....
chrome/browser/ui/browser_init.h:77: static bool was_restarted();
On 2011/12/13 10:53:04, Finnur wrote:
> Document this one. Specifically, it should be made pretty clear that this has
a
> side-effect of setting the variable to false. Also, this should be
WasRestarted,
> not was_restarted (or have a name that reflects the reset functionality).

Done.

http://codereview.chromium.org/8937001/diff/1/chrome/browser/ui/browser_init....
chrome/browser/ui/browser_init.h:255: static bool was_restarted_read_;
On 2011/12/13 10:59:16, Miranda Callahan wrote:
> could you add a comment here to explain the difference between these two
bools?

Done.

[Jói, 2011-12-13 11:27:41]

LGTM

http://codereview.chromium.org/8937001/diff/7001/chrome/browser/ui/browser_li...
File chrome/browser/ui/browser_list.cc (right):

http://codereview.chromium.org/8937001/diff/7001/chrome/browser/ui/browser_li...
chrome/browser/ui/browser_list.cc:523:
pref_service->ScheduleSavePersistentPrefs();
I was slightly concerned that this might not guarantee the pref to be saved
before browser shutdown, but it does.  Scheduling the save is actually a no-op
in practice I think (see chrome/common/json_pref_store.cc) but
BrowserShutdown::ShutdownPreThreadsStop puts the save on the queue of the file
thread before the quit message is posted to the file thread, and the file thread
is joined before the main thread exits, so this is guaranteed to run.

[commit-bot: I haz the power, 2011-12-13 12:01:00]

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

[commit-bot: I haz the power, 2011-12-13 14:16:08]

Change committed as 114213

[sky, 2011-12-13 16:54:41]

Any chance of a test here to make sure wasrestarted is set correctly and unset
correctly?

http://codereview.chromium.org/8937001/diff/1013/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/8937001/diff/1013/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:501: bool BrowserInit::was_restarted_ = false;
Since these are only used in WasRestarted, why not make them static there?

http://codereview.chromium.org/8937001/diff/1013/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.h (right):

http://codereview.chromium.org/8937001/diff/1013/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.h:260: // True if we have already read and reset
the preference kWasRestarted.
nit: newline between 259 and 260.

[marja, 2011-12-14 12:36:37]

sky, thanks for comments, I created a new CL for addressing them:
http://codereview.chromium.org/8929027

On 2011/12/13 16:54:41, sky wrote:
> Any chance of a test here to make sure wasrestarted is set correctly and unset
> correctly?
> 
>
http://codereview.chromium.org/8937001/diff/1013/chrome/browser/ui/browser_in...
> File chrome/browser/ui/browser_init.cc (right):
> 
>
http://codereview.chromium.org/8937001/diff/1013/chrome/browser/ui/browser_in...
> chrome/browser/ui/browser_init.cc:501: bool BrowserInit::was_restarted_ =
false;
> Since these are only used in WasRestarted, why not make them static there?
> 
>
http://codereview.chromium.org/8937001/diff/1013/chrome/browser/ui/browser_in...
> File chrome/browser/ui/browser_init.h (right):
> 
>
http://codereview.chromium.org/8937001/diff/1013/chrome/browser/ui/browser_in...
> chrome/browser/ui/browser_init.h:260: // True if we have already read and
reset
> the preference kWasRestarted.
> nit: newline between 259 and 260.