Utilize ProfileInfoCache to support data type counts in profile deletion flow

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used. (Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy supervised users.
              b.: chrome.send('logRemoveUserWarningShown') is called more than once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if not normal shutdown).

BUG=501916, 502346
Note: it is a good idea to use a JSON formatter (e.g. http://www.jsoneditoronline.org/ ) to pretiffy "Local State" during testing.

TEST=Back up the file "Local State" before starting. You need to restore this file at the beginning of each test. The "Local State" file should not contain the string "stats_settings". If it contains the string, then you can manually delete the keys below (remember to validate the changed file using a JSON formatter):
        "stats_bookmarks": 15,
        "stats_browsing_history": 4,
        "stats_passwords": 0,
        "stats_settings": 121,
Note that the numbers can be different, only the keys should be the same.

TEST=Restore "Local State". Start Chromium. Then close Chromium. Inspect the new "Local State" file for the keys listed above. If possible check that the keys is under the correct profile.

TEST=Restore "Local State". Start Chromium. Then kill the Chromium process. Inspect the new "Local State" file for the keys listed above.

TEST=Restore "Local State". Start Chromium. Open two or more profiles. Close Chromium. Start Chromium again. Go to the user switching dialog by selecting the name in the right side of the title bar, press "Switch Person". Now click the top-right corner of a profile and press "Remove Person". The statistics of the profile should appear if and only if:
(1) The user is not a supervised user, and
(2) The profile has been open when you start Chromium in the first time.
Note that the statistics can be displayed in a few different ways, as stated in bug 501916.

TEST=Restore "Local State". Start Chromium. Then shut down the computer without Chromium closed. The statistics should not have been written in Local State yet. Now start Chromium again, wait for a minute, and reboot again. Now the statistics key should be in Local State. (You may get Local State updated after the second start of Chromium but before the reboot. This is also passing the test.)
Note: Do not shut down Chromium normally or just trying to kill Chromium process. These way will cause statistics to be written in Local State resulting in false passes. Anyway, if Local State gets the statistical keys before the second start of Chromium, you should consider that the test is not carried out correctly and restart the test.

Committed: https://crrev.com/8e1e1231e10afbfe816effeceeb4f10075e6ea09
Cr-Commit-Position: refs/heads/master@{#367568}

[lwchkg, 2015-10-30 18:26:41]

Description was changed from

==========
BUG=501916
==========

to

==========
Patch #1:

Currently only the following files do matter. Other changes will be either
commited in other CLs or deleted.
- chrome/browser/profiles/profile_statistics.cc
- chrome/browser/ui/webui/signin/user_manager_screen_handler.cc 


BUG=501916
==========

[lwchkg, 2015-10-30 18:26:42]

lwchkg@gmail.com changed reviewers:
+ anthonyvd@chromium.org, mlerman@chromium.org

[lwchkg, 2015-10-30 18:33:46]

Dear Anthony and Mike,

  PTAL. This is an unfinished draft, and the purpose is just to demonstrate how
my other CL works. Only the two files listed below do matter:

chrome/browser/profiles/profile_statistics.cc
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc 

  Anyway, I'll finish user_pod_row.js tomorrow, and hopefully add some call to
GetProfileStatistics in browser.cc. I need help understanding browser shutdown
behavior (e.g. pressing the shut down button in Windows/Linux/Mac, closing
chromium in task manager, etc.).

Regards,
WC Leung.

[lwchkg, 2015-11-01 18:12:28]

Description was changed from

==========
Patch #1:

Currently only the following files do matter. Other changes will be either
commited in other CLs or deleted.
- chrome/browser/profiles/profile_statistics.cc
- chrome/browser/ui/webui/signin/user_manager_screen_handler.cc 


BUG=501916
==========

to

==========
Patch #2:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).

user_pod_row.js, user_manager_screen_handler.cc:
  Reads statistics also from ProfileInfoCache.

browser.cc:
  Saves statistics to ProfileInfoCache when the last window of a profile is
closed.

BUG=501916
==========

[lwchkg, 2015-11-01 18:19:16]

Description was changed from

==========
Patch #2:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).

user_pod_row.js, user_manager_screen_handler.cc:
  Reads statistics also from ProfileInfoCache.

browser.cc:
  Saves statistics to ProfileInfoCache when the last window of a profile is
closed.

BUG=501916
==========

to

==========
Patch #2:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called multiple
times when the deletion warning is shown.)

browser.cc:
  Saves statistics to ProfileInfoCache when the last window of a profile is
closed.

TODO:
1. Devise a way to guarantee that statistics are saved to ProfileInfoCache.
2. Investigate shutdown behavior of different OSes. (Any cases that
Browser::~Browser is not run?)

BUG=501916
==========

[lwchkg, 2015-11-01 18:19:33]

Description was changed from

==========
Patch #2:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called multiple
times when the deletion warning is shown.)

browser.cc:
  Saves statistics to ProfileInfoCache when the last window of a profile is
closed.

TODO:
1. Devise a way to guarantee that statistics are saved to ProfileInfoCache.
2. Investigate shutdown behavior of different OSes. (Any cases that
Browser::~Browser is not run?)

BUG=501916
==========

to

==========
Patch #2:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

browser.cc:
  Saves statistics to ProfileInfoCache when the last window of a profile is
closed.

TODO:
1. Devise a way to guarantee that statistics are saved to ProfileInfoCache.
2. Investigate shutdown behavior of different OSes. (Any cases that
Browser::~Browser is not run?)

BUG=501916
==========

[lwchkg, 2015-11-01 18:22:37]

lwchkg@gmail.com changed reviewers:
+ achuith@chromium.org, pkasting@chromium.org, rogerta@chromium.org

[lwchkg, 2015-11-01 18:28:05]

Description was changed from

==========
Patch #2:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

browser.cc:
  Saves statistics to ProfileInfoCache when the last window of a profile is
closed.

TODO:
1. Devise a way to guarantee that statistics are saved to ProfileInfoCache.
2. Investigate shutdown behavior of different OSes. (Any cases that
Browser::~Browser is not run?)

BUG=501916
==========

to

==========
Patch #2:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

browser.cc:
  Saves statistics to ProfileInfoCache when the last window of a profile is
closed.

TODO:
1. Devise a way to guarantee that statistics are saved to ProfileInfoCache.
2. Investigate shutdown behavior of different OSes. (Any cases that
Browser::~Browser is not run?)

BUG=501916, 502346
==========

[lwchkg, 2015-11-01 18:28:42]

+Roger, +achuithb, +Peter Kasting

Dear all,

  This CL is a follow-up to utilize ProfileInfoCache to handle profile
statistics. The statistics are shown when we try to delete a user in the user
selection dialog. PTAL.

  Please note that profile_info_* and profile_attribute_* are still being
reviewed in https://codereview.chromium.org/1415223002/ .

Regards,
WC Leung.

[Peter Kasting, 2015-11-02 03:33:24]

When you list multiple reviewers, please say precisely what you would like each
to look at.

Note that in most cases, each file only needs one reviewer, or two if you're
having one person review a large portion of the change but need signoffs from
OWNERS for various files.

[Mike Lerman, 2015-11-02 15:38:42]

On 2015/11/02 03:33:24, Peter Kasting wrote:
> When you list multiple reviewers, please say precisely what you would like
each
> to look at.
> 
> Note that in most cases, each file only needs one reviewer, or two if you're
> having one person review a large portion of the change but need signoffs from
> OWNERS for various files.

lwchkg@, I suggest waiting until Anthony and I have started looking this over at
a high level before you get OWNERS to look at specific files; there still may be
significant changes at a high level - don't know since no one's looked this over
yet!

[lwchkg, 2015-11-02 17:37:18]

On 2015/11/02 15:38:42, Mike Lerman wrote:
> On 2015/11/02 03:33:24, Peter Kasting wrote:
> > When you list multiple reviewers, please say precisely what you would like
> each
> > to look at.
> > 
> > Note that in most cases, each file only needs one reviewer, or two if you're
> > having one person review a large portion of the change but need signoffs
from
> > OWNERS for various files.

Peter: I see. This CL is the second part of 501916 and I've just invited the old
reviewers, so they have already known what is happening. And I've also added you
because you own chrome/browser/ui/browser.cc. Sorry for forgetting to introduce
the situation to you. BTW, you may want to wait until Anthony, Mike and Roger
finished checking of most of the code. This may take a week or more, but I hope
that the whole thing can be finished on 13 Nov. (If you prefer, I can add you
later instead of now.)

