[SyncFS] Add TaskLogger and wire up it.

[SyncFS] Add TaskLogger and wire up it.

 * Add TaskLogger class to log per-task status.
 * Wire up it SyncFileSystemService to SyncTaskManager.

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

[tzik, 2014-05-23 16:59:39]

PTL.

This is another missing piece for the thread migration.
And also this is desirable for the task parallelization.
I'll wire up this to syncfs-internals and will add actual logging part in
following CL.

[peria, 2014-05-24 02:50:07]

LGTM with a nit.

https://codereview.chromium.org/291403004/diff/20001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend_v1/drive_file_sync_service.cc
(right):

https://codereview.chromium.org/291403004/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend_v1/drive_file_sync_service.cc:1088:
void DriveFileSyncService::RecordTaskLog(scoped_ptr<TaskLogger::TaskLog> log) {
please put NOTIMPLEMENTED(), TODO(), other comment, or {}.
{} shows intentional no-work.

[nhiroki, 2014-05-26 01:30:37]

LGTM

https://codereview.chromium.org/291403004/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/task_logger.h (right):

https://codereview.chromium.org/291403004/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/task_logger.h:54: size_t log_size_;
How about renaming this with "max_log_size_"?

And if this will be immutable, can you make this const or define as a constant
in *.cc?

[tzik, 2014-05-26 03:22:34]

https://codereview.chromium.org/291403004/diff/20001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend_v1/drive_file_sync_service.cc
(right):

https://codereview.chromium.org/291403004/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend_v1/drive_file_sync_service.cc:1088:
void DriveFileSyncService::RecordTaskLog(scoped_ptr<TaskLogger::TaskLog> log) {
On 2014/05/24 02:50:08, peria wrote:
> please put NOTIMPLEMENTED(), TODO(), other comment, or {}.
> {} shows intentional no-work.

Added a comment.

NOTIMPLEMENTED() may cause DCHECK failure or LOG(ERROR) depends on the
configuration.
So, it doesn't fit to intensional situation in production code.

Do we have any guide line that treat "{}" and "{\n}" differently?

[peria, 2014-05-26 03:40:34]

https://codereview.chromium.org/291403004/diff/20001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend_v1/drive_file_sync_service.cc
(right):

https://codereview.chromium.org/291403004/diff/20001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend_v1/drive_file_sync_service.cc:1088:
void DriveFileSyncService::RecordTaskLog(scoped_ptr<TaskLogger::TaskLog> log) {
On 2014/05/26 03:22:34, tzik wrote:
> On 2014/05/24 02:50:08, peria wrote:
> > please put NOTIMPLEMENTED(), TODO(), other comment, or {}.
> > {} shows intentional no-work.
> 
> Added a comment.
> 
> NOTIMPLEMENTED() may cause DCHECK failure or LOG(ERROR) depends on the
> configuration.
> So, it doesn't fit to intensional situation in production code.
> 
> Do we have any guide line that treat "{}" and "{\n}" differently?

The difference between "{}" and "{\n}" is not written in the style guide, and it
is my preference.
"{\n}" is difficult for me to judge if it is
- to be implemented, but forgotten to do.  (--> need more work)
- not implemented intentionally.
- to be implemented in other CLs.  (--> put a TODO)
So I asked it.

[tzik, 2014-05-26 04:08:06]

https://codereview.chromium.org/291403004/diff/40001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/task_logger.h (right):

https://codereview.chromium.org/291403004/diff/40001/chrome/browser/sync_file...
chrome/browser/sync_file_system/task_logger.h:54: size_t log_size_;
On 2014/05/26 01:30:37, nhiroki wrote:
> How about renaming this with "max_log_size_"?
> 
> And if this will be immutable, can you make this const or define as a constant
> in *.cc?

Done.

[tzik, 2014-05-26 04:14:39]

On 2014/05/26 03:40:34, peria wrote:
>
https://codereview.chromium.org/291403004/diff/20001/chrome/browser/sync_file...
> File
chrome/browser/sync_file_system/drive_backend_v1/drive_file_sync_service.cc
> (right):
> 
>
https://codereview.chromium.org/291403004/diff/20001/chrome/browser/sync_file...
>
chrome/browser/sync_file_system/drive_backend_v1/drive_file_sync_service.cc:1088:
> void DriveFileSyncService::RecordTaskLog(scoped_ptr<TaskLogger::TaskLog> log)
{
> On 2014/05/26 03:22:34, tzik wrote:
> > On 2014/05/24 02:50:08, peria wrote:
> > > please put NOTIMPLEMENTED(), TODO(), other comment, or {}.
> > > {} shows intentional no-work.
> > 
> > Added a comment.
> > 
> > NOTIMPLEMENTED() may cause DCHECK failure or LOG(ERROR) depends on the
> > configuration.
> > So, it doesn't fit to intensional situation in production code.
> > 
> > Do we have any guide line that treat "{}" and "{\n}" differently?
> 
> The difference between "{}" and "{\n}" is not written in the style guide, and
it
> is my preference.
> "{\n}" is difficult for me to judge if it is
> - to be implemented, but forgotten to do.  (--> need more work)
> - not implemented intentionally.
> - to be implemented in other CLs.  (--> put a TODO)
> So I asked it.

I believe these two mean exactly same: a empty block or definition.
And an empty is also an implementation.

[tzik, 2014-05-26 04:15:33]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-05-26 04:16:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tzik@chromium.org/291403004/60001

[commit-bot: I haz the power, 2014-05-26 08:33:35]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-26 09:36:02]

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

[commit-bot: I haz the power, 2014-05-26 09:36:03]

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

[tzik, 2014-05-26 14:59:59]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-05-26 15:00:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tzik@chromium.org/291403004/80001

[commit-bot: I haz the power, 2014-05-26 16:46:11]

Change committed as 272857