Report CHECK/DCHECK to test launcher summary output.

Report CHECK/DCHECK to test launcher summary output.

This CL is continuation of previous one:
https://codereview.chromium.org/2606153003/

BUG=

Review-Url: https://codereview.chromium.org/2638763004
Cr-Commit-Position: refs/heads/master@{#465531}
Committed: https://chromium.googlesource.com/chromium/src/+/cc1bde6592ae5e0b5064e7680941569625db9d09

[alex-ac, 2017-01-17 14:18:10]

Description was changed from

==========
Report CHECK/DCHECK to test launcher summary output.

This CL is continuation of previous one:
https://codereview.chromium.org/2606153003/

BUG=
==========

to

==========
Report CHECK/DCHECK to test launcher summary output.

This CL is continuation of previous one:
https://codereview.chromium.org/2606153003/

BUG=
==========

[alex-ac, 2017-01-17 14:18:10]

alex-ac@yandex-team.ru changed reviewers:
+ phajdan.jr@chromium.org

[alex-ac, 2017-01-17 16:24:20]

alex-ac@yandex-team.ru changed reviewers:
+ dcheng@chromium.org

[alex-ac, 2017-01-20 08:13:56]

ping
phajdan.jr: Please take a look on changes in base/test.
dcheng: Please take a look on changes in base/logging_unittest.cc.

[dcheng, 2017-01-20 08:26:23]

https://codereview.chromium.org/2638763004/diff/20001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/2638763004/diff/20001/base/logging_unittest.c...
base/logging_unittest.cc:56: // This object will temporary disable message
handler for test.
Nit: Scoper object to temporarily clear any log message handlers for testing
DCHECK/CHECK functionality.

https://codereview.chromium.org/2638763004/diff/20001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/20001/base/test/test_suite.cc...
base/test/test_suite.cc:325: //   DCHECK(foo) << "bar\nbuzz";
If it helps, we could change the output to be friendlier for test launcher
parsing?

Also, since log message handlers aren't currently chained in any sort of way...
are there problems with things that override the log message handler and don't
forward?

[alex-ac, 2017-01-20 12:11:17]

https://codereview.chromium.org/2638763004/diff/20001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/2638763004/diff/20001/base/logging_unittest.c...
base/logging_unittest.cc:56: // This object will temporary disable message
handler for test.
On 2017/01/20 08:26:23, dcheng wrote:
> Nit: Scoper object to temporarily clear any log message handlers for testing
> DCHECK/CHECK functionality.

Done.

https://codereview.chromium.org/2638763004/diff/20001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/20001/base/test/test_suite.cc...
base/test/test_suite.cc:325: //   DCHECK(foo) << "bar\nbuzz";
On 2017/01/20 08:26:23, dcheng wrote:
> If it helps, we could change the output to be friendlier for test launcher
> parsing?

Done.

> Also, since log message handlers aren't currently chained in any sort of
way...
> are there problems with things that override the log message handler and don't
> forward?

Yes, DCHECK/CHECK will not be catched if log message handler is overriden by the
test. I found less then 10 tests that do this. It's possible to change api to
chain handlers, but i think that implementation will be too complicated for
logger.

There is another way to handle DCHECK/CHECK in all tests. I can change
LogAssertHandlerFunction signature to provide file, line and message_start for
it. Assert handler is not changed by tests and it already used here.

[Paweł Hajdan Jr., 2017-01-23 16:55:07]

https://codereview.chromium.org/2638763004/diff/40001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2638763004/diff/40001/base/logging.cc#newcode345
base/logging.cc:345: const char kStackTraceMarker[] = "\nStack trace is
below:\n";
nit: Could you remove " is below" part?

https://codereview.chromium.org/2638763004/diff/40001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/40001/base/test/test_suite.cc...
base/test/test_suite.cc:252:
logging::SetLogMessageHandler(UnitTestLogMessageHandler);
Is this composable with any other possibly installed log message handlers?

Should we assert on teardown that the installed log message handler at that
point is what we expect?

https://codereview.chromium.org/2638763004/diff/40001/base/test/test_suite.cc...
base/test/test_suite.cc:330: g_test_suite->printer_->OnAssert(file, line,
summary, message);
This entire thing seems hacky. We use globals, and then do some text scraping.

What's the intended goal of this CL? Are there better alternatives, such as
creating logging handler API that would allow the callback to go to specific
object's method instead of a static method?

[alex-ac, 2017-01-27 12:55:46]

https://codereview.chromium.org/2638763004/diff/40001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2638763004/diff/40001/base/logging.cc#newcode345
base/logging.cc:345: const char kStackTraceMarker[] = "\nStack trace is
below:\n";
On 2017/01/23 16:55:06, Paweł Hajdan Jr. wrote:
> nit: Could you remove " is below" part?

Done.

https://codereview.chromium.org/2638763004/diff/40001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/40001/base/test/test_suite.cc...
base/test/test_suite.cc:252:
logging::SetLogMessageHandler(UnitTestLogMessageHandler);
On 2017/01/23 16:55:06, Paweł Hajdan Jr. wrote:
> Is this composable with any other possibly installed log message handlers?
> 
> Should we assert on teardown that the installed log message handler at that
> point is what we expect?

I've changed the way of setting LogAssertMessageHandler. I don't think that we
should assert about correct handler.

https://codereview.chromium.org/2638763004/diff/40001/base/test/test_suite.cc...
base/test/test_suite.cc:330: g_test_suite->printer_->OnAssert(file, line,
summary, message);
On 2017/01/23 16:55:06, Paweł Hajdan Jr. wrote:
> This entire thing seems hacky. We use globals, and then do some text scraping.
> 
> What's the intended goal of this CL? Are there better alternatives, such as
> creating logging handler API that would allow the callback to go to specific
> object's method instead of a static method?

I've changed logging handler API to use base::Callback. There are no globals and
text scraping now.

[Paweł Hajdan Jr., 2017-01-27 17:19:44]

Thank you for the changes. It looks like moving in the right direction. Some
more comments about new pieces.

https://codereview.chromium.org/2638763004/diff/80001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2638763004/diff/80001/base/logging.cc#newcode477
base/logging.cc:477: PushLogAssertHandler(handler);
Why create separate functions instead of putting the logic directly here?

https://codereview.chromium.org/2638763004/diff/80001/base/logging.cc#newcode779
base/logging.cc:779: log_assert_handler.Run(file_, line_, message_start_,
stack_start,
Would it make sense to execute all handlers on the stack?

https://codereview.chromium.org/2638763004/diff/80001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2638763004/diff/80001/base/logging.h#newcode283
base/logging.h:283: size_t message_start,
I'm worried these indexes are a weird API.

Why not pass two strings instead, one for the message and second one for the
stack trace?

https://codereview.chromium.org/2638763004/diff/80001/base/logging.h#newcode288
base/logging.h:288: // You probably must use ScopedLogAssertHandler instead of
this functions.
nit: "Probably" and "must" don't mix well for me. Could you change this to
something like "consider" or similar.

https://codereview.chromium.org/2638763004/diff/80001/base/logging.h#newcode290
base/logging.h:290: BASE_EXPORT void
SetLogAssertHandler(LogAssertHandlerFunction handler);
How about changing the names to more like Push/Pop? That reflects the new
semantics better than Set/Reset.

https://codereview.chromium.org/2638763004/diff/80001/base/test/gtest_xml_uni...
File base/test/gtest_xml_unittest_result_printer.cc (right):

https://codereview.chromium.org/2638763004/diff/80001/base/test/gtest_xml_uni...
base/test/gtest_xml_unittest_result_printer.cc:42: if (output_file_) {
nit: Please reverse condition, do early return, and de-indent:

if (!output_file_)
  return;

https://codereview.chromium.org/2638763004/diff/80001/base/test/logging_utils.cc
File base/test/logging_utils.cc (right):

https://codereview.chromium.org/2638763004/diff/80001/base/test/logging_utils...
base/test/logging_utils.cc:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
nit: 2017, no (c).

https://codereview.chromium.org/2638763004/diff/80001/base/test/logging_utils.h
File base/test/logging_utils.h (right):

https://codereview.chromium.org/2638763004/diff/80001/base/test/logging_utils...
base/test/logging_utils.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
nit: 2017, no (c).

https://codereview.chromium.org/2638763004/diff/80001/base/test/logging_utils...
base/test/logging_utils.h:24: private:
nit: DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/2638763004/diff/80001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/80001/base/test/test_suite.cc...
base/test/test_suite.cc:68: TestSuite* g_test_suite = nullptr;
This shouldn't be needed now, right?

https://codereview.chromium.org/2638763004/diff/80001/net/test/scoped_disable...
File net/test/scoped_disable_exit_on_dfatal.h (right):

https://codereview.chromium.org/2638763004/diff/80001/net/test/scoped_disable...
net/test/scoped_disable_exit_on_dfatal.h:21: class ScopedDisableExitOnDFatal :
public logging::ScopedLogAssertHandler {
Should we use composition instead of inheritance?

[alex-ac, 2017-02-11 20:12:20]

https://codereview.chromium.org/2638763004/diff/80001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2638763004/diff/80001/base/logging.cc#newcode477
base/logging.cc:477: PushLogAssertHandler(handler);
On 2017/01/27 17:19:43, Paweł Hajdan Jr. wrote:
> Why create separate functions instead of putting the logic directly here?

Done.

https://codereview.chromium.org/2638763004/diff/80001/base/logging.cc#newcode779
base/logging.cc:779: log_assert_handler.Run(file_, line_, message_start_,
stack_start,
On 2017/01/27 17:19:44, Paweł Hajdan Jr. wrote:
> Would it make sense to execute all handlers on the stack?

No there's some code that adds new handler to prevent previous one from being
called.

https://codereview.chromium.org/2638763004/diff/80001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2638763004/diff/80001/base/logging.h#newcode283
base/logging.h:283: size_t message_start,
On 2017/01/27 17:19:44, Paweł Hajdan Jr. wrote:
> I'm worried these indexes are a weird API.
> 
> Why not pass two strings instead, one for the message and second one for the
> stack trace?

Done.

https://codereview.chromium.org/2638763004/diff/80001/base/logging.h#newcode288
base/logging.h:288: // You probably must use ScopedLogAssertHandler instead of
this functions.
On 2017/01/27 17:19:44, Paweł Hajdan Jr. wrote:
> nit: "Probably" and "must" don't mix well for me. Could you change this to
> something like "consider" or similar.

Done.

https://codereview.chromium.org/2638763004/diff/80001/base/logging.h#newcode290
base/logging.h:290: BASE_EXPORT void
SetLogAssertHandler(LogAssertHandlerFunction handler);
On 2017/01/27 17:19:44, Paweł Hajdan Jr. wrote:
> How about changing the names to more like Push/Pop? That reflects the new
> semantics better than Set/Reset.

Done.

https://codereview.chromium.org/2638763004/diff/80001/base/test/gtest_xml_uni...
File base/test/gtest_xml_unittest_result_printer.cc (right):

https://codereview.chromium.org/2638763004/diff/80001/base/test/gtest_xml_uni...
base/test/gtest_xml_unittest_result_printer.cc:42: if (output_file_) {
On 2017/01/27 17:19:44, Paweł Hajdan Jr. wrote:
> nit: Please reverse condition, do early return, and de-indent:
> 
> if (!output_file_)
>   return;

Done.

https://codereview.chromium.org/2638763004/diff/80001/base/test/logging_utils.cc
File base/test/logging_utils.cc (right):

https://codereview.chromium.org/2638763004/diff/80001/base/test/logging_utils...
base/test/logging_utils.cc:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2017/01/27 17:19:44, Paweł Hajdan Jr. wrote:
> nit: 2017, no (c).

Done.

https://codereview.chromium.org/2638763004/diff/80001/base/test/logging_utils.h
File base/test/logging_utils.h (right):

https://codereview.chromium.org/2638763004/diff/80001/base/test/logging_utils...
base/test/logging_utils.h:1: // Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2017/01/27 17:19:44, Paweł Hajdan Jr. wrote:
> nit: 2017, no (c).

Done.

https://codereview.chromium.org/2638763004/diff/80001/base/test/logging_utils...
base/test/logging_utils.h:24: private:
On 2017/01/27 17:19:44, Paweł Hajdan Jr. wrote:
> nit: DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/2638763004/diff/80001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/80001/base/test/test_suite.cc...
base/test/test_suite.cc:68: TestSuite* g_test_suite = nullptr;
On 2017/01/27 17:19:44, Paweł Hajdan Jr. wrote:
> This shouldn't be needed now, right?

Done.

https://codereview.chromium.org/2638763004/diff/80001/net/test/scoped_disable...
File net/test/scoped_disable_exit_on_dfatal.h (right):

https://codereview.chromium.org/2638763004/diff/80001/net/test/scoped_disable...
net/test/scoped_disable_exit_on_dfatal.h:21: class ScopedDisableExitOnDFatal :
public logging::ScopedLogAssertHandler {
On 2017/01/27 17:19:44, Paweł Hajdan Jr. wrote:
> Should we use composition instead of inheritance?

Done.

[Paweł Hajdan Jr., 2017-02-13 17:17:23]

phajdan.jr@chromium.org changed reviewers:
+ brettw@chromium.org

[Paweł Hajdan Jr., 2017-02-13 17:17:24]

+brettw to double-check this review ; consider this FYI

https://codereview.chromium.org/2638763004/diff/120001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2638763004/diff/120001/base/logging.cc#newcod...
base/logging.cc:771: // Make a copy of the string for the handler out of
paranoia.
nit: This comment seems out-of-date now that we take substrings instead.

https://codereview.chromium.org/2638763004/diff/120001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2638763004/diff/120001/base/logging.h#newcode287
base/logging.h:287: // Counsider to use ScopedLogAssertHandler instead of this
functions.
nit: counsider -> consider ; this -> these

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
File base/test/logging_utils.cc (right):

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.cc:1: // Copyright (c) 2017 The Chromium Authors. All
rights reserved.
nit: No (c)

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.cc:13: // std::swap doesn't works with base::Callback.
nit: doesn't works -> doesn't work

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_utils.h
File base/test/logging_utils.h (right):

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.h:1: // Copyright (c) 2017 The Chromium Authors. All
rights reserved.
nit: No (c)

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.h:18: ScopedLogAssertHandler();
Why do we need default ctor?

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.h:20: ScopedLogAssertHandler(ScopedLogAssertHandler&&
other);
Why do we need this and operator= ?

https://codereview.chromium.org/2638763004/diff/120001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/120001/base/test/test_suite.c...
base/test/test_suite.cc:304: printer_->OnAssert(file, line, summary, summary +
stack_trace);
Why do we pass "summary + stack_trace" instead of just stack_trace?

https://codereview.chromium.org/2638763004/diff/120001/sandbox/linux/tests/ma...
File sandbox/linux/tests/main.cc (right):

https://codereview.chromium.org/2638763004/diff/120001/sandbox/linux/tests/ma...
sandbox/linux/tests/main.cc:42: void UnitTestAssertHandler(const char* file, int
line, size_t message_start,
This seems to still use the old signature.

[alex-ac, 2017-02-14 10:34:28]

https://codereview.chromium.org/2638763004/diff/120001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2638763004/diff/120001/base/logging.cc#newcod...
base/logging.cc:771: // Make a copy of the string for the handler out of
paranoia.
On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> nit: This comment seems out-of-date now that we take substrings instead.

Done.

https://codereview.chromium.org/2638763004/diff/120001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2638763004/diff/120001/base/logging.h#newcode287
base/logging.h:287: // Counsider to use ScopedLogAssertHandler instead of this
functions.
On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> nit: counsider -> consider ; this -> these

Done.

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
File base/test/logging_utils.cc (right):

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.cc:1: // Copyright (c) 2017 The Chromium Authors. All
rights reserved.
On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> nit: No (c)

Done.

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.cc:13: // std::swap doesn't works with base::Callback.
On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> nit: doesn't works -> doesn't work

Done.

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_utils.h
File base/test/logging_utils.h (right):

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.h:1: // Copyright (c) 2017 The Chromium Authors. All
rights reserved.
On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> nit: No (c)

Done.

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.h:18: ScopedLogAssertHandler();
On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> Why do we need default ctor?
It allows to use this object as an class variable and initialize it after
execution of the constructor. (see base/test/test_suite.cc, where this object is
initialized in the Initialize method).

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.h:20: ScopedLogAssertHandler(ScopedLogAssertHandler&&
other);
On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> Why do we need this and operator= ?
Move-assignment operator allows to initialize default-constructed scoped object.

https://codereview.chromium.org/2638763004/diff/120001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/120001/base/test/test_suite.c...
base/test/test_suite.cc:304: printer_->OnAssert(file, line, summary, summary +
stack_trace);
On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> Why do we pass "summary + stack_trace" instead of just stack_trace?
XmlUnitTestResultPrinter inherits gtest format, where assert has summary and
message. In GTest, summary is just a logged text, and message is a logged text,
concatenated with stack trace of assert. I think it's better to use same format
for DCHECK/CHECK.

https://codereview.chromium.org/2638763004/diff/120001/sandbox/linux/tests/ma...
File sandbox/linux/tests/main.cc (right):

https://codereview.chromium.org/2638763004/diff/120001/sandbox/linux/tests/ma...
sandbox/linux/tests/main.cc:42: void UnitTestAssertHandler(const char* file, int
line, size_t message_start,
On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> This seems to still use the old signature.

Done.

[Paweł Hajdan Jr., 2017-02-14 19:23:26]

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_utils.h
File base/test/logging_utils.h (right):

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.h:18: ScopedLogAssertHandler();
On 2017/02/14 10:34:28, alex-ac wrote:
> On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> > Why do we need default ctor?
> It allows to use this object as an class variable and initialize it after
> execution of the constructor. (see base/test/test_suite.cc, where this object
is
> initialized in the Initialize method).

Why don't we use unique_ptr instead, or some similar solution which does not
require default ctor?

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.h:20: ScopedLogAssertHandler(ScopedLogAssertHandler&&
other);
On 2017/02/14 10:34:28, alex-ac wrote:
> On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> > Why do we need this and operator= ?
> Move-assignment operator allows to initialize default-constructed scoped
object.

To me it's another reason not to need these additions.

https://codereview.chromium.org/2638763004/diff/120001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/120001/base/test/test_suite.c...
base/test/test_suite.cc:304: printer_->OnAssert(file, line, summary, summary +
stack_trace);
On 2017/02/14 10:34:28, alex-ac wrote:
> On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> > Why do we pass "summary + stack_trace" instead of just stack_trace?
> XmlUnitTestResultPrinter inherits gtest format, where assert has summary and
> message. In GTest, summary is just a logged text, and message is a logged
text,
> concatenated with stack trace of assert. I think it's better to use same
format
> for DCHECK/CHECK.

Okay. Since this is not obvious, could you add a comment to the code?

[dcheng, 2017-02-14 23:59:26]

https://codereview.chromium.org/2638763004/diff/140001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2638763004/diff/140001/base/logging.cc#newcod...
base/logging.cc:130: };
Perhaps just use something from STL here? We can just leak it, but we shouldn't
be writing manual new / delete cleanup code.

My other concern is that because we have a manual push, it's possible to get
unpaired push and pops. Is there any way that we can make the logging handler
always scoped?

https://codereview.chromium.org/2638763004/diff/140001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2638763004/diff/140001/base/logging.h#newcode285
base/logging.h:285: LogAssertHandlerFunction;
It seems unnecessary to use base::Callback at the current time, so I'd recommend
keeping it as a function pointer.

Also, please pass the arguments by StringPiece here to avoid creating more
temporaries than necessary.

https://codereview.chromium.org/2638763004/diff/140001/base/logging.h#newcode290
base/logging.h:290: BASE_EXPORT void PopLogAssertHandler();
It seem like Pop and Log don't need to be exposed, so I'd prefer to hide them.

[alex-ac, 2017-03-09 14:15:53]

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_utils.h
File base/test/logging_utils.h (right):

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.h:18: ScopedLogAssertHandler();
On 2017/02/14 19:23:26, Paweł Hajdan Jr. wrote:
> On 2017/02/14 10:34:28, alex-ac wrote:
> > On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> > > Why do we need default ctor?
> > It allows to use this object as an class variable and initialize it after
> > execution of the constructor. (see base/test/test_suite.cc, where this
object
> is
> > initialized in the Initialize method).
> 
> Why don't we use unique_ptr instead, or some similar solution which does not
> require default ctor?

Done.

https://codereview.chromium.org/2638763004/diff/120001/base/test/logging_util...
base/test/logging_utils.h:20: ScopedLogAssertHandler(ScopedLogAssertHandler&&
other);
On 2017/02/14 19:23:26, Paweł Hajdan Jr. wrote:
> On 2017/02/14 10:34:28, alex-ac wrote:
> > On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> > > Why do we need this and operator= ?
> > Move-assignment operator allows to initialize default-constructed scoped
> object.
> 
> To me it's another reason not to need these additions.

Done.

https://codereview.chromium.org/2638763004/diff/120001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/120001/base/test/test_suite.c...
base/test/test_suite.cc:304: printer_->OnAssert(file, line, summary, summary +
stack_trace);
On 2017/02/14 19:23:26, Paweł Hajdan Jr. wrote:
> On 2017/02/14 10:34:28, alex-ac wrote:
> > On 2017/02/13 17:17:24, Paweł Hajdan Jr. wrote:
> > > Why do we pass "summary + stack_trace" instead of just stack_trace?
> > XmlUnitTestResultPrinter inherits gtest format, where assert has summary and
> > message. In GTest, summary is just a logged text, and message is a logged
> text,
> > concatenated with stack trace of assert. I think it's better to use same
> format
> > for DCHECK/CHECK.
> 
> Okay. Since this is not obvious, could you add a comment to the code?

Done.

https://codereview.chromium.org/2638763004/diff/140001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2638763004/diff/140001/base/logging.cc#newcod...
base/logging.cc:130: };
On 2017/02/14 23:59:26, dcheng wrote:
> Perhaps just use something from STL here? We can just leak it, but we
shouldn't
> be writing manual new / delete cleanup code.
> 
> My other concern is that because we have a manual push, it's possible to get
> unpaired push and pops. Is there any way that we can make the logging handler
> always scoped?

We can't use non-pod type for a global variable. It will not be destructed. So
we have to use new/delete here.

I found unpaired push/pop calls in the chrome/common/logging_chrome.cc, It's
used from the chromeos code to suppress default assert handler until end of the
process lifetime.

https://codereview.chromium.org/2638763004/diff/140001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2638763004/diff/140001/base/logging.h#newcode285
base/logging.h:285: LogAssertHandlerFunction;
On 2017/02/14 23:59:26, dcheng wrote:
> It seems unnecessary to use base::Callback at the current time, so I'd
recommend
> keeping it as a function pointer.
> 
> Also, please pass the arguments by StringPiece here to avoid creating more
> temporaries than necessary.

base::Callback is needed because there is class method used as log assert
handler in the base/test/test_suite.cc.

https://codereview.chromium.org/2638763004/diff/140001/base/logging.h#newcode290
base/logging.h:290: BASE_EXPORT void PopLogAssertHandler();
On 2017/02/14 23:59:26, dcheng wrote:
> It seem like Pop and Log don't need to be exposed, so I'd prefer to hide them.

Done.

[dcheng, 2017-03-09 19:41:02]

https://codereview.chromium.org/2638763004/diff/140001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2638763004/diff/140001/base/logging.cc#newcod...
base/logging.cc:130: };
On 2017/03/09 14:15:53, alex-ac wrote:
> On 2017/02/14 23:59:26, dcheng wrote:
> > Perhaps just use something from STL here? We can just leak it, but we
> shouldn't
> > be writing manual new / delete cleanup code.
> > 
> > My other concern is that because we have a manual push, it's possible to get
> > unpaired push and pops. Is there any way that we can make the logging
handler
> > always scoped?
> 
> We can't use non-pod type for a global variable. It will not be destructed. So
> we have to use new/delete here.
> 
> I found unpaired push/pop calls in the chrome/common/logging_chrome.cc, It's
> used from the chromeos code to suppress default assert handler until end of
the
> process lifetime.

It could just be a vector in a LazyInstance/function-local static.

I'd be OK with exposing a push-only API for the CrOS case.

https://codereview.chromium.org/2638763004/diff/140001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2638763004/diff/140001/base/logging.h#newcode285
base/logging.h:285: LogAssertHandlerFunction;
On 2017/03/09 14:15:53, alex-ac wrote:
> On 2017/02/14 23:59:26, dcheng wrote:
> > It seems unnecessary to use base::Callback at the current time, so I'd
> recommend
> > keeping it as a function pointer.
> > 
> > Also, please pass the arguments by StringPiece here to avoid creating more
> > temporaries than necessary.
> 
> base::Callback is needed because there is class method used as log assert
> handler in the base/test/test_suite.cc.

That method appears to be static.

[alex-ac, 2017-03-10 08:16:40]

On 2017/03/09 19:41:02, dcheng wrote:
> https://codereview.chromium.org/2638763004/diff/140001/base/logging.cc
> File base/logging.cc (right):
> 
>
https://codereview.chromium.org/2638763004/diff/140001/base/logging.cc#newcod...
> base/logging.cc:130: };
> On 2017/03/09 14:15:53, alex-ac wrote:
> > On 2017/02/14 23:59:26, dcheng wrote:
> > > Perhaps just use something from STL here? We can just leak it, but we
> > shouldn't
> > > be writing manual new / delete cleanup code.
> > > 
> > > My other concern is that because we have a manual push, it's possible to
get
> > > unpaired push and pops. Is there any way that we can make the logging
> handler
> > > always scoped?
> > 
> > We can't use non-pod type for a global variable. It will not be destructed.
So
> > we have to use new/delete here.
> > 
> > I found unpaired push/pop calls in the chrome/common/logging_chrome.cc, It's
> > used from the chromeos code to suppress default assert handler until end of
> the
> > process lifetime.
> 
> It could just be a vector in a LazyInstance/function-local static.
> 

OK, I changed it to LazyInstance and std::stack.

> I'd be OK with exposing a push-only API for the CrOS case.
> 
> https://codereview.chromium.org/2638763004/diff/140001/base/logging.h
> File base/logging.h (right):
> 
>
https://codereview.chromium.org/2638763004/diff/140001/base/logging.h#newcode285
> base/logging.h:285: LogAssertHandlerFunction;
> On 2017/03/09 14:15:53, alex-ac wrote:
> > On 2017/02/14 23:59:26, dcheng wrote:
> > > It seems unnecessary to use base::Callback at the current time, so I'd
> > recommend
> > > keeping it as a function pointer.
> > > 
> > > Also, please pass the arguments by StringPiece here to avoid creating more
> > > temporaries than necessary.
> > 
> > base::Callback is needed because there is class method used as log assert
> > handler in the base/test/test_suite.cc.
> 
> That method appears to be static.

Sorry, i forgot to remove old comment from that method. It isn't static anymore,
it uses field printer_ to transmit message and stack trace to the test launcher.
I removed that comment now.

[Paweł Hajdan Jr., 2017-03-13 15:49:50]

I think it's good, just one tiny thing left.

https://codereview.chromium.org/2638763004/diff/180001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2638763004/diff/180001/base/logging.h#newcode147
base/logging.h:147: template <typename STRING_TYPE>
For non-trivial types like StringPiece, I'd prefer we just #include the proper
header to avoid the forward declaration getting out of sync.

[alex-ac, 2017-03-14 10:23:14]

https://codereview.chromium.org/2638763004/diff/180001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2638763004/diff/180001/base/logging.h#newcode147
base/logging.h:147: template <typename STRING_TYPE>
On 2017/03/13 15:49:50, Paweł Hajdan Jr. wrote:
> For non-trivial types like StringPiece, I'd prefer we just #include the proper
> header to avoid the forward declaration getting out of sync.

base/strings/string_piece.h includes base/logging.h and uses DCHECK_IS_ON()
macro. If i include that file from here, it will be undefined before use.

I moved forward declaration to base/strings/string_piece_forward.h and included
it from here.

[Paweł Hajdan Jr., 2017-03-14 15:06:27]

LGTM

Nice work!

[alex-ac, 2017-03-14 15:24:21]

The CQ bit was checked by alex-ac@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 15:25:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 15:28:02]

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

[commit-bot: I haz the power, 2017-03-14 15:28:04]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alex-ac, 2017-03-14 15:41:49]

The CQ bit was checked by alex-ac@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 15:42:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 15:48:37]

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

[commit-bot: I haz the power, 2017-03-14 15:48:38]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alex-ac, 2017-03-14 16:19:53]

The CQ bit was checked by alex-ac@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 16:20:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 16:28:41]

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

