Standardize "net" category trace events

Standardize "net" category trace events

Some "net" trace events are logged with "disabled-by-default-" prefix and
some are not. This CL tries to Standardize the usage.

BUG=669918
Committed: https://crrev.com/0b7f5b65efc8cfd59c04619e92a052dc2a0a965f
Cr-Commit-Position: refs/heads/master@{#436706}

[xunjieli, 2016-11-30 16:45:14]

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

[xunjieli, 2016-11-30 16:45:14]

Matt: mind taking a look?
I wanted to change this because "net" categories shows up in trace UI both in
"default categories" and "disabled by default categories". It is somewhat
confusing.

[xunjieli, 2016-11-30 16:45:19]

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

[commit-bot: I haz the power, 2016-11-30 16:45: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-11-30 17:43:36]

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

[commit-bot: I haz the power, 2016-11-30 17:43:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mmenke, 2016-12-01 16:16:14]

LGTM, modulo comment.

https://codereview.chromium.org/2533953005/diff/20001/net/log/trace_constants.h
File net/log/trace_constants.h (right):

https://codereview.chromium.org/2533953005/diff/20001/net/log/trace_constants...
net/log/trace_constants.h:5: #ifndef NET_LOG_TRACE_CONSTANTS_H_
We aren't consistent about it, but this should probably be in net/base (To avoid
circular dependencies, at the directory level.  We do have a lot of them
already, but no reason to add another unless we get something from it)

[xunjieli, 2016-12-01 23:38:12]

Thanks!

https://codereview.chromium.org/2533953005/diff/20001/net/log/trace_constants.h
File net/log/trace_constants.h (right):

https://codereview.chromium.org/2533953005/diff/20001/net/log/trace_constants...
net/log/trace_constants.h:5: #ifndef NET_LOG_TRACE_CONSTANTS_H_
On 2016/12/01 16:16:14, mmenke wrote:
> We aren't consistent about it, but this should probably be in net/base (To
avoid
> circular dependencies, at the directory level.  We do have a lot of them
> already, but no reason to add another unless we get something from it)

Done.

[xunjieli, 2016-12-01 23:38:17]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-12-01 23:38:18]

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

[commit-bot: I haz the power, 2016-12-01 23:39:11]

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-02 00:43:39]

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

[commit-bot: I haz the power, 2016-12-02 00:43:40]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[xunjieli, 2016-12-02 14:13:40]

Hi Matt,

Should I change "NET_EXPORT_PRIVATE extern const" to "NET_EXPORT extern const" ?
The constant is not used outside of net/, so I am not sure if I should change it
to NET_EXPORT. 

There is a linker error which complains:

E:/b/depot_tools/python276_bin/python.exe
../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x64 False
link.exe /nologo /IMPLIB:./net_with_v8.dll.lib /DLL /OUT:./net_with_v8.dll
/PDB:./net_with_v8.dll.pdb @./net_with_v8.dll.rsp
proxy_resolver_v8_tracing.obj : error LNK2019: unresolved external symbol "char
const * const net::kNetTracingCategory" (?kNetTracingCategory@net@@3QBDB)
referenced in function "private: int __cdecl net::`anonymous
namespace'::Job::ExecuteProxyResolver(void)"
(?ExecuteProxyResolver@Job@?A@net@@AEAAHXZ)
./net_with_v8.dll : fatal error LNK1120: 1 unresolved externals

[mmenke, 2016-12-02 15:03:45]

On 2016/12/02 14:13:40, xunjieli wrote:
> Hi Matt,
> 
> Should I change "NET_EXPORT_PRIVATE extern const" to "NET_EXPORT extern const"
?
> The constant is not used outside of net/, so I am not sure if I should change
it
> to NET_EXPORT. 
> 
> There is a linker error which complains:
> 
> E:/b/depot_tools/python276_bin/python.exe
> ../../build/toolchain/win/tool_wrapper.py link-wrapper environment.x64 False
> link.exe /nologo /IMPLIB:./net_with_v8.dll.lib /DLL /OUT:./net_with_v8.dll
> /PDB:./net_with_v8.dll.pdb @./net_with_v8.dll.rsp
> proxy_resolver_v8_tracing.obj : error LNK2019: unresolved external symbol
"char
> const * const net::kNetTracingCategory" (?kNetTracingCategory@net@@3QBDB)
> referenced in function "private: int __cdecl net::`anonymous
> namespace'::Job::ExecuteProxyResolver(void)"
> (?ExecuteProxyResolver@Job@?A@net@@AEAAHXZ)
> ./net_with_v8.dll : fatal error LNK1120: 1 unresolved externals

NET_EXPORT and NET_EXPORT_PRIVATE actually do the exact same thing, they're just
hints that code shouldn't be used outside of net/, so that won't help.  I'm not
sure why this isn't working.