> 
> lwchkg@, I suggest waiting until Anthony and I have started looking this over
at
> a high level before you get OWNERS to look at specific files; there still may
be
> significant changes at a high level - don't know since no one's looked this
over
> yet!

Agree and thanks for reminder! Just a question from an outsider point of view:
are Anthony, you, and Roger in the same office?

[Mike Lerman, 2015-11-03 13:49:25]

On 2015/11/02 17:37:18, lwchkg wrote:
> On 2015/11/02 15:38:42, Mike Lerman wrote:
> > On 2015/11/02 03:33:24, Peter Kasting wrote:
> > > When you list multiple reviewers, please say precisely what you would like
> > each
> > > to look at.
> > > 
> > > Note that in most cases, each file only needs one reviewer, or two if
you're
> > > having one person review a large portion of the change but need signoffs
> from
> > > OWNERS for various files.
> 
> Peter: I see. This CL is the second part of 501916 and I've just invited the
old
> reviewers, so they have already known what is happening. And I've also added
you
> because you own chrome/browser/ui/browser.cc. Sorry for forgetting to
introduce
> the situation to you. BTW, you may want to wait until Anthony, Mike and Roger
> finished checking of most of the code. This may take a week or more, but I
hope
> that the whole thing can be finished on 13 Nov. (If you prefer, I can add you
> later instead of now.)
> 
> > 
> > lwchkg@, I suggest waiting until Anthony and I have started looking this
over
> at
> > a high level before you get OWNERS to look at specific files; there still
may
> be
> > significant changes at a high level - don't know since no one's looked this
> over
> > yet!
> 
> Agree and thanks for reminder! Just a question from an outsider point of view:
> are Anthony, you, and Roger in the same office?

I used to work on the same team as Anthony and Roger and sat in the same office
as them; I moved recently to a new office and a different team, but still do
some code reviews on Chrome Profiles.

[Mike Lerman, 2015-11-03 20:50:42]

Hi,

A few comments here on the interactions with the ProfileInfoCache. Comments
about the implementation there are still in the base CL.

I left out user_pod_spec.js for now, mostly because I don't like reading JS ;)
I'll do it once we're closer to the final implementation and review.

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
File chrome/browser/profiles/profile_statistics.cc (right):

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_statistics.cc:192: ProfileInfoCache&
profile_info_cache =
const ProfileInfoCache& please

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_statistics.cc:194: if (&profile_info_cache) {
you can assume if you have a ProfileManager than you have a ProfileInfoCache. It
is returning an object ref, not a pointer, after all.

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_statistics.cc:293: GetProfileStatistics(profile,
base::Bind(&NoOp), nullptr);
I think you can just write profiles::ProfileCatgeoryStats() as the second bound
parameter; you don't need to explicitly define NoOp.

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:532: // Store statistics into ProfileInfoCache
nit: amend a period. Although this comment isn't necessary, as all those words
are in the method name.

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:533:
profiles::StoreProfileStatisticsToCache(profile_);
There's a better place for this hook. Put this in
ProfileManager::BrowserListObserver::OnBrowserRemoved
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...).

Advantages:
- it already deals with incognito profiles for you
- it only runs on desktop (non-mobile) platforms, which is the only places we'd
want it anyways

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:587:
profiles::GetProfileStatistics(
Only do this if the profile has open browser windows (use BrowserList and
BrowserFinder to determine). If I have an open profile* that has no browsers,
that will imply the data's already been saved to the ProfileInfoCache; no need
to regenerate the stats.

[lwchkg, 2015-11-04 00:11:07]

Mike: thanks for your feedback. I'll make another patch tonight (but feel free
to comment again if something is not right.)

Anyway, I've figured out what should be happening with
BrowserProcessImpl::EndSession().
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...
Hopefully I can add the statistics code tonight. BTW, looks like
rundown_counter->Post(...) has been put in a wrong place (note: it adds an
nonnestable task to the thread.) WDYT?

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
File chrome/browser/profiles/profile_statistics.cc (right):

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_statistics.cc:192: ProfileInfoCache&
profile_info_cache =
On 2015/11/03 20:50:42, Mike Lerman wrote:
> const ProfileInfoCache& please

Acknowledged.

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_statistics.cc:194: if (&profile_info_cache) {
On 2015/11/03 20:50:42, Mike Lerman wrote:
> you can assume if you have a ProfileManager than you have a ProfileInfoCache.
It
> is returning an object ref, not a pointer, after all.

You're right. Thanks.

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_statistics.cc:293: GetProfileStatistics(profile,
base::Bind(&NoOp), nullptr);
On 2015/11/03 20:50:42, Mike Lerman wrote:
> I think you can just write profiles::ProfileCatgeoryStats() as the second
bound
> parameter; you don't need to explicitly define NoOp.

Not sure about this. Will try tonight.

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:532: // Store statistics into ProfileInfoCache
On 2015/11/03 20:50:42, Mike Lerman wrote:
> nit: amend a period. Although this comment isn't necessary, as all those words
> are in the method name.

Acknowledged.

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:533:
profiles::StoreProfileStatisticsToCache(profile_);
On 2015/11/03 20:50:42, Mike Lerman wrote:
> There's a better place for this hook. Put this in
> ProfileManager::BrowserListObserver::OnBrowserRemoved
>
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...).
> 
> Advantages:
> - it already deals with incognito profiles for you
> - it only runs on desktop (non-mobile) platforms, which is the only places
we'd
> want it anyways

Sounds good! Will check tonight.

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:587:
profiles::GetProfileStatistics(
On 2015/11/03 20:50:42, Mike Lerman wrote:
> Only do this if the profile has open browser windows (use BrowserList and
> BrowserFinder to determine). If I have an open profile* that has no browsers,
> that will imply the data's already been saved to the ProfileInfoCache; no need
> to regenerate the stats.

Sounds good.

[lwchkg, 2015-11-05 22:21:30]

Respond to comments. (New patch uploaded, but no change to browser_impl.cc yet.)

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
File chrome/browser/profiles/profile_statistics.cc (right):

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_statistics.cc:192: ProfileInfoCache&
profile_info_cache =
On 2015/11/03 20:50:42, Mike Lerman wrote:
> const ProfileInfoCache& please

I've tried, but the compiler complains.

e:\chromium\src\chrome\browser\profiles\profile_statistics.cc(197) : error
C2662: 'bool ProfileInfoCache::GetProfileAttributesWithPath(const base::FilePath
&,ProfileAttributesEntry **)' : cannot convert 'this' pointer from 'const
ProfileInfoCache' to 'ProfileInfoCache &'
        Conversion loses qualifiers

Anyway, found that reference types cannot be CV-qualified. See
http://en.cppreference.com/w/cpp/language/reference

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_statistics.cc:293: GetProfileStatistics(profile,
base::Bind(&NoOp), nullptr);
On 2015/11/03 20:50:42, Mike Lerman wrote:
> I think you can just write profiles::ProfileCatgeoryStats() as the second
bound
> parameter; you don't need to explicitly define NoOp.

Tried but not succeeded. I've tried to make a lambda function, but it is no good
because lambdas are prvalue which cannot be bound.

