Moving ScopedMockLog from net/test to base/test.

Moving ScopedMockLog from net/test to base/test.

I want to reuse ScopedMockLog from unittests under src/remoting/host.
To do that, I am moving this class from net/test to base/test.

When doing the move, I also wanted to ensure thread-safety in the case when
logging is happening on a thread different from where StartCapturingLogs or
StopCapturingLogs are called.  Having proper locks (and memory barriers implied
by the locks) should ensure that 1) LogMessageHandler won't see a half-way
executed StartCapturingLogs or StopCapturingLogs and 2) that a log write
in-progress won't get a rug pulled from underneath by destroying of gMock's
structures embedded in ScopedMockLog's Log mock method.

BUG=
TEST=net_unittests

Committed: https://crrev.com/b4d01d31b4553b10aae55b66476e5ab9280b1c9d
Cr-Commit-Position: refs/heads/master@{#319105}

[Łukasz Anforowicz, 2015-03-02 23:07:03]

Patchset #1 (id:1) has been deleted

[Łukasz Anforowicz, 2015-03-02 23:07:13]

Patchset #1 (id:20001) has been deleted

[Łukasz Anforowicz, 2015-03-02 23:36:04]

Patchset #2 (id:60001) has been deleted

[Łukasz Anforowicz, 2015-03-02 23:39:32]

lukasza@chromium.org changed reviewers:
+ rvargas@chromium.org

[Łukasz Anforowicz, 2015-03-02 23:39:33]

Ricardo, could you please take a look?

My real goal is to be able to add tests with logs-based verification to
src/remoting/host/policy_watcher_unitest.cc (as you can see in Patchset#1).  I
plan to do that in a separate changelist - the current changelist sets the stage
for this by moving ScopedMockLog from net/test to base/test.

Thanks,

Lukasz

https://codereview.chromium.org/966423003/diff/80001/net/test/gtest_util.h
File net/test/gtest_util.h (right):

https://codereview.chromium.org/966423003/diff/80001/net/test/gtest_util.h#ne...
net/test/gtest_util.h:38: GTEST_CONCAT_TOKEN_(gtest_label_dfatal_, __LINE__) :
fail("")
Most of the churn above is caused by a mandatory "git cl format net".  The only
real change is the change of the namespace from ::net::test::ScopedMockLog to
::base::ScopedMockLog.

[rvargas (doing something else), 2015-03-03 19:45:34]

https://codereview.chromium.org/966423003/diff/80001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/966423003/diff/80001/base/base.gyp#newcode965
base/base.gyp:965: 'test/scoped_mock_log.h',
build.gn has to be updated

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_log.h
File base/test/scoped_mock_log.h (left):

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_lo...
base/test/scoped_mock_log.h:13: namespace test {
Almost all files on the new destination are outside of test::, but I lean
towards thinking that test stuff is different enough from normal code to justify
the extra namespace. Besides, looks like the last file added here has
base::test::. I think we should keep this namespace.

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

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_lo...
base/test/scoped_mock_log.h:40: class ScopedMockLog {
I generally don't like "Scoped" named classes unless they wrap another type for
the sole purpose of providing RAII semantics. That's not the case here so we
should drop that prefix.

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_lo...
base/test/scoped_mock_log.h:43: // If it were to start to capture logs, it could
be a problem if
nit: comments should wrap as close to the limit as possible (this file is not
following that).

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_lo...
base/test/scoped_mock_log.h:57: // StopCapturingLogs() at the same time.
This comment looks stale now.

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_lo...
base/test/scoped_mock_log.h:67: // Sets the Log Message Handler that gets passed
every log message before
nit: This is the documentation of logging::SetLogMessageHandler, which is
confusing given that this is a message handler.

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_lo...
base/test/scoped_mock_log.h:98: };
DISALLOW_COPY_AND_ASSIGN ?

[Łukasz Anforowicz, 2015-03-04 00:22:14]

Patchset #3 (id:100001) has been deleted

[Łukasz Anforowicz, 2015-03-04 00:22:25]

Patchset #3 (id:120001) has been deleted

[Łukasz Anforowicz, 2015-03-04 00:23:01]

Thanks for the feedback Ricardo.  I tried addressing it in patchset #3.

-Lukasz

https://codereview.chromium.org/966423003/diff/80001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/966423003/diff/80001/base/base.gyp#newcode965
base/base.gyp:965: 'test/scoped_mock_log.h',
On 2015/03/03 19:45:33, rvargas wrote:
> build.gn has to be updated

Thanks for catching this.  Done.

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_log.h
File base/test/scoped_mock_log.h (left):

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_lo...
base/test/scoped_mock_log.h:13: namespace test {
On 2015/03/03 19:45:34, rvargas wrote:
> Almost all files on the new destination are outside of test::, but I lean
> towards thinking that test stuff is different enough from normal code to
justify
> the extra namespace. Besides, looks like the last file added here has
> base::test::. I think we should keep this namespace.

Good point.  I was torn on this myself and ended up trying to be consistent. 
I've changed to base::test as suggested.

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

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_lo...
base/test/scoped_mock_log.h:40: class ScopedMockLog {
On 2015/03/03 19:45:34, rvargas wrote:
> I generally don't like "Scoped" named classes unless they wrap another type
for
> the sole purpose of providing RAII semantics. That's not the case here so we
> should drop that prefix.

Done.

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_lo...
base/test/scoped_mock_log.h:43: // If it were to start to capture logs, it could
be a problem if
On 2015/03/03 19:45:34, rvargas wrote:
> nit: comments should wrap as close to the limit as possible (this file is not
> following that).

I re-wrapped where it was saving lines and/or where I felt it was improving
readability.

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_lo...
base/test/scoped_mock_log.h:57: // StopCapturingLogs() at the same time.
On 2015/03/03 19:45:34, rvargas wrote:
> This comment looks stale now.

Oh, right.  Thanks.

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_lo...
base/test/scoped_mock_log.h:67: // Sets the Log Message Handler that gets passed
every log message before
On 2015/03/03 19:45:34, rvargas wrote:
> nit: This is the documentation of logging::SetLogMessageHandler, which is
> confusing given that this is a message handler.

Good catch.  I reworded the comment.

https://codereview.chromium.org/966423003/diff/80001/base/test/scoped_mock_lo...
base/test/scoped_mock_log.h:98: };
On 2015/03/03 19:45:34, rvargas wrote:
> DISALLOW_COPY_AND_ASSIGN ?

Done.

[rvargas (doing something else), 2015-03-04 17:36:55]

LGTM

[Łukasz Anforowicz, 2015-03-04 17:38:34]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2015-03-04 17:39:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/966423003/140001

[commit-bot: I haz the power, 2015-03-04 19:47:25]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2015-03-04 19:48:15]

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