Write "logout-started" event on next boot.

Write "logout-started" event on next boot.

Writing logout-started on process exit is unstable.
So moving it to next boot.

BUG=352130
TEST=manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274649

[Alexander Alekseev, 2014-05-30 00:16:56]

Please review.

jochen@ : chrome/browser/lifetime/application_lifetime.cc
nkostylev@, stejenjb@ : all

[Nikita (slow), 2014-05-30 06:57:44]

So essentiall on boot you're now writing both logout-started and
login-prompt-visible?

Where's the guarantee that former will be written before latter?

[Nikita (slow), 2014-05-30 07:07:02]

lgtm

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.cc (right):

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:345: return uptime + '^' + disk;
nit: Please extract this divider as a constant.

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.h (right):

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.h:87: void OnBoot();
nit: Comment is missing.

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.h:89: void
OnLogoutStarted(PrefService* state);
nit: Comment is missing.

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.h:150: static void
ClearLogoutStartedLastPreference();
nit: Comment is missing.

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:616:
BootTimesLoader::Get()->OnBoot();
nit: empty line after that line.

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:86:
registry->RegisterStringPref(prefs::kLogoutStartedLast, "");
nit: std::string()

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/lifetime/...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/lifetime/...
chrome/browser/lifetime/application_lifetime.cc:150: // Login screen should show
up in owner's locale.
nit: insert empty line before comment.

[stevenjb, 2014-05-30 16:20:23]

https://codereview.chromium.org/303233004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.cc (right):

https://codereview.chromium.org/303233004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:359: }
While on the one hand this is fairly trivial, if we ever expand Stats it will be
a pain to maintain. Maybe consider using DictionaryValue and [de]serializing
that as a JSON string.

https://codereview.chromium.org/303233004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:376: const size_t space_at =
uptime.find_first_of(' ');
Where is this ' ' coming from?

https://codereview.chromium.org/303233004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:382: }
This seems like either it should be a member of Stats (which should really be a
class now, and could be forward declared in the header and defined in the .cc
file). Or if we make Stats a DictionaryValue, this + [De]Serialize should just
be local helper methods.

https://codereview.chromium.org/303233004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.h (right):

https://codereview.chromium.org/303233004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.h:88: void OnBoot();
"Boot" is rather unspecific, could you give this a more descriptive name, e.g.
"OnProfileCreated"?

https://codereview.chromium.org/303233004/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/303233004/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.cc:2076: // This is cleared after reboot.
nit: One line?

[Alexander Alekseev, 2014-05-30 18:44:39]

On 2014/05/30 06:57:44, Nikita Kostylev wrote:
> So essentiall on boot you're now writing both logout-started and
> login-prompt-visible?
> 
> Where's the guarantee that former will be written before latter?

There is no guarantee, but writing of "logout-started" starts early enough,
before UI data is read from files.
So I think it should reasonably finish writing before login prompt is visible
;-)

As we've discussed, we cannot delay "login-prompt-visible" event, so let it be
in a "best effort" way ;-)

[Alexander Alekseev, 2014-05-30 18:44:54]

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.cc (right):

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:345: return uptime + '^' + disk;
On 2014/05/30 07:07:03, Nikita Kostylev wrote:
> nit: Please extract this divider as a constant.

Done.

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.h (right):

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.h:87: void OnBoot();
On 2014/05/30 07:07:03, Nikita Kostylev wrote:
> nit: Comment is missing.

Done.

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.h:89: void
OnLogoutStarted(PrefService* state);
On 2014/05/30 07:07:03, Nikita Kostylev wrote:
> nit: Comment is missing.

Done.

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.h:150: static void
ClearLogoutStartedLastPreference();
On 2014/05/30 07:07:03, Nikita Kostylev wrote:
> nit: Comment is missing.

Done.

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:616:
BootTimesLoader::Get()->OnBoot();
On 2014/05/30 07:07:03, Nikita Kostylev wrote:
> nit: empty line after that line.