[commit-bot: I haz the power, 2017-03-14 16:28:42]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[alex-ac, 2017-03-14 17:12:12]

The CQ bit was checked by alex-ac@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 17:12:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 17:33:15]

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

[commit-bot: I haz the power, 2017-03-14 17:33:17]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[alex-ac, 2017-03-14 18:06:17]

The CQ bit was checked by alex-ac@yandex-team.ru

[alex-ac, 2017-03-14 18:06:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/2638763004/#ps280001
(title: "Fix error on ios.")

[commit-bot: I haz the power, 2017-03-14 18:06:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 18:16:24]

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

[commit-bot: I haz the power, 2017-03-14 18:16:25]

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

[dcheng, 2017-03-15 09:57:24]

https://codereview.chromium.org/2638763004/diff/240001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2638763004/diff/240001/base/logging.h#newcode287
base/logging.h:287: LogAssertHandlerFunction;
Nit: prefer using LogAssertHandlerFunction = base::Callback<...>;

https://codereview.chromium.org/2638763004/diff/280001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2638763004/diff/280001/base/logging.cc#newcod...
base/logging.cc:546: size_t stack_start = stream_.str().length();
Can we use tellp to get the length and avoid double stringification?

https://codereview.chromium.org/2638763004/diff/280001/base/logging.cc#newcod...
base/logging.cc:760: const std::string& full_message = stream_.str();
Can this use str_newline to avoid creating an additional temporary?

https://codereview.chromium.org/2638763004/diff/280001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/280001/base/test/test_suite.c...
base/test/test_suite.cc:307: printer_->OnAssert(file, line, summary.as_string(),
Please convert the string once in a local and reference it here.

[alex-ac, 2017-03-21 12:37:12]

https://codereview.chromium.org/2638763004/diff/240001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2638763004/diff/240001/base/logging.h#newcode287
base/logging.h:287: LogAssertHandlerFunction;
On 2017/03/15 09:57:24, dcheng wrote:
> Nit: prefer using LogAssertHandlerFunction = base::Callback<...>;

Done.

https://codereview.chromium.org/2638763004/diff/280001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2638763004/diff/280001/base/logging.cc#newcod...
base/logging.cc:546: size_t stack_start = stream_.str().length();
On 2017/03/15 09:57:24, dcheng wrote:
> Can we use tellp to get the length and avoid double stringification?

Done.

https://codereview.chromium.org/2638763004/diff/280001/base/logging.cc#newcod...
base/logging.cc:760: const std::string& full_message = stream_.str();
On 2017/03/15 09:57:24, dcheng wrote:
> Can this use str_newline to avoid creating an additional temporary?

Done.

https://codereview.chromium.org/2638763004/diff/280001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/280001/base/test/test_suite.c...
base/test/test_suite.cc:307: printer_->OnAssert(file, line, summary.as_string(),
On 2017/03/15 09:57:24, dcheng wrote:
> Please convert the string once in a local and reference it here.

Done.

[alex-ac, 2017-03-21 12:39:28]

The CQ bit was checked by alex-ac@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 12:39:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 12:49:31]

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

[commit-bot: I haz the power, 2017-03-21 12:49:32]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alex-ac, 2017-03-21 12:54:36]

The CQ bit was checked by alex-ac@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 12:54:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 14:04:52]

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