[xunjieli, 2016-12-02 16:33:20]

Patchset #5 (id:80001) has been deleted

[eroman, 2016-12-02 17:37:52]

eroman@chromium.org changed reviewers:
+ eroman@chromium.org

[eroman, 2016-12-02 17:37:52]

Thinking out loud here (not that familiar with component build on windows):

Could it be that on windows it is building both net.dll and net_with_v8.dll?

In that case, kNetTracingCategory is exported from net.dll.

The complaint is that kNetTracingCategory cannot be found by
Job::ExecuteProxyResolver. This latter function is compiled as part of
net_with_v8.dll (proxy_resolver_v8_tracing.cc) and not net.dll.

So when including the constants file that has the NET_EXPORT,
proxy_resolver_v8_tracing.cc would in effect be looking for the symbol inside of
net_with_v8.dll and not net.dll.

This seems weird, I would have thought net_with_v8.dll was supposed to include
all of net. But it would explain the error...

[eroman, 2016-12-02 17:44:45]

hm, but if that were the case other stuff would be broken too, so probably
doesn't make sense

[xunjieli, 2016-12-02 20:37:44]

Patchset #5 (id:100001) has been deleted

[xunjieli, 2016-12-02 22:30:38]

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

[commit-bot: I haz the power, 2016-12-02 22:31:30]

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-02 22:35:53]

https://codereview.chromium.org/2533953005/diff/120001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2533953005/diff/120001/net/BUILD.gn#newcode89
net/BUILD.gn:89: source_set("constants") {
Thanks so much for the ideas! I was able to reproduce this on my windows
machine. If I try to use net/base/net_errors.h in proxy_resolver_v8_tracing.cc,
I will get the same linker error.

I searched on chromium-dev, it seems that the way to solve this in GN is to have
a separate target either as a source_set or static_library for the
constants(e.g. common:constants, chrome_elf:constants,
remoting/host/mac:constants).

I have updated the CL. Please take a look at the diff between PS#4 and PS#5.

[commit-bot: I haz the power, 2016-12-03 00:24:08]

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

[commit-bot: I haz the power, 2016-12-03 00:24:09]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-12-06 15:19:01]