Anyway I hate that explicit NoOp. Any ideas?

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:587:
profiles::GetProfileStatistics(
On 2015/11/04 00:11:07, lwchkg wrote:
> On 2015/11/03 20:50:42, Mike Lerman wrote:
> > Only do this if the profile has open browser windows (use BrowserList and
> > BrowserFinder to determine). If I have an open profile* that has no
browsers,
> > that will imply the data's already been saved to the ProfileInfoCache; no
need
> > to regenerate the stats.
> 
> Sounds good.

Update: the data's already been saved to ProfileInfoCache, but the data in
user_pod_row.js may be outdated. So we need to send the statistics from
ProfileInfoCache instead.

[lwchkg, 2015-11-05 23:32:19]

> Anyway, I've figured out what should be happening with
> BrowserProcessImpl::EndSession().
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...
> Hopefully I can add the statistics code tonight. BTW, looks like
> rundown_counter->Post(...) has been put in a wrong place (note: it adds an
> nonnestable task to the thread.) WDYT?
> 

Was too naive about EndSession. Tasks in the UI messageloop will never be run,
so async statistics query will not return.

If possible to get stats at all, there will be some complex changes to
profile_statistics.cc, so we'd better give up.

Instead, we can do the statistics when we open a SESSION_ENDED or CRASHED
profile. WDYT?

[lwchkg, 2015-11-08 15:38:42]

Description was changed from

==========
Patch #2:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

browser.cc:
  Saves statistics to ProfileInfoCache when the last window of a profile is
closed.

TODO:
1. Devise a way to guarantee that statistics are saved to ProfileInfoCache.
2. Investigate shutdown behavior of different OSes. (Any cases that
Browser::~Browser is not run?)

BUG=501916, 502346
==========

to

==========
Patch #5:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

TODO:
1. Devise a way to guarantee that statistics are saved to ProfileInfoCache.
2. Investigate shutdown behavior of different OSes. (Any cases that
Browser::~Browser is not run?)

BUG=501916, 502346
==========

[lwchkg, 2015-11-08 15:43:29]

Description was changed from

==========
Patch #5:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

TODO:
1. Devise a way to guarantee that statistics are saved to ProfileInfoCache.
2. Investigate shutdown behavior of different OSes. (Any cases that
Browser::~Browser is not run?)

BUG=501916, 502346
==========

to

==========
Patch #5:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

TODO:
1. Devise a way to guarantee that statistics are saved to ProfileInfoCache.
2. Investigate shutdown behavior of different OSes. (Any cases that
Browser::~Browser is not run?)

BUG=501916, 502346
==========

[lwchkg, 2015-11-08 15:45:58]

Patch #4 and #5 uploaded.

https://codereview.chromium.org/1428973003/diff/80001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1428973003/diff/80001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1060:
profiles::StoreProfileStatisticsToCache(profile);
This is causing Chromium to crash if the exit type is END_SESSION (but not
CRASHED). Any ideas why this is crashing?

P.S. The bad thing here is I don't get a stack trace even if the debug symbols
are on. Does Chromium crash without a stack trace in some specific situation?

[lwchkg, 2015-11-09 21:00:09]

https://codereview.chromium.org/1428973003/diff/80001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1428973003/diff/80001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1060:
profiles::StoreProfileStatisticsToCache(profile);
Found the reason of crash: attempting to access the root nodes in BookmarkModel
before the nodes are loaded, so BookmarkNode::is_url() crashed. Is it possible
to put the code somewhere else where initialization is guaranteed to be
finished?

[lwchkg, 2015-11-09 21:00:18]

lwchkg@gmail.com changed reviewers:
- pkasting@chromium.org

[Mike Lerman, 2015-11-10 15:45:44]

Sorry, was out sick for a few days.

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
File chrome/browser/profiles/profile_statistics.cc (right):

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_statistics.cc:192: ProfileInfoCache&
profile_info_cache =
On 2015/11/05 22:21:30, lwchkg wrote:
> On 2015/11/03 20:50:42, Mike Lerman wrote:
> > const ProfileInfoCache& please
> 
> I've tried, but the compiler complains.
> 
> e:\chromium\src\chrome\browser\profiles\profile_statistics.cc(197) : error
> C2662: 'bool ProfileInfoCache::GetProfileAttributesWithPath(const
base::FilePath
> &,ProfileAttributesEntry **)' : cannot convert 'this' pointer from 'const
> ProfileInfoCache' to 'ProfileInfoCache &'
>         Conversion loses qualifiers
> 
> Anyway, found that reference types cannot be CV-qualified. See
> http://en.cppreference.com/w/cpp/language/reference

Ah - getProfileAttributesWithPath isn't const. Sorry my bad.

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_statistics.cc:293: GetProfileStatistics(profile,
base::Bind(&NoOp), nullptr);
On 2015/11/05 22:21:30, lwchkg wrote:
> On 2015/11/03 20:50:42, Mike Lerman wrote:
> > I think you can just write profiles::ProfileCatgeoryStats() as the second
> bound
> > parameter; you don't need to explicitly define NoOp.
> 
> Tried but not succeeded. I've tried to make a lambda function, but it is no
good
> because lambdas are prvalue which cannot be bound.
> 
> Anyway I hate that explicit NoOp. Any ideas?

Aha! Found an example:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...

We call profiles::SwitchToProfile() in a bunch of places, but for the 4th
parameter we often provide a Noop() callback. There's example syntax for you.

https://codereview.chromium.org/1428973003/diff/80001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1428973003/diff/80001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1060:
profiles::StoreProfileStatisticsToCache(profile);
On 2015/11/09 21:00:09, lwchkg wrote:
> Found the reason of crash: attempting to access the root nodes in
BookmarkModel
> before the nodes are loaded, so BookmarkNode::is_url() crashed. Is it possible
> to put the code somewhere else where initialization is guaranteed to be
> finished?

Some of the services you're looking at may be initializing on their own threads.
The ProfileStatistics should be responsible for delaying capture of each
service's counts until that service is initialized.

[lwchkg, 2015-11-10 18:11:07]

Thanks for your help. A new patch will be coming in minutes. (Needs compiling
and testing with reboots. And recompiling after reboot takes longer. :-) )

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
File chrome/browser/profiles/profile_statistics.cc (right):

https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_statistics.cc:293: GetProfileStatistics(profile,
base::Bind(&NoOp), nullptr);
On 2015/11/10 15:45:44, Mike Lerman wrote:
> On 2015/11/05 22:21:30, lwchkg wrote:
> > On 2015/11/03 20:50:42, Mike Lerman wrote:
> > > I think you can just write profiles::ProfileCatgeoryStats() as the second
> > bound
> > > parameter; you don't need to explicitly define NoOp.
> > 
> > Tried but not succeeded. I've tried to make a lambda function, but it is no
> good
> > because lambdas are prvalue which cannot be bound.
> > 
> > Anyway I hate that explicit NoOp. Any ideas?
> 
> Aha! Found an example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
> 
> We call profiles::SwitchToProfile() in a bunch of places, but for the 4th
> parameter we often provide a Noop() callback. There's example syntax for you.

Thanks. It's working well!

https://codereview.chromium.org/1428973003/diff/80001/chrome/browser/profiles...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1428973003/diff/80001/chrome/browser/profiles...
chrome/browser/profiles/profile_manager.cc:1060:
profiles::StoreProfileStatisticsToCache(profile);
On 2015/11/10 15:45:44, Mike Lerman wrote:
> On 2015/11/09 21:00:09, lwchkg wrote:
> > Found the reason of crash: attempting to access the root nodes in
> BookmarkModel
> > before the nodes are loaded, so BookmarkNode::is_url() crashed. Is it
possible
> > to put the code somewhere else where initialization is guaranteed to be
> > finished?
> 
> Some of the services you're looking at may be initializing on their own
threads.
> The ProfileStatistics should be responsible for delaying capture of each
> service's counts until that service is initialized.

Thanks. I've checked the services, and it looks like only BookmarkModel is
affected. The bookmarks are loaded async if we are the earliest consumer of the
bookmarks.

[lwchkg, 2015-11-10 19:11:33]

Description was changed from

==========
Patch #5:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

TODO:
1. Devise a way to guarantee that statistics are saved to ProfileInfoCache.
2. Investigate shutdown behavior of different OSes. (Any cases that
Browser::~Browser is not run?)

BUG=501916, 502346
==========

to

==========
Patch #6:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

TODO:
1. Devise a way to guarantee that statistics are saved to ProfileInfoCache.
2. Investigate shutdown behavior of different OSes. (Any cases that
Browser::~Browser is not run?)

BUG=501916, 502346
==========

[lwchkg, 2015-11-10 19:13:41]

Description was changed from

==========
Patch #6:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

TODO:
1. Devise a way to guarantee that statistics are saved to ProfileInfoCache.
2. Investigate shutdown behavior of different OSes. (Any cases that
Browser::~Browser is not run?)

BUG=501916, 502346
==========

to

==========
Patch #6:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

BUG=501916, 502346
==========

[lwchkg, 2015-11-10 19:14:41]

Mike: take care! It's very inconvenient to be sick in front of your baby!

New patch uploaded.

[lwchkg, 2015-11-22 05:23:31]

Dear all,

  New patch uploaded. PTAL!

Regards,
WC Leung.

