netlog: Don't enable recording if the "netlog" tracing category is disabled.

netlog: Don't enable recording if the "netlog" tracing category is disabled.

NetLog enables recording whenever an observer is
registered. TraceNetLogObserver would always register an observer, even
if the "netlog" tracing category is disabled. This is wasteful, since in
this case the trace events are dropped by TraceLog.

This CL only registers the observer when the category is enabled.

BUG=670732
Committed: https://crrev.com/be0aa5179a641f9b087b119177be0ea26472358f
Cr-Commit-Position: refs/heads/master@{#436368}

[Benoit L, 2016-12-05 11:01:12]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 11:01:43]

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, 2016-12-05 13:00:36]

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

[commit-bot: I haz the power, 2016-12-05 13:00:37]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2016-12-05 14:22:26]

lizeb@chromium.org changed reviewers:
+ xunjieli@chromium.org

[Benoit L, 2016-12-05 14:22:26]

Hi xunjieli@,

Here is a CL to disable netlog when the matching tracing category is not
enabled.

I'm not entirely sure of the best way to proceed here:
- Testing is tricky without adding test-specific members in either NetLog or
TraceNetLogObserver, since NetLog always has an observer in these tests, so
NetLog::IsRecording() is not useful.
- The comment in StopWatchForTraceStart() could be exchanged for a bool member
to track the current state.

wdyt?

[xunjieli, 2016-12-05 14:55:01]

https://codereview.chromium.org/2554483002/diff/1/net/log/trace_net_log_obser...
File net/log/trace_net_log_observer_unittest.cc (right):

https://codereview.chromium.org/2554483002/diff/1/net/log/trace_net_log_obser...
net/log/trace_net_log_observer_unittest.cc:350: TEST_F(TraceNetLogObserverTest,
DisabledCategoryDoesntEnableNetLog) {
Thanks for working on this. The change looks good. 
We should test whether NetLog::IsCapturing() is set accordingly. If
IsCapturing(), it means that we have at least one observer. However, since the
TraceNetLogObserverTest fixture always has a TestNetLog so it will always have
an observer.

Could you use TEST instead of TEST_P (to avoid instantiating a TestNetLog) and
do something along the following lines?

TEST(TraceNetLogEnableTest, DisabledCategory) {
  TraceNetLogObserver observer;
  NetLog net_log;  
  observer.WatchForTraceStart(&net_log);

  EXPECT_FALSE(net_log.IsCapturing());
  std::string disabled_netlog_category =
      std::string("-") + kNetLogTracingCategory;
  TraceLog::GetInstance()->SetEnabled(
      base::trace_event::TraceConfig(disabled_netlog_category, ""),
      TraceLog::RECORDING_MODE);
  net_log.AddGlobalEntry(NetLogEventType::REQUEST_ALIVE);

  EXPECT_FALSE(net_log.IsCapturing());

  observer.StopWatchForTraceStart();

  EXPECT_FALSE(net_log.IsCapturing());
}


And another test where netlog category is enabled and we have
net_log.IsCapturing() equals to true before StopWatchForTraceStart().

[Benoit L, 2016-12-05 15:45:27]

Thanks for the suggestion!
Done.

[Benoit L, 2016-12-05 15:45:35]

The CQ bit was checked by lizeb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 15:45:59]

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

[xunjieli, 2016-12-05 15:57:04]

lgtm mod comment below.

https://codereview.chromium.org/2554483002/diff/20001/net/log/trace_net_log_o...
File net/log/trace_net_log_observer_unittest.cc (right):

https://codereview.chromium.org/2554483002/diff/20001/net/log/trace_net_log_o...
net/log/trace_net_log_observer_unittest.cc:354: // and TEST_F() is not allowed.
This is okay. But alternatively you can use TEST instead of TEST_F under a
different test name. You can have different test names in a single unittest.cc
file. I think it's slightly cleaner that way and the tests are easier to read.

e.g. TEST(TraceNetLogCategoryTest, DisabledCategory)
and TEST(TraceNetLogCategoryTest, EnabledCategory)