Done.

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/preferences.cc (right):

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/preferences.cc:86:
registry->RegisterStringPref(prefs::kLogoutStartedLast, "");
On 2014/05/30 07:07:03, Nikita Kostylev wrote:
> nit: std::string()

Done.

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/lifetime/...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/303233004/diff/20001/chrome/browser/lifetime/...
chrome/browser/lifetime/application_lifetime.cc:150: // Login screen should show
up in owner's locale.
On 2014/05/30 07:07:03, Nikita Kostylev wrote:
> nit: insert empty line before comment.

Done.

[Alexander Alekseev, 2014-05-30 21:12:45]

Steven, please review again.

https://codereview.chromium.org/303233004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.cc (right):

https://codereview.chromium.org/303233004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:359: }
On 2014/05/30 16:20:23, stevenjb wrote:
> While on the one hand this is fairly trivial, if we ever expand Stats it will
be
> a pain to maintain. Maybe consider using DictionaryValue and [de]serializing
> that as a JSON string.

Done.

https://codereview.chromium.org/303233004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:376: const size_t space_at =
uptime.find_first_of(' ');
On 2014/05/30 16:20:23, stevenjb wrote:
> Where is this ' ' coming from?

/proc/uptime has two fields: uptime in seconds and cumulative idle time in
seconds. So here the first field is extracted.

https://codereview.chromium.org/303233004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:382: }
On 2014/05/30 16:20:23, stevenjb wrote:
> This seems like either it should be a member of Stats (which should really be
a
> class now, and could be forward declared in the header and defined in the .cc
> file). Or if we make Stats a DictionaryValue, this + [De]Serialize should just
> be local helper methods.


There is a member of type Stats, so I decided to leave declaration in header.
The rest is done.

https://codereview.chromium.org/303233004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.h (right):

https://codereview.chromium.org/303233004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.h:88: void OnBoot();
On 2014/05/30 16:20:23, stevenjb wrote:
> "Boot" is rather unspecific, could you give this a more descriptive name, e.g.
> "OnProfileCreated"?

There are many different OnProfileCreated events.
Let it be OnChromeProcessStart().

https://codereview.chromium.org/303233004/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/303233004/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.cc:2076: // This is cleared after reboot.
On 2014/05/30 16:20:23, stevenjb wrote:
> nit: One line?

I've updated comment, so now it doesn't fit into a single line.

[jochen (gone - plz use gerrit), 2014-06-02 07:06:01]

lgtm

[jochen (gone - plz use gerrit), 2014-06-02 07:06:34]

should the c/b/lifetime directory have an OWNERS file?

[stevenjb, 2014-06-02 15:30:30]

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.cc (right):

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:87: }
nit: {} unnecessary

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:140:
base::ThreadRestrictions::ScopedAllowIO allow_io;
We should avoid using ScopedAllowIO if at all possible. If unavoidable it should
be clearly commented.

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:155: return std::string();
nit: Should probably log an error here. 'result' will be empty so you can just
return it.

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:168: if (!value ||
!value->GetAsDictionary(&dictionary))
LOG(ERROR)

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:173: !dictionary->GetString(kDisk,
&result.disk_)) {
LOG(ERROR)

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.h (right):

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.h:146: bool uptimeD(double* result)
const;
It's not clear what the 'D' stands for (double?). Also this should be CamelCase,
e.g. UpdtimeDouble'.

[Alexander Alekseev, 2014-06-02 18:46:03]

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.cc (right):

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:87: }
On 2014/06/02 15:30:31, stevenjb wrote:
> nit: {} unnecessary

Done.

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:140:
base::ThreadRestrictions::ScopedAllowIO allow_io;
On 2014/06/02 15:30:31, stevenjb wrote:
> We should avoid using ScopedAllowIO if at all possible. If unavoidable it
should
> be clearly commented.

Done.

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:155: return std::string();
On 2014/06/02 15:30:31, stevenjb wrote:
> nit: Should probably log an error here. 'result' will be empty so you can just
> return it.

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:168: if (!value ||
!value->GetAsDictionary(&dictionary))
On 2014/06/02 15:30:31, stevenjb wrote:
> LOG(ERROR)