[Mike Lerman, 2015-11-26 15:06:25]

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1060: // Record statistics to
ProfileInfoCache if not a normal shutdown.
rephrase to
Record statistics to ProfileInfoCache if the last shutdown wasn't clean.

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1063:
profile->GetLastSessionExitType() != Profile::EXIT_NORMAL)
nit: use braces around the statement

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_statistics.cc (right):

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.cc:28: using
bookmarks::BookmarkModel;
ugh; please try not to use using statements.

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.cc:86: void PrepareCountBookmarks();
Perhaps call this WaitOrCountBookmarks()? The method doesn't really do any
preparation - it just waits.

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.cc:291: }
if there's no bookmark_model, what's the appropriate result?

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_statistics.h (right):

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.h:52: // information to
ProfileInfoCache.
nit: to the ProfileInfoCache

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.h:53: void
StoreProfileStatisticsToCache(Profile* profile);
Why is this method really needed? Can't I just call GetProfileStatistics and
pass null or placeholder values for the two other parameters?

This method seems odd from an API perspective. Just something to consider.

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:590: //
ProfileInfoCache are update. The statistics in ProfileInfoCache are
update -> up to date.

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:592: // However,
if a statistic failed in ProfileInfoCache, then the actual
I don't know what "statistic failed in ProfileInfoCache" means. Do you meant
"the statistics couldn't be retrieved form the ProfileInfoCache"?

[lwchkg, 2015-12-02 15:33:35]

Here's my reply, and I'll prepare another patch today.

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1060: // Record statistics to
ProfileInfoCache if not a normal shutdown.
On 2015/11/26 15:06:25, Mike Lerman wrote:
> rephrase to
> Record statistics to ProfileInfoCache if the last shutdown wasn't clean.

Yeah we need to make this comment clearer.

I've rather for "Record statistics to ProfileInfoCache if the last shutdown was
a result of a system shutdown or a crash."

Anyway, the words "wasn't clean" contradicts the code/comment in
browser_process_impl.cc (see the link below), so I've made the above suggestion.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro...

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1063:
profile->GetLastSessionExitType() != Profile::EXIT_NORMAL)
On 2015/11/26 15:06:25, Mike Lerman wrote:
> nit: use braces around the statement

Yeah it looks better with the braces.

Just something out-of-topic: do you know about any recent changes to Google C++
style about conditionals? I remember that single-line conditionals like
    if (x == kFoo) return new Foo();
were forbidden, but the read today says that it's allowed.

https://google.github.io/styleguide/cppguide.html#Conditionals

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_statistics.cc (right):

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.cc:28: using
bookmarks::BookmarkModel;
On 2015/11/26 15:06:25, Mike Lerman wrote:
> ugh; please try not to use using statements.

Acknowledged.

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.cc:86: void PrepareCountBookmarks();
On 2015/11/26 15:06:25, Mike Lerman wrote:
> Perhaps call this WaitOrCountBookmarks()? The method doesn't really do any
> preparation - it just waits.

Good idea. Thanks!

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.cc:291: }
On 2015/11/26 15:06:25, Mike Lerman wrote:
> if there's no bookmark_model, what's the appropriate result?

The statistics should fail. That's what StatisticsCallbackFailure is doing.

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_statistics.h (right):

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.h:52: // information to
ProfileInfoCache.
On 2015/11/26 15:06:25, Mike Lerman wrote:
> nit: to the ProfileInfoCache

Acknowledged.

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.h:53: void
StoreProfileStatisticsToCache(Profile* profile);
On 2015/11/26 15:06:25, Mike Lerman wrote:
> Why is this method really needed? Can't I just call GetProfileStatistics and
> pass null or placeholder values for the two other parameters?
> 
> This method seems odd from an API perspective. Just something to consider.

Calling GetProfileStatistics to store the statistics is of course possible, but
the name of that function looks confusing in the call site, as the intent of the
call is not to "get profile statistics". So I've made
StoreProfileStatisticsToCache to make the code more readable. WDYT?

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:590: //
ProfileInfoCache are update. The statistics in ProfileInfoCache are
On 2015/11/26 15:06:25, Mike Lerman wrote:
> update -> up to date.

Acknowledged with thanks.

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:592: // However,
if a statistic failed in ProfileInfoCache, then the actual
On 2015/11/26 15:06:25, Mike Lerman wrote:
> I don't know what "statistic failed in ProfileInfoCache" means. Do you meant
> "the statistics couldn't be retrieved form the ProfileInfoCache"?

It means that one or more statistics are missing in ProfileInfoCache. Not sure
whether it is exactly what you mean, but at least it is close.

The statistics are supposed to be stored in ProfileInfoCache before when we call
GetProfileStatistics. Normally all 4 stats are stored in ProfileInfoCache.
However, if an individual statistic fail in all calls to GetProfileStatistics
(in different times), then that statistic is missing in ProfileInfoCache. In
this case GetProfileStatisticsFromCache returns a failure for that statistic.

Anyway, I'll rephrase the comment in the next patch.

(Note: if GetProfileStatistics has a failure in one of the statistics, the old
value (if exists) in ProfileInfoCache is not modified/deleted.)

[lwchkg, 2015-12-02 15:35:41]

Here's my reply, and I'll prepare another patch today.

[lwchkg, 2015-12-02 17:17:12]

New patch uploaded. PTAL.

[Mike Lerman, 2015-12-07 16:21:21]

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1063:
profile->GetLastSessionExitType() != Profile::EXIT_NORMAL)
On 2015/12/02 15:33:34, lwchkg wrote:
> On 2015/11/26 15:06:25, Mike Lerman wrote:
> > nit: use braces around the statement
> 
> Yeah it looks better with the braces.
> 
> Just something out-of-topic: do you know about any recent changes to Google
C++
> style about conditionals? I remember that single-line conditionals like
>     if (x == kFoo) return new Foo();
> were forbidden, but the read today says that it's allowed.
> 
> https://google.github.io/styleguide/cppguide.html#Conditionals

I'm not aware of a change, but that doesn't mean one didn't happen.

The braces are required if either the statement or the condition are multi-line.

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_statistics.h (right):

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.h:53: void
StoreProfileStatisticsToCache(Profile* profile);
On 2015/12/02 15:33:34, lwchkg wrote:
> On 2015/11/26 15:06:25, Mike Lerman wrote:
> > Why is this method really needed? Can't I just call GetProfileStatistics and
> > pass null or placeholder values for the two other parameters?
> > 
> > This method seems odd from an API perspective. Just something to consider.
> 
> Calling GetProfileStatistics to store the statistics is of course possible,
but
> the name of that function looks confusing in the call site, as the intent of
the
> call is not to "get profile statistics". So I've made
> StoreProfileStatisticsToCache to make the code more readable. WDYT?

You could rename GetProfileStatistics to GatherProfileStatistics. Get really is
a bit of a misnomer since it doesn't return anything, at best it executes a
callback that gets passed the information.

https://codereview.chromium.org/1428973003/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_statistics.cc (right):