[commit-bot: I haz the power, 2017-03-21 14:04:53]

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

[dcheng, 2017-03-22 09:27:25]

LGTM

https://codereview.chromium.org/2638763004/diff/320001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2638763004/diff/320001/base/logging.cc#newcode15
base/logging.cc:15: #include <windows.h>
Btw, it's not clear to me why this is needed now (or has it always been needed
and was just incorrectly omitted)?

https://codereview.chromium.org/2638763004/diff/320001/base/logging.cc#newcod...
base/logging.cc:456: log_assert_handler_stack.Get().push(handler);
Nit: std::move(), since we're passing Callback by value

https://codereview.chromium.org/2638763004/diff/320001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2638763004/diff/320001/base/logging.h#newcode286
base/logging.h:286: const base::StringPiece& message,
Nit: pass by value

[alex-ac, 2017-04-04 11:25:15]

https://codereview.chromium.org/2638763004/diff/320001/base/logging.cc
File base/logging.cc (right):

https://codereview.chromium.org/2638763004/diff/320001/base/logging.cc#newcode15
base/logging.cc:15: #include <windows.h>
On 2017/03/22 09:27:25, dcheng wrote:
> Btw, it's not clear to me why this is needed now (or has it always been needed
> and was just incorrectly omitted)?

This include was incorrectly ommited, because activity_tracker.h includes it. I
moved base/debug/activity_tracker.h down to the other includes and found that
windows.h is required for HANDLE type.

