Move ChromeNetLog to //components

Move ChromeNetLog to //components

BUG=508071
TBR=jochen

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

[droger, 2015-09-16 12:24:55]

Patchset #1 (id:1) has been deleted

[droger, 2015-09-16 12:41:28]

Patchset #1 (id:20001) has been deleted

[droger, 2015-09-16 14:10:28]

Patchset #1 (id:40001) has been deleted

[droger, 2015-09-16 14:10:35]

Patchset #1 (id:60001) has been deleted

[droger, 2015-09-16 14:10:43]

Patchset #1 (id:80001) has been deleted

[droger, 2015-09-16 14:11:15]

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

[mmenke, 2015-09-16 15:25:10]

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

[mmenke, 2015-09-16 15:25:11]

[+eroman]

I'm assuming NetLogTempFile is used on iOS, and it's not just the constants we
need from this CL?

https://codereview.chromium.org/1347043002/diff/100001/components/net_log/net...
File components/net_log/net_log_temp_file.cc (right):

https://codereview.chromium.org/1347043002/diff/100001/components/net_log/net...
components/net_log/net_log_temp_file.cc:125:
DCHECK(thread_checker_.CalledOnValidThread());
I don't think these thread checks are correct.  It's actually created on the UI
thread, when the ChromeNetLog is created, and it looks like thread checker binds
to the thread it's created on.

[droger, 2015-09-16 15:34:20]

Yes, I think it is needed for about://net-export.

https://codereview.chromium.org/1347043002/diff/100001/components/net_log/net...
File components/net_log/net_log_temp_file.cc (right):

https://codereview.chromium.org/1347043002/diff/100001/components/net_log/net...
components/net_log/net_log_temp_file.cc:125:
DCHECK(thread_checker_.CalledOnValidThread());
On 2015/09/16 15:25:11, mmenke wrote:
> I don't think these thread checks are correct.  It's actually created on the
UI
> thread, when the ChromeNetLog is created, and it looks like thread checker
binds
> to the thread it's created on.

Good catch. I added a DetachFromThread() here and updated some comments.

[eroman, 2015-09-16 20:52:21]

https://codereview.chromium.org/1347043002/diff/120001/chrome/browser/net/chr...
File chrome/browser/net/chrome_net_log_helper.cc (right):

https://codereview.chromium.org/1347043002/diff/120001/chrome/browser/net/chr...
chrome/browser/net/chrome_net_log_helper.cc:13:
*base::CommandLine::ForCurrentProcess();
Can you make this an input parameter to the function instead?
I think it makes the function invocation more readable too, since it is clear it
is doing command line parsing

https://codereview.chromium.org/1347043002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/net_export_ui.cc (right):

https://codereview.chromium.org/1347043002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/net_export_ui.cc:71: net_log::NetLogTempFile*
net_log_temp_file,
Observation: the distinction of the "net_log" namespace isn't entirely clear, in
that this is also a common name in the src/net/log directory. I don't have a
better suggestion though; this is still probably a good name for it.

https://codereview.chromium.org/1347043002/diff/120001/components/net_log.gypi
File components/net_log.gypi (right):

https://codereview.chromium.org/1347043002/diff/120001/components/net_log.gyp...
components/net_log.gypi:14: 'data_reduction_proxy_core_common',
Not sure this dependency is desirable, what is this for again?

https://codereview.chromium.org/1347043002/diff/120001/components/net_log/chr...
File components/net_log/chrome_net_log.cc (right):

https://codereview.chromium.org/1347043002/diff/120001/components/net_log/chr...
components/net_log/chrome_net_log.cc:88:
base::CommandLine::ForCurrentProcess()->GetCommandLineString());
Same comment as earlier: Could you make this a parameter to the function? Maybe
this is inconsistent since the version information is not an input, so feel free
to argue this.

[droger, 2015-09-17 08:39:11]

Thank you for the review.

https://codereview.chromium.org/1347043002/diff/120001/chrome/browser/net/chr...
File chrome/browser/net/chrome_net_log_helper.cc (right):

https://codereview.chromium.org/1347043002/diff/120001/chrome/browser/net/chr...
chrome/browser/net/chrome_net_log_helper.cc:13:
*base::CommandLine::ForCurrentProcess();
On 2015/09/16 20:52:21, eroman wrote:
> Can you make this an input parameter to the function instead?
> I think it makes the function invocation more readable too, since it is clear
it
> is doing command line parsing

Done.

https://codereview.chromium.org/1347043002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/net_export_ui.cc (right):

https://codereview.chromium.org/1347043002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/net_export_ui.cc:71: net_log::NetLogTempFile*
net_log_temp_file,
On 2015/09/16 20:52:21, eroman wrote:
> Observation: the distinction of the "net_log" namespace isn't entirely clear,
in
> that this is also a common name in the src/net/log directory. I don't have a
> better suggestion though; this is still probably a good name for it.

I need to componentize some code from net-export as well, and this code (in
particular the temp file log) is mainly/only used for net-export/net-internals
UI. There are no plans to componentize NetInternalsUI for now.

What about something like net_export::? Or net_debug::?

Otherwise I can suggest net_logging::, which is a bit more different from
net/log.