https://codereview.chromium.org/1428973003/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.cc:115: void
BookmarkNodeMoved(bookmarks::BookmarkModel* model,
nit: when more than 2 lines, one per line, please.

[lwchkg, 2015-12-09 03:39:41]

Please see another patch loaded.

BTW, I have something to ask outside of the patches.

1. I'm been involving in the Chromium project for a few months already, and want
to apply for the status of contributor. If I'm good enough now then I need your
nomination and secondments. Anyway, here is the search result of me
(lwchkg@gmail.com):

https://codereview.chromium.org/search?closed=1&owner=lwchkg%40gmail.com&revi...

2. Actually I join this project to find a chance to join Google as a software
engineer. Since I don't have a CS degree nor any IT-job experience, chances for
me to get an interview by Google careers is slim.

I'm quite confident in my potentials, so I joined the Chromium project, in order
to practice coding, and to get experience in a large-scale project. (Special
thanks to Michael for reviewing most of my code. I've learned a lot from you!)

Of course, it will be even better for me if I can get a referral from you.
(Anyway, you can know something about me in my "new" personal webpage in
https://lwchkg.github.io .) I'm looking forward for your kind replies to
lwchkg@gmail.com . Thanks a lot!

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1063:
profile->GetLastSessionExitType() != Profile::EXIT_NORMAL)
On 2015/12/07 16:21:20, Mike Lerman wrote:
> On 2015/12/02 15:33:34, lwchkg wrote:
> > On 2015/11/26 15:06:25, Mike Lerman wrote:
> > > nit: use braces around the statement
> > 
> > Yeah it looks better with the braces.
> > 
> > Just something out-of-topic: do you know about any recent changes to Google
> C++
> > style about conditionals? I remember that single-line conditionals like
> >     if (x == kFoo) return new Foo();
> > were forbidden, but the read today says that it's allowed.
> > 
> > https://google.github.io/styleguide/cppguide.html#Conditionals
> 
> I'm not aware of a change, but that doesn't mean one didn't happen.
> 
> The braces are required if either the statement or the condition are
multi-line.

I see. I can't read this message clearly with the current version of the c++
style guide. Maybe the guide needs some rephrasing.

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile_statistics.h (right):

https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.h:53: void
StoreProfileStatisticsToCache(Profile* profile);
On 2015/12/07 16:21:20, Mike Lerman wrote:
> You could rename GetProfileStatistics to GatherProfileStatistics. Get really
is
> a bit of a misnomer since it doesn't return anything, at best it executes a
> callback that gets passed the information.

Brilliant idea! Problem solved. And now I finally understand why Anthony say
that you're excellent in naming.

[Mike Lerman, 2015-12-14 16:22:19]

On 2015/12/09 03:39:41, lwchkg wrote:
> Please see another patch loaded.
> 
> BTW, I have something to ask outside of the patches.
> 
> 1. I'm been involving in the Chromium project for a few months already, and
want
> to apply for the status of contributor. If I'm good enough now then I need
your
> nomination and secondments. Anyway, here is the search result of me
> (mailto:lwchkg@gmail.com):
> 
>
https://codereview.chromium.org/search?closed=1&owner=lwchkg%40gmail.com&revi...
> 
> 2. Actually I join this project to find a chance to join Google as a software
> engineer. Since I don't have a CS degree nor any IT-job experience, chances
for
> me to get an interview by Google careers is slim.
> 
> I'm quite confident in my potentials, so I joined the Chromium project, in
order
> to practice coding, and to get experience in a large-scale project. (Special
> thanks to Michael for reviewing most of my code. I've learned a lot from you!)
> 
> Of course, it will be even better for me if I can get a referral from you.
> (Anyway, you can know something about me in my "new" personal webpage in
> https://lwchkg.github.io .) I'm looking forward for your kind replies to
> mailto:lwchkg@gmail.com . Thanks a lot!
> 
>
https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
> File chrome/browser/profiles/profile_manager.cc (right):
> 
>
https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
> chrome/browser/profiles/profile_manager.cc:1063:
> profile->GetLastSessionExitType() != Profile::EXIT_NORMAL)
> On 2015/12/07 16:21:20, Mike Lerman wrote:
> > On 2015/12/02 15:33:34, lwchkg wrote:
> > > On 2015/11/26 15:06:25, Mike Lerman wrote:
> > > > nit: use braces around the statement
> > > 
> > > Yeah it looks better with the braces.
> > > 
> > > Just something out-of-topic: do you know about any recent changes to
Google
> > C++
> > > style about conditionals? I remember that single-line conditionals like
> > >     if (x == kFoo) return new Foo();
> > > were forbidden, but the read today says that it's allowed.
> > > 
> > > https://google.github.io/styleguide/cppguide.html#Conditionals
> > 
> > I'm not aware of a change, but that doesn't mean one didn't happen.
> > 
> > The braces are required if either the statement or the condition are
> multi-line.
> 
> I see. I can't read this message clearly with the current version of the c++
> style guide. Maybe the guide needs some rephrasing.
> 
>
https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
> File chrome/browser/profiles/profile_statistics.h (right):
> 
>
https://codereview.chromium.org/1428973003/diff/120001/chrome/browser/profile...
> chrome/browser/profiles/profile_statistics.h:53: void
> StoreProfileStatisticsToCache(Profile* profile);
> On 2015/12/07 16:21:20, Mike Lerman wrote:
> > You could rename GetProfileStatistics to GatherProfileStatistics. Get really
> is
> > a bit of a misnomer since it doesn't return anything, at best it executes a
> > callback that gets passed the information.
> 
> Brilliant idea! Problem solved. And now I finally understand why Anthony say
> that you're excellent in naming.

These changes LGTM.

I'll respond to the personal comments privately via email.

[lwchkg, 2015-12-14 18:26:27]

The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-14 18:27:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428973003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428973003/160001

[commit-bot: I haz the power, 2015-12-14 19:00:56]

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

[commit-bot: I haz the power, 2015-12-14 19:00:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lwchkg, 2015-12-22 08:18:38]

The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-22 08:18:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428973003/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428973003/180001

[commit-bot: I haz the power, 2015-12-22 08:26:13]

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

[commit-bot: I haz the power, 2015-12-22 08:26:14]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lwchkg, 2015-12-22 11:22:53]

The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-22 11:23:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428973003/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428973003/200001

[commit-bot: I haz the power, 2015-12-22 12:12:10]

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

[commit-bot: I haz the power, 2015-12-22 12:12:11]

Dry run: This issue passed the CQ dry run.

[lwchkg, 2015-12-22 13:26:00]

Dear all,

  I've fixed a few bugs in the newest patch, and therefore needs your review
again. Thanks! (Please check the newest code against patch #9, that Mike have
LGTMed before.)

  And please also answer my questions written in the code. I need your feedback
to get the code finalized.

Regards,
WC Leung.

https://codereview.chromium.org/1428973003/diff/180001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right):

https://codereview.chromium.org/1428973003/diff/180001/chrome/browser/ui/webu...
chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:602:
return_value.SetWithoutPathExpansion(item.category, stat.Pass());
Will change from stat.Pass() to std::move(stat)
See crbug.com/557422 for details.

The same applies to all changes below.

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1485: if (!profile->IsOffTheRecord())
{
Mike said that this function handles incognito profiles for me, but in fact it
doesn't. Is this a bug? (To all: please comment if you know this.)

c.f.
https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/brows...

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_statistics.cc (right):

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.cc:343: LOG(WARNING) <<
The code here is just a draft. I need your comments to finalize it. Two
questions:
(1) should I use CHECK or LOG for this check?
(2) what exact conditions should I check against? Is that the code below, or
just a subset of it?
!profile->IsGuestSession() && !profile->IsSystemProfile() &&
!profile->IsNewProfile() && !profile->IsOffTheRecord()

[Mike Lerman, 2015-12-23 20:25:50]

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1485: if (!profile->IsOffTheRecord())
{
On 2015/12/22 13:26:00, lwchkg wrote:
> Mike said that this function handles incognito profiles for me, but in fact it
> doesn't. Is this a bug? (To all: please comment if you know this.)
> 
> c.f.
>
https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/brows...

This, uh, really should handle incognito (and guest) profiles.

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_statistics.cc (right):

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.cc:343: LOG(WARNING) <<
On 2015/12/22 13:26:00, lwchkg wrote:
> The code here is just a draft. I need your comments to finalize it. Two
> questions:
> (1) should I use CHECK or LOG for this check?
> (2) what exact conditions should I check against? Is that the code below, or
> just a subset of it?
> !profile->IsGuestSession() && !profile->IsSystemProfile() &&
> !profile->IsNewProfile() && !profile->IsOffTheRecord()

(1)
Definitely not CHECK(). This isn't worth crashing over in the field, which
CHECK() will do. Similarly, I don't think LOG is necessary, as we don't need
in-the-field logging over this.

Will this crash if called for an incognito profile? If it will crash later, then
I'm all for a DCHECK, which will help diagnose issues down the road. If it won't
crash later, then I think a DLOG is fine.

(2)
First off, IsOffTheRecord() will be true whenever IsGuestSession() is true, so
there's no need to get IsGuestSession() separately.
Second, I think we just need to check IsOffTheRecord and IsSystemProfile. While
IsNewProfile isn't something we want to have happen (from the ProfileManager),
there's nothing innately wrong with doing so. But you can include it if you feel
more comfortable.

[lwchkg, 2015-12-24 07:14:32]

Thanks Mike. It's likely to be two hidden bugs, so I'll investigate them.
Anyway, I'll land this CL by Jan 15, with the workarounds and their respective
comments if necessary.

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1485: if (!profile->IsOffTheRecord())
{
On 2015/12/23 20:25:50, Mike Lerman wrote:
> On 2015/12/22 13:26:00, lwchkg wrote:
> > Mike said that this function handles incognito profiles for me, but in fact
it
> > doesn't. Is this a bug? (To all: please comment if you know this.)
> > 
> > c.f.
> >
>
https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/brows...
> 
> This, uh, really should handle incognito (and guest) profiles. 

Thanks. This is likely a bug behind. Maybe someone changed the behavior of the
observer. I'll investigate into this issue.

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_statistics.cc (right):

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.cc:284:
BookmarkModelFactory::GetForProfileIfExists(profile_);
The crash happens here inside GetForProfileIfExists(profile_). I guess it is a
bug anyway, so I'll investigate and sumbit another CL if appropriate. WDYT?

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.cc:343: LOG(WARNING) <<
On 2015/12/23 20:25:50, Mike Lerman wrote:
> On 2015/12/22 13:26:00, lwchkg wrote:
> > The code here is just a draft. I need your comments to finalize it. Two
> > questions:
> > (1) should I use CHECK or LOG for this check?
> > (2) what exact conditions should I check against? Is that the code below, or
> > just a subset of it?
> > !profile->IsGuestSession() && !profile->IsSystemProfile() &&
> > !profile->IsNewProfile() && !profile->IsOffTheRecord()
> 
> (1)
> Definitely not CHECK(). This isn't worth crashing over in the field, which
> CHECK() will do. Similarly, I don't think LOG is necessary, as we don't need
> in-the-field logging over this.
> 
> Will this crash if called for an incognito profile? If it will crash later,
then
> I'm all for a DCHECK, which will help diagnose issues down the road. If it
won't
> crash later, then I think a DLOG is fine.
> 

DCHECK and DLOG won't help here because the crash happens in browser tests.

Anyway, I don't understand why incognito profiles does crash, which shouldn't. I
expect some or all statistics fail though. Most likely the check is not
necessary after the bug is fixed. WDYT?

> (2)
> First off, IsOffTheRecord() will be true whenever IsGuestSession() is true, so
> there's no need to get IsGuestSession() separately.
> Second, I think we just need to check IsOffTheRecord and IsSystemProfile.
While
> IsNewProfile isn't something we want to have happen (from the ProfileManager),
> there's nothing innately wrong with doing so. But you can include it if you
feel
> more comfortable.

Thanks. Then IsOffTheRecord and IsSystemProfile is what we want (if the check is
needed at all.)

[lwchkg, 2015-12-28 13:52:52]

Description was changed from

==========
Patch #6:

profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

BUG=501916, 502346
==========

to

==========
profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

BUG=501916, 502346
==========

[lwchkg, 2015-12-28 15:11:21]

Description was changed from

==========
profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

BUG=501916, 502346
==========

to

==========
profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

BUG=501916, 502346

Note: it is a good idea to use a JSON formatter (e.g.
http://www.jsoneditoronline.org/ ) to pretiffy "Local State" during testing.

TEST=Back up the file "Local State" before starting. You need to restore this
file at the beginning of each test. The "Local State" file should not contain
the string "stats_settings". If it contains the string, then you can manually
delete the keys below (remember to validate the changed file using a JSON
formatter):
        "stats_bookmarks": 15,
        "stats_browsing_history": 4,
        "stats_passwords": 0,
        "stats_settings": 121,
Note that the numbers can be something else, only the keys should be there. If
possible check that the key is under the correct profile.

TEST=Restore "Local State". Start Chromium. Then close Chromium. Inspect the new
"Local State" file for the keys listed above.

TEST=Restore "Local State". Start Chromium. Open two or more profiles. Close
Chromium. Start Chromium again. Go to the user switching dialog by selecting the
name in the right side of the title bar, press "Switch Person". Now click the
top-right corner of a profile and press "Remove Person". The statistics of the
profile should appear if and only if:
(1) The user is not a supervised user, and
(2) The profile has been open when you start Chromium in the first time.
Note that the statistics can be displayed in a few different ways, as stated in
bug 501916.

TEST=Restore "Local State". Start Chromium. Then shut down the computer without
Chromium closed. The statistics should not have been written in Local State yet.
Now start Chromium again, wait for a minute, and reboot again. Now the
statistics key should be in Local State.
subject: Utilize ProfileInfoCache to support data type counts in profile
deletion flow
==========

[lwchkg, 2015-12-28 15:12:53]

Description was changed from

==========
profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

BUG=501916, 502346

Note: it is a good idea to use a JSON formatter (e.g.
http://www.jsoneditoronline.org/ ) to pretiffy "Local State" during testing.

TEST=Back up the file "Local State" before starting. You need to restore this
file at the beginning of each test. The "Local State" file should not contain
the string "stats_settings". If it contains the string, then you can manually
delete the keys below (remember to validate the changed file using a JSON
formatter):
        "stats_bookmarks": 15,
        "stats_browsing_history": 4,
        "stats_passwords": 0,
        "stats_settings": 121,
Note that the numbers can be something else, only the keys should be there. If
possible check that the key is under the correct profile.

TEST=Restore "Local State". Start Chromium. Then close Chromium. Inspect the new
"Local State" file for the keys listed above.

TEST=Restore "Local State". Start Chromium. Open two or more profiles. Close
Chromium. Start Chromium again. Go to the user switching dialog by selecting the
name in the right side of the title bar, press "Switch Person". Now click the
top-right corner of a profile and press "Remove Person". The statistics of the
profile should appear if and only if:
(1) The user is not a supervised user, and
(2) The profile has been open when you start Chromium in the first time.
Note that the statistics can be displayed in a few different ways, as stated in
bug 501916.

TEST=Restore "Local State". Start Chromium. Then shut down the computer without
Chromium closed. The statistics should not have been written in Local State yet.
Now start Chromium again, wait for a minute, and reboot again. Now the
statistics key should be in Local State.
subject: Utilize ProfileInfoCache to support data type counts in profile
deletion flow
==========

to

==========
profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

BUG=501916, 502346

Note: it is a good idea to use a JSON formatter (e.g.
http://www.jsoneditoronline.org/ ) to pretiffy "Local State" during testing.

TEST=Back up the file "Local State" before starting. You need to restore this
file at the beginning of each test. The "Local State" file should not contain
the string "stats_settings". If it contains the string, then you can manually
delete the keys below (remember to validate the changed file using a JSON
formatter):
        "stats_bookmarks": 15,
        "stats_browsing_history": 4,
        "stats_passwords": 0,
        "stats_settings": 121,
Note that the numbers can be different, only the keys should be the same.

TEST=Restore "Local State". Start Chromium. Then close Chromium. Inspect the new
"Local State" file for the keys listed above. If possible check that the keys is
under the correct profile.

TEST=Restore "Local State". Start Chromium. Open two or more profiles. Close
Chromium. Start Chromium again. Go to the user switching dialog by selecting the
name in the right side of the title bar, press "Switch Person". Now click the
top-right corner of a profile and press "Remove Person". The statistics of the
profile should appear if and only if:
(1) The user is not a supervised user, and
(2) The profile has been open when you start Chromium in the first time.
Note that the statistics can be displayed in a few different ways, as stated in
bug 501916.

TEST=Restore "Local State". Start Chromium. Then shut down the computer without
Chromium closed. The statistics should not have been written in Local State yet.
Now start Chromium again, wait for a minute, and reboot again. Now the
statistics key should be in Local State.
==========

[lwchkg, 2015-12-28 15:13:33]

The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-28 15:13:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428973003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428973003/220001

[commit-bot: I haz the power, 2015-12-28 15:14:56]

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

[commit-bot: I haz the power, 2015-12-28 15:14:57]

Dry run: Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lwchkg, 2015-12-28 16:24:23]

The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2015-12-28 16:24:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428973003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428973003/240001

[commit-bot: I haz the power, 2015-12-28 17:14:42]

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

[commit-bot: I haz the power, 2015-12-28 17:14:43]

Dry run: This issue passed the CQ dry run.

[lwchkg, 2015-12-29 04:32:20]

Description was changed from

==========
profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

BUG=501916, 502346

Note: it is a good idea to use a JSON formatter (e.g.
http://www.jsoneditoronline.org/ ) to pretiffy "Local State" during testing.

TEST=Back up the file "Local State" before starting. You need to restore this
file at the beginning of each test. The "Local State" file should not contain
the string "stats_settings". If it contains the string, then you can manually
delete the keys below (remember to validate the changed file using a JSON
formatter):
        "stats_bookmarks": 15,
        "stats_browsing_history": 4,
        "stats_passwords": 0,
        "stats_settings": 121,
Note that the numbers can be different, only the keys should be the same.

TEST=Restore "Local State". Start Chromium. Then close Chromium. Inspect the new
"Local State" file for the keys listed above. If possible check that the keys is
under the correct profile.

TEST=Restore "Local State". Start Chromium. Open two or more profiles. Close
Chromium. Start Chromium again. Go to the user switching dialog by selecting the
name in the right side of the title bar, press "Switch Person". Now click the
top-right corner of a profile and press "Remove Person". The statistics of the
profile should appear if and only if:
(1) The user is not a supervised user, and
(2) The profile has been open when you start Chromium in the first time.
Note that the statistics can be displayed in a few different ways, as stated in
bug 501916.

TEST=Restore "Local State". Start Chromium. Then shut down the computer without
Chromium closed. The statistics should not have been written in Local State yet.
Now start Chromium again, wait for a minute, and reboot again. Now the
statistics key should be in Local State.
==========

to

==========
profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

BUG=501916, 502346

Note: it is a good idea to use a JSON formatter (e.g.
http://www.jsoneditoronline.org/ ) to pretiffy "Local State" during testing.

TEST=Back up the file "Local State" before starting. You need to restore this
file at the beginning of each test. The "Local State" file should not contain
the string "stats_settings". If it contains the string, then you can manually
delete the keys below (remember to validate the changed file using a JSON
formatter):
        "stats_bookmarks": 15,
        "stats_browsing_history": 4,
        "stats_passwords": 0,
        "stats_settings": 121,
Note that the numbers can be different, only the keys should be the same.

TEST=Restore "Local State". Start Chromium. Then close Chromium. Inspect the new
"Local State" file for the keys listed above. If possible check that the keys is
under the correct profile.

TEST=Restore "Local State". Start Chromium. Then kill the Chromium process.
Inspect the new "Local State" file for the keys listed above.

TEST=Restore "Local State". Start Chromium. Open two or more profiles. Close
Chromium. Start Chromium again. Go to the user switching dialog by selecting the
name in the right side of the title bar, press "Switch Person". Now click the
top-right corner of a profile and press "Remove Person". The statistics of the
profile should appear if and only if:
(1) The user is not a supervised user, and
(2) The profile has been open when you start Chromium in the first time.
Note that the statistics can be displayed in a few different ways, as stated in
bug 501916.

TEST=Restore "Local State". Start Chromium. Then shut down the computer without
Chromium closed. The statistics should not have been written in Local State yet.
Now start Chromium again, wait for a minute, and reboot again. Now the
statistics key should be in Local State. (You may get Local State updated after
the second start of Chromium but before the reboot. This is also passing the
test.)
Note: Do not shut down Chromium normally or just trying to kill Chromium
process. These way will cause statistics to be written in Local State resulting
in false passes. Anyway, if Local State gets the statistical keys before the
second start of Chromium, you should consider that the test is not carried out
correctly and restart the test.
==========

[lwchkg, 2015-12-29 04:51:43]

Uploaded a new patch. This should be the last patch if no more nits are found.

achuith@ : PTAL! The code you're reviewing is *.js, while the rest of the code
has been reviewed by Mike. Thanks.

Mike : You may want to have a delta against patch #11 (the one that you have
commented last time).

Regards,
WC Leung.

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1475: // do that now.
I don't really understand the comment above. The code below reads "if the last
window of a ephemeral profile is closed, remove the ephemeral profile from
disk." I'd rather updating the comment myself. WDYT?

Anyway, the original code review is here:
https://codereview.chromium.org/25495003

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_manager.cc:1485: if (!profile->IsOffTheRecord())
{
On 2015/12/24 07:14:32, lwchkg wrote:
> On 2015/12/23 20:25:50, Mike Lerman wrote:
> > On 2015/12/22 13:26:00, lwchkg wrote:
> > > Mike said that this function handles incognito profiles for me, but in
fact
> it
> > > doesn't. Is this a bug? (To all: please comment if you know this.)
> > > 
> > > c.f.
> > >
> >
>
https://codereview.chromium.org/1428973003/diff/20001/chrome/browser/ui/brows...
> > 
> > This, uh, really should handle incognito (and guest) profiles. 
> 
> Thanks. This is likely a bug behind. Maybe someone changed the behavior of the
> observer. I'll investigate into this issue.

(1) Investigated the code about the BrowserListObserver. It is added to the
observer list at the creation of ProfileManager and removed when ProfileManager
is deleted. And it doesn't handling incognito profiles for me AFAIK, so I need
to handle myself.

(2) Anyway, the way of handling incognito profiles is wrong here. I'll update
the code in the next patch.

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
File chrome/browser/profiles/profile_statistics.cc (right):

https://codereview.chromium.org/1428973003/diff/200001/chrome/browser/profile...
chrome/browser/profiles/profile_statistics.cc:343: LOG(WARNING) <<
On 2015/12/24 07:14:32, lwchkg wrote:
> On 2015/12/23 20:25:50, Mike Lerman wrote:
> > On 2015/12/22 13:26:00, lwchkg wrote:
> > > The code here is just a draft. I need your comments to finalize it. Two
> > > questions:
> > > (1) should I use CHECK or LOG for this check?
> > > (2) what exact conditions should I check against? Is that the code below,
or
> > > just a subset of it?
> > > !profile->IsGuestSession() && !profile->IsSystemProfile() &&
> > > !profile->IsNewProfile() && !profile->IsOffTheRecord()
> > 
> > (1)
> > Definitely not CHECK(). This isn't worth crashing over in the field, which
> > CHECK() will do. Similarly, I don't think LOG is necessary, as we don't need
> > in-the-field logging over this.
> > 
> > Will this crash if called for an incognito profile? If it will crash later,
> then
> > I'm all for a DCHECK, which will help diagnose issues down the road. If it
> won't
> > crash later, then I think a DLOG is fine.
> > 
> 
> DCHECK and DLOG won't help here because the crash happens in browser tests.
> 
> Anyway, I don't understand why incognito profiles does crash, which shouldn't.
I
> expect some or all statistics fail though. Most likely the check is not
> necessary after the bug is fixed. WDYT?
> 
> > (2)
> > First off, IsOffTheRecord() will be true whenever IsGuestSession() is true,
so
> > there's no need to get IsGuestSession() separately.
> > Second, I think we just need to check IsOffTheRecord and IsSystemProfile.
> While
> > IsNewProfile isn't something we want to have happen (from the
ProfileManager),
> > there's nothing innately wrong with doing so. But you can include it if you
> feel
> > more comfortable.
> 
> Thanks. Then IsOffTheRecord and IsSystemProfile is what we want (if the check
is
> needed at all.)

Now I understand that incognito profiles crashed.

The reason is as follows: in browser tests incognito profiles get deallocated
before the profile statistics tasks get executed. So crash happens due to
dangling reference. After a few hours of search I've found that incognito
profiles are deallocated by the following code:
 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

The good news is no bug is found.

[achuithb, 2016-01-04 18:45:29]

user_pod_row.js lgtm

https://codereview.chromium.org/1428973003/diff/240001/ui/login/account_picke...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/1428973003/diff/240001/ui/login/account_picke...
ui/login/account_picker/user_pod_row.js:979: * Get the elements used for
statistics display.
s/Get/Gets

[lwchkg, 2016-01-05 16:57:21]

Thanks everyone. Will commit now (and send the draft comment). Browser tests
will be added in a follow-up CL.

https://codereview.chromium.org/1428973003/diff/240001/ui/login/account_picke...
File ui/login/account_picker/user_pod_row.js (right):

https://codereview.chromium.org/1428973003/diff/240001/ui/login/account_picke...
ui/login/account_picker/user_pod_row.js:979: * Get the elements used for
statistics display.
On 2016/01/04 18:45:29, achuithb wrote:
> s/Get/Gets

Done.

[lwchkg, 2016-01-05 16:57:31]

The CQ bit was checked by lwchkg@gmail.com

[lwchkg, 2016-01-05 16:57:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/1428973003/#ps260001
(title: "Respond to comments, minor edits")

[commit-bot: I haz the power, 2016-01-05 16:58:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428973003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428973003/260001

[commit-bot: I haz the power, 2016-01-05 17:54:11]

Description was changed from

==========
profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

BUG=501916, 502346

Note: it is a good idea to use a JSON formatter (e.g.
http://www.jsoneditoronline.org/ ) to pretiffy "Local State" during testing.

TEST=Back up the file "Local State" before starting. You need to restore this
file at the beginning of each test. The "Local State" file should not contain
the string "stats_settings". If it contains the string, then you can manually
delete the keys below (remember to validate the changed file using a JSON
formatter):
        "stats_bookmarks": 15,
        "stats_browsing_history": 4,
        "stats_passwords": 0,
        "stats_settings": 121,
Note that the numbers can be different, only the keys should be the same.

TEST=Restore "Local State". Start Chromium. Then close Chromium. Inspect the new
"Local State" file for the keys listed above. If possible check that the keys is
under the correct profile.

TEST=Restore "Local State". Start Chromium. Then kill the Chromium process.
Inspect the new "Local State" file for the keys listed above.

TEST=Restore "Local State". Start Chromium. Open two or more profiles. Close
Chromium. Start Chromium again. Go to the user switching dialog by selecting the
name in the right side of the title bar, press "Switch Person". Now click the
top-right corner of a profile and press "Remove Person". The statistics of the
profile should appear if and only if:
(1) The user is not a supervised user, and
(2) The profile has been open when you start Chromium in the first time.
Note that the statistics can be displayed in a few different ways, as stated in
bug 501916.

TEST=Restore "Local State". Start Chromium. Then shut down the computer without
Chromium closed. The statistics should not have been written in Local State yet.
Now start Chromium again, wait for a minute, and reboot again. Now the
statistics key should be in Local State. (You may get Local State updated after
the second start of Chromium but before the reboot. This is also passing the
test.)
Note: Do not shut down Chromium normally or just trying to kill Chromium
process. These way will cause statistics to be written in Local State resulting
in false passes. Anyway, if Local State gets the statistical keys before the
second start of Chromium, you should consider that the test is not carried out
correctly and restart the test.
==========

to

==========
profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

BUG=501916, 502346

Note: it is a good idea to use a JSON formatter (e.g.
http://www.jsoneditoronline.org/ ) to pretiffy "Local State" during testing.

TEST=Back up the file "Local State" before starting. You need to restore this
file at the beginning of each test. The "Local State" file should not contain
the string "stats_settings". If it contains the string, then you can manually
delete the keys below (remember to validate the changed file using a JSON
formatter):
        "stats_bookmarks": 15,
        "stats_browsing_history": 4,
        "stats_passwords": 0,
        "stats_settings": 121,
Note that the numbers can be different, only the keys should be the same.

TEST=Restore "Local State". Start Chromium. Then close Chromium. Inspect the new
"Local State" file for the keys listed above. If possible check that the keys is
under the correct profile.

TEST=Restore "Local State". Start Chromium. Then kill the Chromium process.
Inspect the new "Local State" file for the keys listed above.

TEST=Restore "Local State". Start Chromium. Open two or more profiles. Close
Chromium. Start Chromium again. Go to the user switching dialog by selecting the
name in the right side of the title bar, press "Switch Person". Now click the
top-right corner of a profile and press "Remove Person". The statistics of the
profile should appear if and only if:
(1) The user is not a supervised user, and
(2) The profile has been open when you start Chromium in the first time.
Note that the statistics can be displayed in a few different ways, as stated in
bug 501916.

TEST=Restore "Local State". Start Chromium. Then shut down the computer without
Chromium closed. The statistics should not have been written in Local State yet.
Now start Chromium again, wait for a minute, and reboot again. Now the
statistics key should be in Local State. (You may get Local State updated after
the second start of Chromium but before the reboot. This is also passing the
test.)
Note: Do not shut down Chromium normally or just trying to kill Chromium
process. These way will cause statistics to be written in Local State resulting
in false passes. Anyway, if Local State gets the statistical keys before the
second start of Chromium, you should consider that the test is not carried out
correctly and restart the test.
==========

[commit-bot: I haz the power, 2016-01-05 17:54:12]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-01-05 17:55:20]

Description was changed from

==========
profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users. 
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

BUG=501916, 502346

Note: it is a good idea to use a JSON formatter (e.g.
http://www.jsoneditoronline.org/ ) to pretiffy "Local State" during testing.

TEST=Back up the file "Local State" before starting. You need to restore this
file at the beginning of each test. The "Local State" file should not contain
the string "stats_settings". If it contains the string, then you can manually
delete the keys below (remember to validate the changed file using a JSON
formatter):
        "stats_bookmarks": 15,
        "stats_browsing_history": 4,
        "stats_passwords": 0,
        "stats_settings": 121,
Note that the numbers can be different, only the keys should be the same.

TEST=Restore "Local State". Start Chromium. Then close Chromium. Inspect the new
"Local State" file for the keys listed above. If possible check that the keys is
under the correct profile.

TEST=Restore "Local State". Start Chromium. Then kill the Chromium process.
Inspect the new "Local State" file for the keys listed above.

TEST=Restore "Local State". Start Chromium. Open two or more profiles. Close
Chromium. Start Chromium again. Go to the user switching dialog by selecting the
name in the right side of the title bar, press "Switch Person". Now click the
top-right corner of a profile and press "Remove Person". The statistics of the
profile should appear if and only if:
(1) The user is not a supervised user, and
(2) The profile has been open when you start Chromium in the first time.
Note that the statistics can be displayed in a few different ways, as stated in
bug 501916.

TEST=Restore "Local State". Start Chromium. Then shut down the computer without
Chromium closed. The statistics should not have been written in Local State yet.
Now start Chromium again, wait for a minute, and reboot again. Now the
statistics key should be in Local State. (You may get Local State updated after
the second start of Chromium but before the reboot. This is also passing the
test.)
Note: Do not shut down Chromium normally or just trying to kill Chromium
process. These way will cause statistics to be written in Local State resulting
in false passes. Anyway, if Local State gets the statistical keys before the
second start of Chromium, you should consider that the test is not carried out
correctly and restart the test.
==========

to

==========
profile_statistics.*:
  When a statistic query in is successful, it is saved to ProfileInfoCache
automatically.
  If nullptr is passed to tracker, the default tracker inside the class is used.
(Tasks cannot be cancelled this way.)
  Added convenience method StoreProfileStatisticsToCache, which calls
GetProfileStatistics(profile, no_op, nullptr).
  Reworked bookmark counting that using BookmarkModel before other bookmark
consumers do not result in a seg fault.

user_pod_row.js, user_manager_screen_handler.cc:
  Able to reads statistics from ProfileInfoCache.
  Bug fixes. (a.: Statistics are unnecessarily queried for desktop legacy
supervised users.
              b.: chrome.send('logRemoveUserWarningShown') is called more than
once when the deletion warning is shown.)

profile_manager.cc
  Store statistics to ProfileInfoCache during profile close and profile open (if
not normal shutdown).

BUG=501916, 502346

Note: it is a good idea to use a JSON formatter (e.g.
http://www.jsoneditoronline.org/ ) to pretiffy "Local State" during testing.

TEST=Back up the file "Local State" before starting. You need to restore this
file at the beginning of each test. The "Local State" file should not contain
the string "stats_settings". If it contains the string, then you can manually
delete the keys below (remember to validate the changed file using a JSON
formatter):
        "stats_bookmarks": 15,
        "stats_browsing_history": 4,
        "stats_passwords": 0,
        "stats_settings": 121,
Note that the numbers can be different, only the keys should be the same.

TEST=Restore "Local State". Start Chromium. Then close Chromium. Inspect the new
"Local State" file for the keys listed above. If possible check that the keys is
under the correct profile.

TEST=Restore "Local State". Start Chromium. Then kill the Chromium process.
Inspect the new "Local State" file for the keys listed above.

TEST=Restore "Local State". Start Chromium. Open two or more profiles. Close
Chromium. Start Chromium again. Go to the user switching dialog by selecting the
name in the right side of the title bar, press "Switch Person". Now click the
top-right corner of a profile and press "Remove Person". The statistics of the
profile should appear if and only if:
(1) The user is not a supervised user, and
(2) The profile has been open when you start Chromium in the first time.
Note that the statistics can be displayed in a few different ways, as stated in
bug 501916.

TEST=Restore "Local State". Start Chromium. Then shut down the computer without
Chromium closed. The statistics should not have been written in Local State yet.
Now start Chromium again, wait for a minute, and reboot again. Now the
statistics key should be in Local State. (You may get Local State updated after
the second start of Chromium but before the reboot. This is also passing the
test.)
Note: Do not shut down Chromium normally or just trying to kill Chromium
process. These way will cause statistics to be written in Local State resulting
in false passes. Anyway, if Local State gets the statistical keys before the
second start of Chromium, you should consider that the test is not carried out
correctly and restart the test.

Committed: https://crrev.com/8e1e1231e10afbfe816effeceeb4f10075e6ea09
Cr-Commit-Position: refs/heads/master@{#367568}
==========

[commit-bot: I haz the power, 2016-01-05 17:55:21]

Patchset 14 (id:??) landed as
https://crrev.com/8e1e1231e10afbfe816effeceeb4f10075e6ea09
Cr-Commit-Position: refs/heads/master@{#367568}