Restore ScopedLogger and add documentation.

Restore ScopedLogger and add documentation.

ScopedLogger was removed in http://crrev.com/381906.

The original review is on http://crrev.com/1379443002.

Committed: https://crrev.com/5eacfac3760f625103d79da1b63d02df4766c270
Cr-Commit-Position: refs/heads/master@{#383893}

[skobes, 2016-03-28 15:38:46]

Description was changed from

==========
Restore ScopedLogger.

This was removed in r381906.
==========

to

==========
Restore ScopedLogger.

This was removed in r381906.

The original review is on http://crrev.com/1379443002.
==========

[skobes, 2016-03-28 15:39:00]

Description was changed from

==========
Restore ScopedLogger.

This was removed in r381906.

The original review is on http://crrev.com/1379443002.
==========

to

==========
Restore ScopedLogger.

This was removed in http://crrev.com/381906.

The original review is on http://crrev.com/1379443002.
==========

[skobes, 2016-03-28 15:39:36]

skobes@chromium.org changed reviewers:
+ esprehn@chromium.org, tkent@chromium.org

[skobes, 2016-03-28 23:12:14]

I added a wiki on ScopedLogger:

http://dev.chromium.org/developers/how-tos/scopedlogger

It is linked from the "Blink" section of the main developer page.

[tkent, 2016-03-28 23:16:30]

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/Assertions.cpp (right):

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Assertions.cpp:236:
ScopedLogger::ScopedLogger(bool condition, const char* format, ...)
The code should be wrapped with |namespace WTF|.

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/Assertions.h (right):

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Assertions.h:96: // the output, particularly when
instrumenting recursive functions.
Please add a comment that this class is not used in checked-in code, but we'd
like to keep it.
Without it, someonw will remove ScopedLogger again.

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Assertions.h:108: friend class AssertionsTest;
Please use FRIEND_TEST_ALL_PREFIXES defined in base/gtest_prod_util.h.

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Assertions.h:123: #if LOG_DISABLED
We'd like to remove LOG_DISABLED macro.  Can you make it !ENABLE(ASSERT)?

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Assertions.h:128: #define
WTF_CREATE_SCOPED_LOGGER(name, ...) ScopedLogger name(true, __VA_ARGS__)
ScopedLogger class should be declared here to avoid to ship it in production.

[esprehn, 2016-03-28 23:16:48]

Can you link the wiki in the commit message?

I'm fine with this, lgtm. Are you okay with this tkent?

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/Assertions.cpp (right):

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Assertions.cpp:234: static const char
kScopedLoggerIndent[] = "  ";
can we just add a method called printIndent() that does this?

[tkent, 2016-03-28 23:18:18]

On 2016/03/28 at 23:12:14, skobes wrote:
> I added a wiki on ScopedLogger:
> 
> http://dev.chromium.org/developers/how-tos/scopedlogger
> 
> It is linked from the "Blink" section of the main developer page.


Wiki is not a good place to document it.  We should have a checked-in document
like wtf/Assertions.md.

[esprehn, 2016-03-28 23:30:10]

On 2016/03/28 at 23:18:18, tkent wrote:
> On 2016/03/28 at 23:12:14, skobes wrote:
> > I added a wiki on ScopedLogger:
> > 
> > http://dev.chromium.org/developers/how-tos/scopedlogger
> > 
> > It is linked from the "Blink" section of the main developer page.
> 
> 
> Wiki is not a good place to document it.  We should have a checked-in document
like wtf/Assertions.md.

Ah yeah, that is better.

[jbroman, 2016-03-29 16:59:06]

By the way, since Blink is in the process of migrating away from printf-style
logging toward stream-style logging, should this be migrated to expose a stream
instead of printf syntax in the future?

[skobes, 2016-03-29 18:19:40]

Description was changed from

==========
Restore ScopedLogger.

This was removed in http://crrev.com/381906.

The original review is on http://crrev.com/1379443002.
==========

to

==========
Restore ScopedLogger and add documentation.

ScopedLogger was removed in http://crrev.com/381906.

The original review is on http://crrev.com/1379443002.
==========

[skobes, 2016-03-29 18:23:23]

Patchset #2 (id:20001) has been deleted

[skobes, 2016-03-29 18:23:40]

On 2016/03/28 23:18:18, tkent wrote:
> Wiki is not a good place to document it.  We should have a checked-in document
> like wtf/Assertions.md.

Done.

On 2016/03/29 16:59:06, jbroman wrote:
> By the way, since Blink is in the process of migrating away from printf-style
> logging toward stream-style logging, should this be migrated to expose a
stream
> instead of printf syntax in the future?

SGTM.

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/Assertions.cpp (right):

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Assertions.cpp:234: static const char
kScopedLoggerIndent[] = "  ";
On 2016/03/28 23:16:48, esprehn wrote:
> can we just add a method called printIndent() that does this?

Done.

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Assertions.cpp:236:
ScopedLogger::ScopedLogger(bool condition, const char* format, ...)
On 2016/03/28 23:16:30, tkent wrote:
> The code should be wrapped with |namespace WTF|.

Done.

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/Assertions.h (right):

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Assertions.h:96: // the output, particularly when
instrumenting recursive functions.
On 2016/03/28 23:16:30, tkent wrote:
> Please add a comment that this class is not used in checked-in code, but we'd
> like to keep it.
> Without it, someonw will remove ScopedLogger again.
> 

Done.

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Assertions.h:108: friend class AssertionsTest;
On 2016/03/28 23:16:30, tkent wrote:
> Please use FRIEND_TEST_ALL_PREFIXES defined in base/gtest_prod_util.h.

Done.

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Assertions.h:123: #if LOG_DISABLED
On 2016/03/28 23:16:30, tkent wrote:
> We'd like to remove LOG_DISABLED macro.  Can you make it !ENABLE(ASSERT)?
> 

That breaks blink_logging_always_on.

https://codereview.chromium.org/1840693002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/Assertions.h:128: #define
WTF_CREATE_SCOPED_LOGGER(name, ...) ScopedLogger name(true, __VA_ARGS__)
On 2016/03/28 23:16:30, tkent wrote:
> ScopedLogger class should be declared here to avoid to ship it in production.

Done.

[tkent, 2016-03-30 00:09:50]

lgtm

[skobes, 2016-03-30 01:18:35]

The CQ bit was checked by skobes@chromium.org

[skobes, 2016-03-30 01:18:36]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1840693002/#ps40001
(title: "address review comments")

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

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

[commit-bot: I haz the power, 2016-03-30 01:25:14]

Description was changed from

==========
Restore ScopedLogger and add documentation.

ScopedLogger was removed in http://crrev.com/381906.

The original review is on http://crrev.com/1379443002.
==========

to

==========
Restore ScopedLogger and add documentation.

ScopedLogger was removed in http://crrev.com/381906.

The original review is on http://crrev.com/1379443002.
==========

[commit-bot: I haz the power, 2016-03-30 01:25:15]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-03-30 01:26:55]

Description was changed from

==========
Restore ScopedLogger and add documentation.

ScopedLogger was removed in http://crrev.com/381906.

The original review is on http://crrev.com/1379443002.
==========

to

==========
Restore ScopedLogger and add documentation.

ScopedLogger was removed in http://crrev.com/381906.

The original review is on http://crrev.com/1379443002.

Committed: https://crrev.com/5eacfac3760f625103d79da1b63d02df4766c270
Cr-Commit-Position: refs/heads/master@{#383893}
==========

[commit-bot: I haz the power, 2016-03-30 01:26:56]

Patchset 2 (id:??) landed as
https://crrev.com/5eacfac3760f625103d79da1b63d02df4766c270
Cr-Commit-Position: refs/heads/master@{#383893}