https://codereview.chromium.org/1347043002/diff/120001/components/net_log.gypi
File components/net_log.gypi (right):

https://codereview.chromium.org/1347043002/diff/120001/components/net_log.gyp...
components/net_log.gypi:14: 'data_reduction_proxy_core_common',
On 2015/09/16 20:52:21, eroman wrote:
> Not sure this dependency is desirable, what is this for again?

This is used in GetConstants(), that was in NetinternalsUI, but I moved in
ChromeNetLog.
See here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/1347043002/diff/120001/components/net_log/chr...
File components/net_log/chrome_net_log.cc (right):

https://codereview.chromium.org/1347043002/diff/120001/components/net_log/chr...
components/net_log/chrome_net_log.cc:88:
base::CommandLine::ForCurrentProcess()->GetCommandLineString());
On 2015/09/16 20:52:21, eroman wrote:
> Same comment as earlier: Could you make this a parameter to the function?
Maybe
> this is inconsistent since the version information is not an input, so feel
free
> to argue this.

I don't have a strong opinion. Done.

https://codereview.chromium.org/1347043002/diff/120001/components/net_log/chr...
components/net_log/chrome_net_log.cc:92:
data_reduction_proxy::DataReductionProxyEventStore::AddConstants(
Note: this is the dependency on DataReductionProxyEventStore.

[mmenke, 2015-09-17 15:50:51]

LGTM, just nits.

https://codereview.chromium.org/1347043002/diff/160001/components/net_log/chr...
File components/net_log/chrome_net_log.h (right):

https://codereview.chromium.org/1347043002/diff/160001/components/net_log/chr...
components/net_log/chrome_net_log.h:10: #include "base/command_line.h"
While you're here, mind including base/macros.h, for DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/1347043002/diff/160001/components/net_log/chr...
components/net_log/chrome_net_log.h:30: class ChromeNetLog : public net::NetLog
{
Hrm...I don't think we're in Chrome anymore.  Should this be renamed?  Suppose I
don't have any better suggestions (I think net_log::NetLog is a much worse
name).

https://codereview.chromium.org/1347043002/diff/160001/components/net_log/net...
File components/net_log/net_log_temp_file.h (right):

https://codereview.chromium.org/1347043002/diff/160001/components/net_log/net...
components/net_log/net_log_temp_file.h:167: std::string channel_string_;
const?

[droger, 2015-09-17 15:53:23]

https://codereview.chromium.org/1347043002/diff/160001/components/net_log/chr...
File components/net_log/chrome_net_log.h (right):

https://codereview.chromium.org/1347043002/diff/160001/components/net_log/chr...
components/net_log/chrome_net_log.h:30: class ChromeNetLog : public net::NetLog
{
On 2015/09/17 15:50:51, mmenke wrote:
> Hrm...I don't think we're in Chrome anymore.  Should this be renamed?  Suppose
I
> don't have any better suggestions (I think net_log::NetLog is a much worse
> name).

What about NetLogImpl?

[droger, 2015-09-17 16:00:36]

On 2015/09/17 15:53:23, droger wrote:
> What about NetLogImpl?

Collin argued that ChromeNetLog a good name, because it includes product
specific information (such as version info).
Besides this, we have many Google-specific code and resources in components.
Referencing Chrome from a component definitely doesn't break any rule.

[droger, 2015-09-18 08:27:58]

I'm going to keep ChromeNetLog and net_log because I don't really have better
ideas. Let me know if you have strong opinions or better suggestions.

https://codereview.chromium.org/1347043002/diff/160001/components/net_log/chr...
File components/net_log/chrome_net_log.h (right):

https://codereview.chromium.org/1347043002/diff/160001/components/net_log/chr...
components/net_log/chrome_net_log.h:10: #include "base/command_line.h"
On 2015/09/17 15:50:51, mmenke wrote:
> While you're here, mind including base/macros.h, for DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/1347043002/diff/160001/components/net_log/net...
File components/net_log/net_log_temp_file.h (right):

https://codereview.chromium.org/1347043002/diff/160001/components/net_log/net...
components/net_log/net_log_temp_file.h:167: std::string channel_string_;
On 2015/09/17 15:50:51, mmenke wrote:
> const?

Done.

[droger, 2015-09-18 09:48:59]

Barring any objections, I'll push this on Monday.

[eroman, 2015-09-18 19:57:13]

lgtm

[droger, 2015-09-21 12:52:24]

droger@chromium.org changed reviewers:
+ blundell@chromium.org

[droger, 2015-09-21 12:52:25]

+blundell for //components

[blundell, 2015-09-21 12:56:21]

lgtm

[droger, 2015-09-21 13:17:50]

droger@chromium.org changed reviewers:
+ jochen@chromium.org

[droger, 2015-09-21 13:18:16]

TBR=jochen for trivial namespace and #include changes in //chrome/browser.

[droger, 2015-09-21 13:18:42]

The CQ bit was checked by droger@chromium.org

[droger, 2015-09-21 13:18:42]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org,
eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1347043002/#ps220001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-21 13:18:49]

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

[commit-bot: I haz the power, 2015-09-21 14:29:25]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2015-09-21 14:30:09]

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