[xunjieli, 2016-12-05 16:00:00]

https://codereview.chromium.org/2554483002/diff/20001/net/log/trace_net_log_o...
File net/log/trace_net_log_observer_unittest.cc (right):

https://codereview.chromium.org/2554483002/diff/20001/net/log/trace_net_log_o...
net/log/trace_net_log_observer_unittest.cc:354: // and TEST_F() is not allowed.
On 2016/12/05 15:57:04, xunjieli wrote:
> This is okay. But alternatively you can use TEST instead of TEST_F under a
> different test name. You can have different test names in a single unittest.cc
> file. I think it's slightly cleaner that way and the tests are easier to read.
> 
> e.g. TEST(TraceNetLogCategoryTest, DisabledCategory)
> and TEST(TraceNetLogCategoryTest, EnabledCategory)

And move these two tests either to the top or after the tests with TEST_F.

[Benoit L, 2016-12-05 16:44:25]

Thanks!

https://codereview.chromium.org/2554483002/diff/20001/net/log/trace_net_log_o...
File net/log/trace_net_log_observer_unittest.cc (right):

https://codereview.chromium.org/2554483002/diff/20001/net/log/trace_net_log_o...
net/log/trace_net_log_observer_unittest.cc:354: // and TEST_F() is not allowed.
On 2016/12/05 16:00:00, xunjieli wrote:
> On 2016/12/05 15:57:04, xunjieli wrote:
> > This is okay. But alternatively you can use TEST instead of TEST_F under a
> > different test name. You can have different test names in a single
unittest.cc
> > file. I think it's slightly cleaner that way and the tests are easier to
read.
> > 
> > e.g. TEST(TraceNetLogCategoryTest, DisabledCategory)
> > and TEST(TraceNetLogCategoryTest, EnabledCategory)
> 
> And move these two tests either to the top or after the tests with TEST_F.

Done.

https://codereview.chromium.org/2554483002/diff/20001/net/log/trace_net_log_o...
net/log/trace_net_log_observer_unittest.cc:354: // and TEST_F() is not allowed.
On 2016/12/05 15:57:04, xunjieli wrote:
> This is okay. But alternatively you can use TEST instead of TEST_F under a
> different test name. You can have different test names in a single unittest.cc
> file. I think it's slightly cleaner that way and the tests are easier to read.
> 
> e.g. TEST(TraceNetLogCategoryTest, DisabledCategory)
> and TEST(TraceNetLogCategoryTest, EnabledCategory)

Done.

[Benoit L, 2016-12-05 16:44:46]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2016-12-05 16:44:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/2554483002/#ps40001
(title: "Address comment.")

[commit-bot: I haz the power, 2016-12-05 16:45:10]

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, 2016-12-05 18:57:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1480956286035300,
"parent_rev": "81a78d2d0ff1cd04fbabe491e8082ef4928f3294", "commit_rev":
"5e94d0b1f49c8a5bb5596206cde7bf9f9dfd0b07"}

[commit-bot: I haz the power, 2016-12-05 18:58:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-05 18:59:51]

Description was changed from

==========
netlog: Don't enable recording if the "netlog" tracing category is disabled.

NetLog enables recording whenever an observer is
registered. TraceNetLogObserver would always register an observer, even
if the "netlog" tracing category is disabled. This is wasteful, since in
this case the trace events are dropped by TraceLog.

This CL only registers the observer when the category is enabled.

BUG=670732
==========

to

==========
netlog: Don't enable recording if the "netlog" tracing category is disabled.

NetLog enables recording whenever an observer is
registered. TraceNetLogObserver would always register an observer, even
if the "netlog" tracing category is disabled. This is wasteful, since in
this case the trace events are dropped by TraceLog.

This CL only registers the observer when the category is enabled.

BUG=670732

Committed: https://crrev.com/be0aa5179a641f9b087b119177be0ea26472358f
Cr-Commit-Position: refs/heads/master@{#436368}
==========

[commit-bot: I haz the power, 2016-12-05 18:59:52]

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