cros: Flush profile files at critical moments

cros: Flush profile files at critical moments

- Add a FileFlusher helper to flush files under a given dir;
- Add ProfileHelper::FlushProfile to flush files for a given profile;
- FlushProfile after a user profile is loaded and again after merge session;
- FlushProfile if the signin profile is just created;
- Commit local state change upon user session starts, oauth status
  and force online signin flag is changed;

BUG=592096
Committed: https://crrev.com/7131ebe0af4b20c6b0d8c138733cf944094e5adb
Cr-Commit-Position: refs/heads/master@{#382632}

[xiyuan, 2016-03-18 22:47:51]

xiyuan@chromium.org changed reviewers:
+ achuith@chromium.org

[xiyuan, 2016-03-18 22:47:51]

Achuith, could you help to review this? Thanks.

[achuithb, 2016-03-21 17:45:41]

What is the plan for test coverage for FileFlusher?

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
File chrome/browser/chromeos/base/file_flusher.cc (right):

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:52: void
ScheduleFinishOnUIThread();
Maybe add a comment to say that the Schedule call posts to the UI thread?

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:102: 
Would a VLOG(1) logging statement be useful here?

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:103: const bool kRecursive = true;
Is kRecursive appropriate for const stack variables? shouldn't this just be
recursive? Why not just inlign the arg with true /* recursive */

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:128:
content::BrowserThread::PostTask(
DCHECK_CURRENTLY_ON(content::BrowserThread::UI)?

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:148: for (const auto& job : jobs_)
are we allowed to drop {} here?

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:173: for (auto it = jobs_.begin();
it != jobs_.end(); ++it) {
for (auto job : jobs_)?

[achuithb, 2016-03-21 17:59:42]

lgtm.

The nits are optional and we had a conversation about the difficulty of end to
end testing here

[xiyuan, 2016-03-21 22:14:54]

Thanks for the review.

Per our discussion, even though we cannot really test the underlying file
system, it still good to have some coverage for FileFlusher itself. So added
sanity tests for FileFlusher to make sure flush is called on expected files.

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
File chrome/browser/chromeos/base/file_flusher.cc (right):

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:52: void
ScheduleFinishOnUIThread();
On 2016/03/21 17:45:41, achuithb wrote:
> Maybe add a comment to say that the Schedule call posts to the UI thread?

Done.

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:102: 
On 2016/03/21 17:45:41, achuithb wrote:
> Would a VLOG(1) logging statement be useful here?

Done.

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:103: const bool kRecursive = true;
On 2016/03/21 17:45:41, achuithb wrote:
> Is kRecursive appropriate for const stack variables? shouldn't this just be
> recursive? Why not just inlign the arg with true /* recursive */

Personally prefer to use a const bool instead of using a comment. The compiler
should optimize the const stack variables out. Not feel strongly about it
through. Changed as suggested.

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:128:
content::BrowserThread::PostTask(
On 2016/03/21 17:45:41, achuithb wrote:
> DCHECK_CURRENTLY_ON(content::BrowserThread::UI)?

The name is misleading. It schedules "FinishOnUITherad" but itself can be called
on any threads. Renamed to ScheduleFinish to avoid confusions.

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:148: for (const auto& job : jobs_)
On 2016/03/21 17:45:41, achuithb wrote:
> are we allowed to drop {} here?

Cancel() does not remove it from the vector. It schedules a removal. So we
should be good here.

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:173: for (auto it = jobs_.begin();
it != jobs_.end(); ++it) {
On 2016/03/21 17:45:41, achuithb wrote:
> for (auto job : jobs_)?

Need the iterator to remove |job| from |jobs_| so cannot use range loop here.

[achuithb, 2016-03-21 22:32:56]

lgtm! Thanks for the unittests!

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
File chrome/browser/chromeos/base/file_flusher.cc (right):

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:148: for (const auto& job : jobs_)
On 2016/03/21 22:14:54, xiyuan wrote:
> On 2016/03/21 17:45:41, achuithb wrote:
> > are we allowed to drop {} here?
> 
> Cancel() does not remove it from the vector. It schedules a removal. So we
> should be good here.

I didn't realize that braces were optional for single statement for loops (I
thought it was only for if statements)

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:173: for (auto it = jobs_.begin();
it != jobs_.end(); ++it) {
On 2016/03/21 22:14:54, xiyuan wrote:
> On 2016/03/21 17:45:41, achuithb wrote:
> > for (auto job : jobs_)?
> 
> Need the iterator to remove |job| from |jobs_| so cannot use range loop here.

Acknowledged.

https://codereview.chromium.org/1815853002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/base/file_flusher_unittest.cc (right):

https://codereview.chromium.org/1815853002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/base/file_flusher_unittest.cc:43: base::FilePath path =
base::FilePath::FromUTF8Unsafe(path_string);
nit: const

https://codereview.chromium.org/1815853002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/base/file_flusher_unittest.cc:54: int
GetFlushCount(const std::string& path_string) {
nit: const function

https://codereview.chromium.org/1815853002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/base/file_flusher_unittest.cc:92: // Invalid exclude
will be ignore.
s/ignore/ignored

[xiyuan, 2016-03-22 17:38:20]

Patchset #3 (id:40001) has been deleted

[xiyuan, 2016-03-22 17:41:20]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-03-22 17:41:20]

The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/1815853002/#ps60001
(title: "fix nits and create test files to fix trybots")

[commit-bot: I haz the power, 2016-03-22 17:41:39]

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

[xiyuan, 2016-03-22 18:41:33]

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
File chrome/browser/chromeos/base/file_flusher.cc (right):

https://codereview.chromium.org/1815853002/diff/1/chrome/browser/chromeos/bas...
chrome/browser/chromeos/base/file_flusher.cc:148: for (const auto& job : jobs_)
On 2016/03/21 22:32:56, achuithb wrote:
> On 2016/03/21 22:14:54, xiyuan wrote:
> > On 2016/03/21 17:45:41, achuithb wrote:
> > > are we allowed to drop {} here?
> > 
> > Cancel() does not remove it from the vector. It schedules a removal. So we
> > should be good here.
> 
> I didn't realize that braces were optional for single statement for loops (I
> thought it was only for if statements)

I completely mis-understood to comment. :p

And yes, "Braces are optional for single-statement loops", see
https://google.github.io/styleguide/cppguide.html#Loops_and_Switch_Statements

https://codereview.chromium.org/1815853002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/base/file_flusher_unittest.cc (right):

https://codereview.chromium.org/1815853002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/base/file_flusher_unittest.cc:43: base::FilePath path =
base::FilePath::FromUTF8Unsafe(path_string);
On 2016/03/21 22:32:56, achuithb wrote:
> nit: const

Done.

https://codereview.chromium.org/1815853002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/base/file_flusher_unittest.cc:54: int
GetFlushCount(const std::string& path_string) {
On 2016/03/21 22:32:56, achuithb wrote:
> nit: const function

Done.

https://codereview.chromium.org/1815853002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/base/file_flusher_unittest.cc:92: // Invalid exclude
will be ignore.
On 2016/03/21 22:32:56, achuithb wrote:
> s/ignore/ignored

Done.

[commit-bot: I haz the power, 2016-03-22 19:22:15]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-03-22 19:24:39]

Description was changed from

==========
cros: Flush profile files at critical moments

- Add a FileFlusher helper to flush files under a given dir;
- Add ProfileHelper::FlushProfile to flush files for a given profile;
- FlushProfile after a user profile is loaded and again after merge session;
- FlushProfile if the signin profile is just created;
- Commit local state change upon user session starts, oauth status
  and force online signin flag is changed;

BUG=592096
==========

to

==========
cros: Flush profile files at critical moments

- Add a FileFlusher helper to flush files under a given dir;
- Add ProfileHelper::FlushProfile to flush files for a given profile;
- FlushProfile after a user profile is loaded and again after merge session;
- FlushProfile if the signin profile is just created;
- Commit local state change upon user session starts, oauth status
  and force online signin flag is changed;

BUG=592096

Committed: https://crrev.com/7131ebe0af4b20c6b0d8c138733cf944094e5adb
Cr-Commit-Position: refs/heads/master@{#382632}
==========

[commit-bot: I haz the power, 2016-03-22 19:24:40]

Patchset 3 (id:??) landed as
https://crrev.com/7131ebe0af4b20c6b0d8c138733cf944094e5adb
Cr-Commit-Position: refs/heads/master@{#382632}