On 2016/12/02 22:35:53, xunjieli wrote:
> https://codereview.chromium.org/2533953005/diff/120001/net/BUILD.gn
> File net/BUILD.gn (right):
> 
> https://codereview.chromium.org/2533953005/diff/120001/net/BUILD.gn#newcode89
> net/BUILD.gn:89: source_set("constants") {
> Thanks so much for the ideas! I was able to reproduce this on my windows
> machine. If I try to use net/base/net_errors.h in
proxy_resolver_v8_tracing.cc,
> I will get the same linker error.
> 
> I searched on chromium-dev, it seems that the way to solve this in GN is to
have
> a separate target either as a source_set or static_library for the
> constants(e.g. common:constants, chrome_elf:constants,
> remoting/host/mac:constants).
> 
> I have updated the CL. Please take a look at the diff between PS#4 and PS#5.

Matt, could you take an another look? I made some change in the BUILD.gn file.
Thanks!

[mmenke, 2016-12-06 15:34:59]

On 2016/12/06 15:19:01, xunjieli wrote:
> On 2016/12/02 22:35:53, xunjieli wrote:
> > https://codereview.chromium.org/2533953005/diff/120001/net/BUILD.gn
> > File net/BUILD.gn (right):
> > 
> >
https://codereview.chromium.org/2533953005/diff/120001/net/BUILD.gn#newcode89
> > net/BUILD.gn:89: source_set("constants") {
> > Thanks so much for the ideas! I was able to reproduce this on my windows
> > machine. If I try to use net/base/net_errors.h in
> proxy_resolver_v8_tracing.cc,
> > I will get the same linker error.
> > 
> > I searched on chromium-dev, it seems that the way to solve this in GN is to
> have
> > a separate target either as a source_set or static_library for the
> > constants(e.g. common:constants, chrome_elf:constants,
> > remoting/host/mac:constants).
> > 
> > I have updated the CL. Please take a look at the diff between PS#4 and PS#5.
> 
> Matt, could you take an another look? I made some change in the BUILD.gn file.
> Thanks!

LGTM.  I wonder if the issue is related to it being an array with a length that
can't be determined from the header.

[xunjieli, 2016-12-06 15:51:09]

On 2016/12/06 15:34:59, mmenke wrote:
> On 2016/12/06 15:19:01, xunjieli wrote:
> > On 2016/12/02 22:35:53, xunjieli wrote:
> > > https://codereview.chromium.org/2533953005/diff/120001/net/BUILD.gn
> > > File net/BUILD.gn (right):
> > > 
> > >
> https://codereview.chromium.org/2533953005/diff/120001/net/BUILD.gn#newcode89
> > > net/BUILD.gn:89: source_set("constants") {
> > > Thanks so much for the ideas! I was able to reproduce this on my windows
> > > machine. If I try to use net/base/net_errors.h in
> > proxy_resolver_v8_tracing.cc,
> > > I will get the same linker error.
> > > 
> > > I searched on chromium-dev, it seems that the way to solve this in GN is
to
> > have
> > > a separate target either as a source_set or static_library for the
> > > constants(e.g. common:constants, chrome_elf:constants,
> > > remoting/host/mac:constants).
> > > 
> > > I have updated the CL. Please take a look at the diff between PS#4 and
PS#5.
> > 
> > Matt, could you take an another look? I made some change in the BUILD.gn
file.
> > Thanks!
> 
> LGTM.  I wonder if the issue is related to it being an array with a length
that
> can't be determined from the header.

Same thing happens if I use a char* instead of a char[], so I am not sure if
that's related..

[xunjieli, 2016-12-06 15:51:16]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2016-12-06 15:51:32]

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-06 17:16:24]

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

[commit-bot: I haz the power, 2016-12-06 17:16:25]

Failed to apply patch for net/quic/chromium/quic_stream_factory.cc:
While running git apply --index -p1;
  error: patch failed: net/quic/chromium/quic_stream_factory.cc:1617
  error: net/quic/chromium/quic_stream_factory.cc: patch does not apply

Patch:       net/quic/chromium/quic_stream_factory.cc
Index: net/quic/chromium/quic_stream_factory.cc
diff --git a/net/quic/chromium/quic_stream_factory.cc
b/net/quic/chromium/quic_stream_factory.cc
index
d7cc346de9363a753f88a469d51a8e862770d6e4..82b46af1c1025a1967e94a956289841a1d288e32
100644
--- a/net/quic/chromium/quic_stream_factory.cc
+++ b/net/quic/chromium/quic_stream_factory.cc
@@ -25,6 +25,7 @@
 #include "net/base/ip_address.h"
 #include "net/base/net_errors.h"
 #include "net/base/proxy_delegate.h"
+#include "net/base/trace_constants.h"
 #include "net/cert/cert_verifier.h"
 #include "net/cert/ct_verifier.h"
 #include "net/dns/host_resolver.h"
@@ -402,7 +403,7 @@ int QuicStreamFactory::Job::Run(const CompletionCallback&
callback) {
 }
 
 int QuicStreamFactory::Job::DoLoop(int rv) {
-  TRACE_EVENT0("net", "QuicStreamFactory::Job::DoLoop");
+  TRACE_EVENT0(kNetTracingCategory, "QuicStreamFactory::Job::DoLoop");
   do {
     IoState state = io_state_;
     io_state_ = STATE_NONE;
@@ -1617,7 +1618,7 @@ int QuicStreamFactory::CreateSession(
 
     need_to_evaluate_consecutive_disabled_count_ = false;
   }
-  TRACE_EVENT0("net", "QuicStreamFactory::CreateSession");
+  TRACE_EVENT0(kNetTracingCategory, "QuicStreamFactory::CreateSession");
   IPEndPoint addr = *address_list.begin();
   bool enable_port_selection = enable_port_selection_;
   if (enable_port_selection && base::ContainsKey(gone_away_aliases_, key)) {

[xunjieli, 2016-12-06 18:45:11]

The CQ bit was checked by xunjieli@chromium.org

[xunjieli, 2016-12-06 18:45:12]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/2533953005/#ps140001
(title: "Rebased")

[commit-bot: I haz the power, 2016-12-06 18:45:35]

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-06 20:43:52]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481049911717000,
"parent_rev": "b23a014d1a21fa256f4c6b3e85f4d2044270b929", "commit_rev":
"43e33d902224447f1164aef7bccfb0f04f139f5e"}

[commit-bot: I haz the power, 2016-12-06 20:44:19]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-12-06 20:46:42]

Description was changed from

==========
Standardize "net" category trace events

Some "net" trace events are logged with "disabled-by-default-" prefix and
some are not. This CL tries to Standardize the usage.

BUG=669918
==========

to

==========
Standardize "net" category trace events

Some "net" trace events are logged with "disabled-by-default-" prefix and
some are not. This CL tries to Standardize the usage.

BUG=669918

Committed: https://crrev.com/0b7f5b65efc8cfd59c04619e92a052dc2a0a965f
Cr-Commit-Position: refs/heads/master@{#436706}
==========

[commit-bot: I haz the power, 2016-12-06 20:46:44]

Patchset 6 (id:??) landed as
https://crrev.com/0b7f5b65efc8cfd59c04619e92a052dc2a0a965f
Cr-Commit-Position: refs/heads/master@{#436706}