Done.

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:173: !dictionary->GetString(kDisk,
&result.disk_)) {
On 2014/06/02 15:30:31, stevenjb wrote:
> LOG(ERROR)

Done.

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.h (right):

https://codereview.chromium.org/303233004/diff/70008/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.h:146: bool uptimeD(double* result)
const;
On 2014/06/02 15:30:31, stevenjb wrote:
> It's not clear what the 'D' stands for (double?). Also this should be
CamelCase,
> e.g. UpdtimeDouble'.

Done.

[stevenjb, 2014-06-02 19:11:37]

lgtm

https://codereview.chromium.org/303233004/diff/90001/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.cc (right):

https://codereview.chromium.org/303233004/diff/90001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:158: return result;
nit: don't return here ('return result' occurs below which was what I meant to
suggest), or explicitly return std::string() as before if you prefer.

https://codereview.chromium.org/303233004/diff/90001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:184: return Stats();
nit: Here also, explicit return isn't necessary.

[Alexander Alekseev, 2014-06-02 19:18:44]

Ok, I'll commit this.

https://codereview.chromium.org/303233004/diff/90001/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.cc (right):

https://codereview.chromium.org/303233004/diff/90001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:158: return result;
On 2014/06/02 19:11:38, stevenjb wrote:
> nit: don't return here ('return result' occurs below which was what I meant to
> suggest), or explicitly return std::string() as before if you prefer.

Done.

https://codereview.chromium.org/303233004/diff/90001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:184: return Stats();
On 2014/06/02 19:11:38, stevenjb wrote:
> nit: Here also, explicit return isn't necessary.

It is dangerous, because if first call succeeds, but second fails, the returned
object will be half-initialized.

[Alexander Alekseev, 2014-06-02 19:18:53]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-06-02 19:20:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alemate@chromium.org/303233004/110001

[stevenjb, 2014-06-02 19:26:32]

https://codereview.chromium.org/303233004/diff/90001/chrome/browser/chromeos/...
File chrome/browser/chromeos/boot_times_loader.cc (right):

https://codereview.chromium.org/303233004/diff/90001/chrome/browser/chromeos/...
chrome/browser/chromeos/boot_times_loader.cc:184: return Stats();
On 2014/06/02 19:18:45, alemate wrote:
> On 2014/06/02 19:11:38, stevenjb wrote:
> > nit: Here also, explicit return isn't necessary.
> 
> It is dangerous, because if first call succeeds, but second fails, the
returned
> object will be half-initialized.
Good point, looks good.

[commit-bot: I haz the power, 2014-06-02 22:48:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-02 22:51:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-02 22:51:23]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[Alexander Alekseev, 2014-06-03 14:19:17]

maruel@: please review PRESUBMIT.py
We need IO exception here because only /proc is accessed in synchronous way.

[M-A Ruel, 2014-06-03 14:33:56]

rubberstamp lgtm. Avi added this check, I'm not invested in it.

[Alexander Alekseev, 2014-06-03 14:40:33]

On 2014/06/03 14:33:56, M-A Ruel wrote:
> rubberstamp lgtm. Avi added this check, I'm not invested in it.

avi@ , what do you think?

[Avi (use Gerrit), 2014-06-03 14:54:45]

On 2014/06/03 14:40:33, alemate wrote:
> On 2014/06/03 14:33:56, M-A Ruel wrote:
> > rubberstamp lgtm. Avi added this check, I'm not invested in it.
> 
> avi@ , what do you think?

I'm OK with it.

[Alexander Alekseev, 2014-06-03 15:13:30]

The CQ bit was checked by alemate@chromium.org

[commit-bot: I haz the power, 2014-06-03 15:14:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/alemate@chromium.org/303233004/130001

[commit-bot: I haz the power, 2014-06-03 19:11:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-03 22:20:22]

Change committed as 274649