https://codereview.chromium.org/2638763004/diff/320001/base/logging.cc#newcod...
base/logging.cc:456: log_assert_handler_stack.Get().push(handler);
On 2017/03/22 09:27:25, dcheng wrote:
> Nit: std::move(), since we're passing Callback by value

Done.

https://codereview.chromium.org/2638763004/diff/320001/base/logging.h
File base/logging.h (right):

https://codereview.chromium.org/2638763004/diff/320001/base/logging.h#newcode286
base/logging.h:286: const base::StringPiece& message,
On 2017/03/22 09:27:25, dcheng wrote:
> Nit: pass by value

Done.

[alex-ac, 2017-04-04 11:25:36]

The CQ bit was checked by alex-ac@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 11:25:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 11:44:54]

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

[commit-bot: I haz the power, 2017-04-04 11:44:55]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[alex-ac, 2017-04-17 11:10:19]

The CQ bit was checked by alex-ac@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 11:10:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 11:32:15]

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

[commit-bot: I haz the power, 2017-04-17 11:32:16]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[alex-ac, 2017-04-17 18:47:57]

The CQ bit was checked by alex-ac@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 18:48:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alex-ac, 2017-04-17 19:38:20]

brettw, please take a look.

[commit-bot: I haz the power, 2017-04-17 20:30:56]

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

