Mirror syncfs log to console and WebUI, with LogSeverity support.

Mirror syncfs log to console and WebUI, with LogSeverity support.

Please note that this supports INFO, WARNING, ERROR. I have yet to add support for DVLOG levels.

BUG=241689
TEST=chrome/browser/sync_file_system/logger_unittest.cc
unit_tests.LoggerTest*

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

[calvinlo, 2013-05-22 04:53:06]

Hi Hiroki, can you please take an initial look at this to see if this feature
addition would be useful?

It's manually testable using:
out/Debug/chrome --enable-logging --log-level=0 
out/Debug/chrome --enable-logging --log-level=1

go to URL chrome://syncfs-internals to check the output.
log-level=0 to get INFO messages and above, 1 for warnings and above. With level
0, you should see all output but with level 1, you should only see the warns and
errors.

If this looks ok to you, I'll add some tests for this before  going for a final
review to get a LGTM

[nhiroki, 2013-05-22 05:42:40]

Looks good to me

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive/api_util.cc (right):

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive/api_util.cc:311:
util::Log(logging::LOG_ERROR,
nit: Can you align the arguments?

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1592:
std::string("Detected unexpected OriginRoot deletion:").append(
nit: I'd prefer to use operator+() instead of append() here and elsewhere.

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive_file_sync_util.cc (right):

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_util.cc:80: LOG(WARNING) << "Got
unexpected error: " << error;
Can you also change this logging code?

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/local_file_sync_service.cc (right):

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/local_file_sync_service.cc:204: std::string
warn_msg = std::string(
How about using std::ostringstream?

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/local_file_sync_service.cc:205:
"PrepareForProcessRemoteChange called for non-existing origin:")
super-nit: s/origin:/origin: / for readability

[calvinlo, 2013-05-22 08:42:56]

Also added tests now. I think the patch is complete enough now for final review.

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive/api_util.cc (right):

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive/api_util.cc:311:
util::Log(logging::LOG_ERROR,
On 2013/05/22 05:42:40, nhiroki wrote:
> nit: Can you align the arguments?

Done.

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1592:
std::string("Detected unexpected OriginRoot deletion:").append(
On 2013/05/22 05:42:40, nhiroki wrote:
> nit: I'd prefer to use operator+() instead of append() here and elsewhere.

I forgot this logging statement is like printf and I can give variable number of
params. So I can use %s in the format string and just get rid of all the
appends.

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/drive_file_sync_util.cc (right):

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/drive_file_sync_util.cc:80: LOG(WARNING) << "Got
unexpected error: " << error;
On 2013/05/22 05:42:40, nhiroki wrote:
> Can you also change this logging code?

Done.

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
File chrome/browser/sync_file_system/local_file_sync_service.cc (right):

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/local_file_sync_service.cc:204: std::string
warn_msg = std::string(
On 2013/05/22 05:42:40, nhiroki wrote:
> How about using std::ostringstream?

Done.

https://codereview.chromium.org/15657002/diff/1/chrome/browser/sync_file_syst...
chrome/browser/sync_file_system/local_file_sync_service.cc:205:
"PrepareForProcessRemoteChange called for non-existing origin:")
On 2013/05/22 05:42:40, nhiroki wrote:
> super-nit: s/origin:/origin: / for readability

Done.

[nhiroki, 2013-05-22 09:18:24]

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger.cc (right):

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:52: // Static
nit: I think you don't have to add "Static" comment since this is just a
function.

We'd add the comment only when a function is static member in a class.

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:57: // Static
ditto.

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:80: // Static
ditto.

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger_unittest.cc (right):

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:18:
util::Log(logging::LOG_ERROR, "Error info message");
Can you define these messages as constant? For example,

const char kInfoMessage[] = "Info test message";
const char kWarningMessage[] = "Warning test message";
const char kErrorMessage[] = "Error test message";

And then pass them at line 42-44 and 54.

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:44: ASSERT_EQ("[Error] Error
info message", log[2].what);
How about adding ClearLog() test?

util::ClearLog();
ASSERT_EQ(0u, log.size());

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:48: // Check that all
messages are logged when log LogSeverity is set to INFO.
Can you update this comment?

[tzik, 2013-05-22 09:30:54]

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger.cc (right):

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:33: void
LogToConsole(logging::LogSeverity level, std::string log_event) {
const ref?

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/sync_file_system_service.cc (right):

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/sync_file_system_service.cc:73:
origin.spec().c_str());
This replacement drops the location of the log (e.g. file name, line number and
function name).
Could you add a macro to keep them?

[tzik, 2013-05-22 09:35:56]

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger.cc (right):

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:74: ptr->Log("%s", (level_string +
what).c_str());
Not needed in this CL, but can we add non-format version to EventLogger?

[calvinlo, 2013-05-23 10:00:15]

Please see the screenshot attached to the bug
https://code.google.com/p/chromium/issues/detail?id=241689 for  reference of how
this looks now in the console and chrome://syncfs-internals UI

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger.cc (right):

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:33: void
LogToConsole(logging::LogSeverity level, std::string log_event) {
On 2013/05/22 09:30:54, tzik wrote:
> const ref?

Done.

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:52: // Static
On 2013/05/22 09:18:24, nhiroki wrote:
> nit: I think you don't have to add "Static" comment since this is just a
> function.
> 
> We'd add the comment only when a function is static member in a class.

Done.

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:57: // Static
On 2013/05/22 09:18:24, nhiroki wrote:
> ditto.

Done.

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:74: ptr->Log("%s", (level_string +
what).c_str());
On 2013/05/22 09:35:56, tzik wrote:
> Not needed in this CL, but can we add non-format version to EventLogger?

Ok thanks for explaining this offline. I will try to do this in another CL and
send it to one of the owners of google_apis later.

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:80: // Static
On 2013/05/22 09:18:24, nhiroki wrote:
> ditto.

Done.

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger_unittest.cc (right):

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:18:
util::Log(logging::LOG_ERROR, "Error info message");
On 2013/05/22 09:18:24, nhiroki wrote:
> Can you define these messages as constant? For example,
> 
> const char kInfoMessage[] = "Info test message";
> const char kWarningMessage[] = "Warning test message";
> const char kErrorMessage[] = "Error test message";
> 
> And then pass them at line 42-44 and 54.

Per discussion, it seems like this is not worth doing because of compilation
issues.

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:44: ASSERT_EQ("[Error] Error
info message", log[2].what);
On 2013/05/22 09:18:24, nhiroki wrote:
> How about adding ClearLog() test?
> 
> util::ClearLog();
> ASSERT_EQ(0u, log.size());

Done.

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:48: // Check that all
messages are logged when log LogSeverity is set to INFO.
On 2013/05/22 09:18:24, nhiroki wrote:
> Can you update this comment?

Done. Sorry, lazy copy and paste.

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/sync_file_system_service.cc (right):

https://codereview.chromium.org/15657002/diff/15001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/sync_file_system_service.cc:73:
origin.spec().c_str());
On 2013/05/22 09:30:54, tzik wrote:
> This replacement drops the location of the log (e.g. file name, line number
and
> function name).
> Could you add a macro to keep them?

Thanks for pointing this out and suggesting the implementation. I think I have
this working now.

[tzik, 2013-05-24 04:08:32]

https://codereview.chromium.org/15657002/diff/40001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger.cc (right):

https://codereview.chromium.org/15657002/diff/40001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:73: base::FilePath
path(FILE_PATH_LITERAL(location.file_name()));
FILE_PATH_LITERAL is only for a literal. This will hit a compile error on
windows.

https://codereview.chromium.org/15657002/diff/40001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:88: LogToConsole(severity,
log_output);
Can we use RowLog instead? And then can we drop LogToConsole?

https://codereview.chromium.org/15657002/diff/40001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger.h (right):

https://codereview.chromium.org/15657002/diff/40001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.h:42: (__VA_ARGS__)
Could you avoid using macros in other than simplest case?
This looks too complicated for me to accept...

If this is needed, we should use Log directly.

[calvinlo, 2013-05-24 05:12:43]

https://codereview.chromium.org/15657002/diff/40001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger.cc (right):

https://codereview.chromium.org/15657002/diff/40001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:73: base::FilePath
path(FILE_PATH_LITERAL(location.file_name()));
On 2013/05/24 04:08:32, tzik wrote:
> FILE_PATH_LITERAL is only for a literal. This will hit a compile error on
> windows.

Thanks for Hiroki for pointing out that I can probably use
base::FilePath::FromUTF8Unsafe(location.file_name());

https://codereview.chromium.org/15657002/diff/40001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:88: LogToConsole(severity,
log_output);
On 2013/05/24 04:08:32, tzik wrote:
> Can we use RowLog instead? And then can we drop LogToConsole?

I tried this originally but I couldn't figure out a good way to map from
logging::LogSeverity severity back to the macros INFO, WARNING, ERROR. i.e. how
to do 
RAW_LOG(severity, message) without the switch from LogToConsole();

I could move the LOG(severity) statement back into the macro but then I'd have
to keep the StringPrintf in the macro too.

Is there a third solution?

https://codereview.chromium.org/15657002/diff/40001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger.h (right):

https://codereview.chromium.org/15657002/diff/40001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.h:42: (__VA_ARGS__)
On 2013/05/24 04:08:32, tzik wrote:
> Could you avoid using macros in other than simplest case?
> This looks too complicated for me to accept...
> 
> If this is needed, we should use Log directly.

Ok, I did a little bit more research on MARCROs and I think I figured out the
magic syntax to make this work with just a single macro now.

[tzik, 2013-05-24 11:17:38]

https://codereview.chromium.org/15657002/diff/40001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger.cc (right):

https://codereview.chromium.org/15657002/diff/40001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.cc:88: LogToConsole(severity,
log_output);
On 2013/05/24 05:12:43, calvinlo wrote:
> On 2013/05/24 04:08:32, tzik wrote:
> > Can we use RowLog instead? And then can we drop LogToConsole?
> 
> I tried this originally but I couldn't figure out a good way to map from
> logging::LogSeverity severity back to the macros INFO, WARNING, ERROR. i.e.
how
> to do 
> RAW_LOG(severity, message) without the switch from LogToConsole();
> 
> I could move the LOG(severity) statement back into the macro but then I'd have
> to keep the StringPrintf in the macro too.
> 
> Is there a third solution?

Just like below seems to work:
logging::RawLog(severity, log_output.c_str());

https://codereview.chromium.org/15657002/diff/55001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger.h (right):

https://codereview.chromium.org/15657002/diff/55001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.h:34: FROM_HERE, logging::LOG_##severity,
format, ##__VA_ARGS__);
This is a shorthand of
util::Log(FROM_HERE, logging::LOG_WARNING,
          "abc%s", "ba");
with
SYNCFS_LOG(WARNING, "abc%s", "ba")

But, this seem not worth defining new macro in .h as our coding style warns:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro...

Yes, once I suggested you to use macro to insert FROM_HERE, though let me change
my recommendation:
Could we directly write down FROM_HERE and logging::LOG_ for each call site?

[calvinlo, 2013-05-25 06:03:25]

https://codereview.chromium.org/15657002/diff/55001/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger.h (right):

https://codereview.chromium.org/15657002/diff/55001/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.h:34: FROM_HERE, logging::LOG_##severity,
format, ##__VA_ARGS__);
On 2013/05/24 11:17:38, tzik wrote:
> This is a shorthand of
> util::Log(FROM_HERE, logging::LOG_WARNING,
>           "abc%s", "ba");
> with
> SYNCFS_LOG(WARNING, "abc%s", "ba")
> 
> But, this seem not worth defining new macro in .h as our coding style warns:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro...
> 
> Yes, once I suggested you to use macro to insert FROM_HERE, though let me
change
> my recommendation:
> Could we directly write down FROM_HERE and logging::LOG_ for each call site?

Ah yes, after reading the Style guide reference you linked I agree that using
this Macro is a great idea here. I will change all the call sites to be regular
function calls.

[kinuko, 2013-05-27 02:30:46]

I've written a few high-level comments on the issue. I continue to defer this
review to tzik/nhiroki but please also take a look at the comment (all of you):
https://code.google.com/p/chromium/issues/detail?id=242792#c1

[nhiroki, 2013-05-27 08:34:32]

lgtm, please wait for tzik-san's comment

[tzik, 2013-05-27 09:08:51]

lgtm

https://codereview.chromium.org/15657002/diff/68002/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger.h (right):

https://codereview.chromium.org/15657002/diff/68002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.h:26: tracked_objects::Location location,
const ref?

https://codereview.chromium.org/15657002/diff/68002/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger_unittest.cc (right):

https://codereview.chromium.org/15657002/diff/68002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:47:
ASSERT_TRUE(ContainsString("Info test message", log[0]));
can be EXPECT_TRUE for this and two below?

https://codereview.chromium.org/15657002/diff/68002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:59:
ASSERT_TRUE(ContainsString("Error test message", log[0]));
ditto for this ASSERT_TRUE

https://codereview.chromium.org/15657002/diff/68002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:64: ASSERT_EQ(3u,
util::GetLogHistory().size());
ditto for both of this ASSERT_EQ

[calvinlo, 2013-05-27 09:29:27]

https://codereview.chromium.org/15657002/diff/68002/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger.h (right):

https://codereview.chromium.org/15657002/diff/68002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger.h:26: tracked_objects::Location location,
On 2013/05/27 09:08:52, tzik wrote:
> const ref?

Done.

https://codereview.chromium.org/15657002/diff/68002/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger_unittest.cc (right):

https://codereview.chromium.org/15657002/diff/68002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:47:
ASSERT_TRUE(ContainsString("Info test message", log[0]));
On 2013/05/27 09:08:52, tzik wrote:
> can be EXPECT_TRUE for this and two below?

Done.

https://codereview.chromium.org/15657002/diff/68002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:59:
ASSERT_TRUE(ContainsString("Error test message", log[0]));
On 2013/05/27 09:08:52, tzik wrote:
> ditto for this ASSERT_TRUE

Done.

https://codereview.chromium.org/15657002/diff/68002/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:64: ASSERT_EQ(3u,
util::GetLogHistory().size());
On 2013/05/27 09:08:52, tzik wrote:
> ditto for both of this ASSERT_EQ

Done.

[commit-bot: I haz the power, 2013-05-27 09:32:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/15657002/84001

[commit-bot: I haz the power, 2013-05-27 10:00:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-27 10:41:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/15657002/84005

[tzik, 2013-05-27 10:55:44]

https://codereview.chromium.org/15657002/diff/84005/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger_unittest.cc (right):

https://codereview.chromium.org/15657002/diff/84005/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:46: EXPECT_EQ(3u,
log.size());
The three EXPECT_TRUE below do not cause crash even if one of them doesn't hold,
but the EXPECT_EQ above does.
So, this should be ASSERT_EQ.

https://codereview.chromium.org/15657002/diff/84005/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:58: EXPECT_EQ(1u,
log.size());
ditto.

[calvinlo, 2013-05-27 11:02:33]

On 2013/05/27 10:55:44, tzik wrote:
>
https://codereview.chromium.org/15657002/diff/84005/chrome/browser/sync_file_...
> File chrome/browser/sync_file_system/logger_unittest.cc (right):
> 
>
https://codereview.chromium.org/15657002/diff/84005/chrome/browser/sync_file_...
> chrome/browser/sync_file_system/logger_unittest.cc:46: EXPECT_EQ(3u,
> log.size());
> The three EXPECT_TRUE below do not cause crash even if one of them doesn't
hold,
> but the EXPECT_EQ above does.
> So, this should be ASSERT_EQ.
> 
>
https://codereview.chromium.org/15657002/diff/84005/chrome/browser/sync_file_...
> chrome/browser/sync_file_system/logger_unittest.cc:58: EXPECT_EQ(1u,
> log.size());
> ditto.

Sorry fixed. I didn't notice that after I did a search and string replace.

[calvinlo, 2013-05-27 11:02:42]

https://codereview.chromium.org/15657002/diff/84005/chrome/browser/sync_file_...
File chrome/browser/sync_file_system/logger_unittest.cc (right):

https://codereview.chromium.org/15657002/diff/84005/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:46: EXPECT_EQ(3u,
log.size());
On 2013/05/27 10:55:45, tzik wrote:
> The three EXPECT_TRUE below do not cause crash even if one of them doesn't
hold,
> but the EXPECT_EQ above does.
> So, this should be ASSERT_EQ.

Done.

https://codereview.chromium.org/15657002/diff/84005/chrome/browser/sync_file_...
chrome/browser/sync_file_system/logger_unittest.cc:58: EXPECT_EQ(1u,
log.size());
On 2013/05/27 10:55:45, tzik wrote:
> ditto.

Done.

[commit-bot: I haz the power, 2013-05-27 11:03:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calvinlo@chromium.org/15657002/99001

[commit-bot: I haz the power, 2013-05-27 14:35:27]

Change committed as 202424