[commit-bot: I haz the power, 2017-04-17 20:30:57]

Dry run: This issue passed the CQ dry run.

[brettw, 2017-04-17 22:20:35]

This looks like a good improvement.

https://codereview.chromium.org/2638763004/diff/380001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/2638763004/diff/380001/base/logging_unittest....
base/logging_unittest.cc:40: void LogSink(const char* file,
Now that we have a stack, is there a test for the nested behavior of the assert
registration?

https://codereview.chromium.org/2638763004/diff/380001/base/strings/string_pi...
File base/strings/string_piece_forward.h (right):

https://codereview.chromium.org/2638763004/diff/380001/base/strings/string_pi...
base/strings/string_piece_forward.h:4: // Forward declaration of StringPiece
types from base/strings/string_piece.h
Can you put a blank line between the copyright and the description?

https://codereview.chromium.org/2638763004/diff/380001/base/test/gtest_xml_un...
File base/test/gtest_xml_unittest_result_printer.cc (right):

https://codereview.chromium.org/2638763004/diff/380001/base/test/gtest_xml_un...
base/test/gtest_xml_unittest_result_printer.cc:42: if (!output_file_)
This function seems to duplicate WriteTestPartResult for the most part. Is it
possible to de-dupe by either creating a TestPartResult here and calling the
other one, or having them both call a helper that takes everything as arguments?

https://codereview.chromium.org/2638763004/diff/380001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/380001/base/test/test_suite.c...
base/test/test_suite.cc:308: const std::string& summary_str =
summary.as_string();
The & on these lines is technically correct but weirds me out since StringPiece
always returns a new string by value. If you really like this, you can keep it
this way, but I'd remove the &.

[alex-ac, 2017-04-18 06:34:21]

https://codereview.chromium.org/2638763004/diff/380001/base/logging_unittest.cc
File base/logging_unittest.cc (right):

https://codereview.chromium.org/2638763004/diff/380001/base/logging_unittest....
base/logging_unittest.cc:40: void LogSink(const char* file,
On 2017/04/17 22:20:34, brettw (plz ping after 24h) wrote:
> Now that we have a stack, is there a test for the nested behavior of the
assert
> registration?

Done.

https://codereview.chromium.org/2638763004/diff/380001/base/strings/string_pi...
File base/strings/string_piece_forward.h (right):

https://codereview.chromium.org/2638763004/diff/380001/base/strings/string_pi...
base/strings/string_piece_forward.h:4: // Forward declaration of StringPiece
types from base/strings/string_piece.h
On 2017/04/17 22:20:34, brettw (plz ping after 24h) wrote:
> Can you put a blank line between the copyright and the description?

Done.

https://codereview.chromium.org/2638763004/diff/380001/base/test/gtest_xml_un...
File base/test/gtest_xml_unittest_result_printer.cc (right):

https://codereview.chromium.org/2638763004/diff/380001/base/test/gtest_xml_un...
base/test/gtest_xml_unittest_result_printer.cc:42: if (!output_file_)
On 2017/04/17 22:20:34, brettw (plz ping after 24h) wrote:
> This function seems to duplicate WriteTestPartResult for the most part. Is it
> possible to de-dupe by either creating a TestPartResult here and calling the
> other one, or having them both call a helper that takes everything as
arguments?

Done.

https://codereview.chromium.org/2638763004/diff/380001/base/test/test_suite.cc
File base/test/test_suite.cc (right):

https://codereview.chromium.org/2638763004/diff/380001/base/test/test_suite.c...
base/test/test_suite.cc:308: const std::string& summary_str =
summary.as_string();
On 2017/04/17 22:20:34, brettw (plz ping after 24h) wrote:
> The & on these lines is technically correct but weirds me out since
StringPiece
> always returns a new string by value. If you really like this, you can keep it
> this way, but I'd remove the &.

Done.

[brettw, 2017-04-18 20:36:59]

lgtm

[alex-ac, 2017-04-19 06:35:38]

The CQ bit was checked by alex-ac@yandex-team.ru

[alex-ac, 2017-04-19 06:35:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2638763004/#ps400001
(title: "Add test for assert handlers nesting.")

[commit-bot: I haz the power, 2017-04-19 06:36:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 08:33:58]

CQ is committing da patch.

Bot data: {"patchset_id": 400001, "attempt_start_ts": 1492583738736840,
"parent_rev": "5446c242afb1902d829c524c6cc96e5f95a6a904", "commit_rev":
"cc1bde6592ae5e0b5064e7680941569625db9d09"}

[commit-bot: I haz the power, 2017-04-19 08:34:09]

Description was changed from

==========
Report CHECK/DCHECK to test launcher summary output.

This CL is continuation of previous one:
https://codereview.chromium.org/2606153003/

BUG=
==========

to

==========
Report CHECK/DCHECK to test launcher summary output.

This CL is continuation of previous one:
https://codereview.chromium.org/2606153003/

BUG=

Review-Url: https://codereview.chromium.org/2638763004
Cr-Commit-Position: refs/heads/master@{#465531}
Committed:
https://chromium.googlesource.com/chromium/src/+/cc1bde6592ae5e0b5064e7680941...
==========

[commit-bot: I haz the power, 2017-04-19 08:34:14]

Committed patchset #21 (id:400001) as
https://chromium.googlesource.com/chromium/src/+/cc1bde6592ae5e0b5064e7680941...

[krasin1, 2017-04-20 17:39:51]

A revert of this CL (patchset #21 id:400001) has been created in
https://codereview.chromium.org/2828373002/ by krasin@chromium.org.

The reason for reverting is: Breaks 'LTO Linux' bot:
https://build.chromium.org/p/chromium.fyi/builders/LTO%20Linux/builds/2531

Quick investigation revealed, it's official / chrome branded related, not lto
related, as LoggingTest.NestedLogAssertHandlers test is failing even if
LinkTimeOptimization is explicitly turned off, like with the following gn flags:

$ gn gen out/off '--args=is_chrome_branded=true is_official_build=true
is_debug=false is_cfi=false use_thin_lto=false allow_posix_link_time_opt=false
use_lld=false use_goma=true' --check
.