Refactor NetLog::LogLevel --> NetLogCaptureMode.

Refactor NetLog::LogLevel --> NetLogCaptureMode.

This is both a rename, and a functional change.

Before LogLevel was an enum, and consumers would compare its numeric value, or use it for serialization purposes. Whereas NetLogCaptureMode is a class that encapsulates the various capabilities for a logging level.

BUG=472710
TBR=asanka@chromium.org,sergeyu@chromium.org,yurys@chromium.org,bengr@chromium.org,asargent@chromium.org,zea@chromium.org,scherkus@chromium.org

Committed: https://crrev.com/001c374c5356777641161bb11b8dd792508381ca
Cr-Commit-Position: refs/heads/master@{#326440}

[eroman, 2015-04-03 20:26:58]

Patchset #2 (id:20001) has been deleted

[eroman, 2015-04-03 20:27:03]

Patchset #1 (id:1) has been deleted

[eroman, 2015-04-08 23:49:46]

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

[eroman, 2015-04-09 00:08:20]

This is mostly simple mechanical changes.

I have added some comments on more interesting areas of change to help focus
your review.

https://codereview.chromium.org/1059843002/diff/140001/chrome/browser/net/chr...
File chrome/browser/net/chrome_net_log.cc (right):

https://codereview.chromium.org/1059843002/diff/140001/chrome/browser/net/chr...
chrome/browser/net/chrome_net_log.cc:54: command_line_log_level >=
net::NetLogCaptureMode::NONE &&
Changes here

https://codereview.chromium.org/1059843002/diff/140001/net/disk_cache/blockfi...
File net/disk_cache/blockfile/sparse_control.cc (right):

https://codereview.chromium.org/1059843002/diff/140001/net/disk_cache/blockfi...
net/disk_cache/blockfile/sparse_control.cc:164: net::NetLog::EventType
event_type;
changed this

https://codereview.chromium.org/1059843002/diff/140001/net/http/http_log_util.cc
File net/http/http_log_util.cc (right):

https://codereview.chromium.org/1059843002/diff/140001/net/http/http_log_util...
net/http/http_log_util.cc:43: if (redact_begin == redact_end &&
!capture_mode.include_private_data()) {
change here

https://codereview.chromium.org/1059843002/diff/140001/net/log/net_log.cc
File net/log/net_log.cc (right):

https://codereview.chromium.org/1059843002/diff/140001/net/log/net_log.cc#new...
net/log/net_log.cc:271: // Accumulate the capture mode of all the observers to
find the maximum level.
changes here

https://codereview.chromium.org/1059843002/diff/140001/net/log/net_log.h
File net/log/net_log.h (left):

https://codereview.chromium.org/1059843002/diff/140001/net/log/net_log.h#oldc...
net/log/net_log.h:69: // Specifies the granularity of events that should be
emitted to the log.
changes in this file

https://codereview.chromium.org/1059843002/diff/140001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/140001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:22: namespace net {
changes here

[eroman, 2015-04-09 01:11:27]

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log.cc
File net/log/net_log.cc (right):

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log.cc#new...
net/log/net_log.cc:201: : last_id_(0),
effective_capture_mode_level_(NetLogCaptureMode::NONE) {
Is it legal to initialize an atomic int in this manner, or must it be set using
NoBarrier_Store ?
Guess I will look into that tomorrow...

[mmenke, 2015-04-09 15:47:12]

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log.cc
File net/log/net_log.cc (right):

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log.cc#new...
net/log/net_log.cc:201: : last_id_(0),
effective_capture_mode_level_(NetLogCaptureMode::NONE) {
On 2015/04/09 01:11:27, eroman wrote:
> Is it legal to initialize an atomic int in this manner, or must it be set
using
> NoBarrier_Store ?
> Guess I will look into that tomorrow...

It won't lead to bugs (Since we can only pass the NetLog to other threads with a
post message, or have other threads read the NetLog with a mutex or similar,
both of which include memory barriers, and can only be done strictly after
NetLog construction).  Whether it's legal or not, I couldn't tell you.  Ask a
lawyer?

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log.cc#new...
net/log/net_log.cc:280: new_capture_mode.level());
Should we really be storing the raw level here?  Seems like we should instead
have our own NetLogCaptureMode.  Suppose doing that safely does get hairy, but I
thought nothing should be using raw log levels?

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:49: };
Are these just public for net-internals?

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:62: void AddMode(NetLogCaptureMode mode) {
Think an "Add" method that works like max is a little weird.

Suggest making this something like:

static NetLogCaptureMode max(NetLogCaptureMode mode1, NetLogCaptureMode mode2) {
  mode1.level_ >= mode2.level_ ? return mode1 : return mode2;
}

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:62: void AddMode(NetLogCaptureMode mode) {
Also, this should either not be inlined, or called add_mode

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:77: static NetLogCaptureMode All() { return
NetLogCaptureMode(ALL); }
Given the naming of these methods, think they probably shouldn't be inlined.

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:81: bool operator==(NetLogCaptureMode o) const {
return level() == o.level(); }
"o" violates naming guidelines.  Suggest just mode.

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:84: Level level_;
+Comment about copy and assign being deliberately allowed?  Or add it before
class definition.

[eroman, 2015-04-16 22:51:09]

OK I have removed the "log" level stuff completely now, and renamed things.
Please review the differences between patchset 10 --> 12. To narrow things down
these are the files with the biggest changes:

https://codereview.chromium.org/1059843002/diff2/220001:240001/net/log/net_lo...


https://codereview.chromium.org/1059843002/diff2/220001:260001/chrome/browser...

https://codereview.chromium.org/1059843002/diff2/220001:260001/chrome/browser...

https://codereview.chromium.org/1059843002/diff2/220001:260001/chrome/browser...

https://codereview.chromium.org/1059843002/diff2/220001:260001/chrome/browser...

https://codereview.chromium.org/1059843002/diff2/220001:260001/chrome/browser...

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log.cc
File net/log/net_log.cc (right):

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log.cc#new...
net/log/net_log.cc:280: new_capture_mode.level());
On 2015/04/09 15:47:11, mmenke wrote:
> Should we really be storing the raw level here?  Seems like we should instead
> have our own NetLogCaptureMode.  Suppose doing that safely does get hairy, but
I
> thought nothing should be using raw log levels?

I have made an allowance for NetLog (by friending it), and renamed these to
ToInternalValue() and FromInternalValue().

We could remove the dependency alltogether by having NetLog define its own
mapping to an integer, however that seemed less future proof. What do you think
of the current compromise?

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:49: };
On 2015/04/09 15:47:12, mmenke wrote:
> Are these just public for net-internals?

Done -- I have now removed these from the public header.

I had originally planned on doing this piecemeal, but may as well do it all in
this change.

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:62: void AddMode(NetLogCaptureMode mode) {
On 2015/04/09 15:47:11, mmenke wrote:
> Also, this should either not be inlined, or called add_mode

Done -- I have added net_log_caputure_mode.cc and removed most of the inlined
functions.

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:62: void AddMode(NetLogCaptureMode mode) {
On 2015/04/09 15:47:11, mmenke wrote:
> Think an "Add" method that works like max is a little weird.
> 
> Suggest making this something like:
> 
> static NetLogCaptureMode max(NetLogCaptureMode mode1, NetLogCaptureMode mode2)
{
>   mode1.level_ >= mode2.level_ ? return mode1 : return mode2;
> }

Done -- I have followed your suggesting, but instead of "max" I have called it
"Union".

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:77: static NetLogCaptureMode All() { return
NetLogCaptureMode(ALL); }
On 2015/04/09 15:47:12, mmenke wrote:
> Given the naming of these methods, think they probably shouldn't be inlined.

Done.

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:81: bool operator==(NetLogCaptureMode o) const {
return level() == o.level(); }
On 2015/04/09 15:47:12, mmenke wrote:
> "o" violates naming guidelines.  Suggest just mode.

Done.

https://codereview.chromium.org/1059843002/diff/200001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:84: Level level_;
On 2015/04/09 15:47:12, mmenke wrote:
> +Comment about copy and assign being deliberately allowed?  Or add it before
> class definition.

Done.

[mmenke, 2015-04-17 15:16:06]

Think this looks pretty good.

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:40: // between |mode1| and |mode1|.
mode2

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:69: bool operator==(NetLogCaptureMode mode)
const;
This just for tests, or are there any other uses for this?

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:71: int32_t ToInternalValueForTesting() const {
return ToInternalValue(); }
We'll of course need tests for all this stuff.

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_util.cc
File net/log/net_log_util.cc (right):

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_util.c...
net/log/net_log_util.cc:257: 
Removing the block breaks loading newer logs with older versions of Chrome. 
Maybe keep an empty dictionary for a quarter or two?  (And should verify that we
can load logs across versions)

[eroman, 2015-04-17 19:20:50]

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:40: // between |mode1| and |mode1|.
On 2015/04/17 15:16:06, mmenke wrote:
> mode2

Done.

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:69: bool operator==(NetLogCaptureMode mode)
const;
On 2015/04/17 15:16:06, mmenke wrote:
> This just for tests, or are there any other uses for this?

This is currently just being used by tests (DCHECK_EQ(....)). I could turn this
into something like:
  EqualsForTesting()
However that would uglify a bunch of tests. Seems like it could be useful to
have a general support for equals, but meh.

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:71: int32_t ToInternalValueForTesting() const {
return ToInternalValue(); }
On 2015/04/17 15:16:06, mmenke wrote:
> We'll of course need tests for all this stuff.

I have attached a new file, net_log_capture_mode_unittest.cc which adds tests
for all of the capture mode, including ToInternalValueForTesting() and Max().

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_util.cc
File net/log/net_log_util.cc (right):

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_util.c...
net/log/net_log_util.cc:257: 
On 2015/04/17 15:16:06, mmenke wrote:
> Removing the block breaks loading newer logs with older versions of Chrome. 
> Maybe keep an empty dictionary for a quarter or two?  (And should verify that
we
> can load logs across versions)

Oh right, good point! Done

[mmenke, 2015-04-20 15:54:39]

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/280001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:69: bool operator==(NetLogCaptureMode mode)
const;
On 2015/04/17 19:20:50, eroman wrote:
> On 2015/04/17 15:16:06, mmenke wrote:
> > This just for tests, or are there any other uses for this?
> 
> This is currently just being used by tests (DCHECK_EQ(....)). I could turn
this
> into something like:
>   EqualsForTesting()
> However that would uglify a bunch of tests. Seems like it could be useful to
> have a general support for equals, but meh.

I'm perfectly happy with it as-is, was just wondering why it was needed (And
can't easily search the CL for uses of NetLogCaptureMode.operator==).

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log.h
File net/log/net_log.h (right):

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log.h#newc...
net/log/net_log.h:304: // The current capture mode (as an int).
May want to mention that storing the int allows getting its value without
locks....though perhaps that's obvious from the fact it's an atomic.

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
File net/log/net_log_capture_mode.cc (right):

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.cc:39: bool NetLogCaptureMode::enabled() const {
Hrm...Under the old style guide, this naming was only allowed if inlined.  The
new style guide seems less clear on the topic.

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.cc:44: return value_ > DEFAULT;
Seems more intuitive to use >= INCLUDE_COOKIES_AND_CREDENTIALS.

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.cc:48: return value_ == ALL;
Hrm...  >=?

Don't expect us to include anything more aggressive, but if we ever split up
types of captured bytes or something, seems less regression prone.

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.cc:51: NetLogCaptureMode
NetLogCaptureMode::Max(NetLogCaptureMode mode1,
All static methods should be prefixed by "// static".

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:11: 
Remove blank line and alphabetize.  Actually, why do we need build_config?

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:22: // NOTE: Default assignment and copy
constructor are OK
+ period

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:67: static NetLogCaptureMode All();
I suggest putting all these static methods just below the default constructor,
as they're basically constructors (And making Max the final one, as it's less
constructor-like).

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:84: int32_t ToInternalValue() const { return
value_; }
Hrm...These two probably shouldn't be inlined, given their names.  New style
guide isn't as clear on that as the old one, though...

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
File net/log/net_log_capture_mode_unittest.cc (right):

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode_unittest.cc:13: TEST(NetLogCaptureMode, Max) {
Hrm... we're pretty much assuming operator== works.  We do check some identity
cases further down, but:

1)  We never use it with EXPECT_NE.  Maybe it always returns true?
2)  We assume EXPECT_EQ returning true means their internal states match
exactly.  I think we're probably fine making this assumption (At least if we
have a couple EXPECT_NE calls), but think it's worth noting.

[eroman, 2015-04-20 21:07:20]

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log.h
File net/log/net_log.h (right):

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log.h#newc...
net/log/net_log.h:304: // The current capture mode (as an int).
On 2015/04/20 15:54:38, mmenke wrote:
> May want to mention that storing the int allows getting its value without
> locks....though perhaps that's obvious from the fact it's an atomic.

Done.

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
File net/log/net_log_capture_mode.cc (right):

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.cc:39: bool NetLogCaptureMode::enabled() const {
On 2015/04/20 15:54:38, mmenke wrote:
> Hrm...Under the old style guide, this naming was only allowed if inlined.  The
> new style guide seems less clear on the topic.

Acknowledged. I have not made any changes.

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.cc:44: return value_ > DEFAULT;
On 2015/04/20 15:54:38, mmenke wrote:
> Seems more intuitive to use >= INCLUDE_COOKIES_AND_CREDENTIALS.

Done.

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.cc:48: return value_ == ALL;
On 2015/04/20 15:54:38, mmenke wrote:
> Hrm...  >=?
> 
> Don't expect us to include anything more aggressive, but if we ever split up
> types of captured bytes or something, seems less regression prone.

Done. I also renamed ALL --> INCLUDE_SOCKET_BYTES, since it seemed weird to test
for something greater than ALL. I think this also makes some of the consumers
clearer that the intent is to actually log socket bytes.

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.cc:51: NetLogCaptureMode
NetLogCaptureMode::Max(NetLogCaptureMode mode1,
On 2015/04/20 15:54:38, mmenke wrote:
> All static methods should be prefixed by "// static".

I couldn't find any description of this in the style guides. I recognize that a
lot of chrome code does this, but I have gotten feedback from other reviewers
(both in Blink, and in other parts of Chrome code) not to do this. And I agree
with them, // static is something of an empty calorie.

I have left without a // static comment, let me know if you still feel it should
be there.

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:11: 
On 2015/04/20 15:54:38, mmenke wrote:
> Remove blank line and alphabetize.  Actually, why do we need build_config?

Removed all these headers except for net_export.h.

Was a consequence of having duplicated an existing file as header file template,
and then failing to properly trim everything.

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:22: // NOTE: Default assignment and copy
constructor are OK
On 2015/04/20 15:54:39, mmenke wrote:
> + period

Done.

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:67: static NetLogCaptureMode All();
On 2015/04/20 15:54:38, mmenke wrote:
> I suggest putting all these static methods just below the default constructor,
> as they're basically constructors (And making Max the final one, as it's less
> constructor-like).

Done

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:84: int32_t ToInternalValue() const { return
value_; }
On 2015/04/20 15:54:38, mmenke wrote:
> Hrm...These two probably shouldn't be inlined, given their names.  New style
> guide isn't as clear on that as the old one, though...

Done.

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
File net/log/net_log_capture_mode_unittest.cc (right):

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode_unittest.cc:13: TEST(NetLogCaptureMode, Max) {
On 2015/04/20 15:54:39, mmenke wrote:
> Hrm... we're pretty much assuming operator== works.  We do check some identity
> cases further down, but:
> 
> 1)  We never use it with EXPECT_NE.  Maybe it always returns true?
> 2)  We assume EXPECT_EQ returning true means their internal states match
> exactly.  I think we're probably fine making this assumption (At least if we
> have a couple EXPECT_NE calls), but think it's worth noting.

Done. I had to add operator!= for EXPECT_NE to work though.

[mmenke, 2015-04-20 21:11:25]

I'll do a full pass tomorrow, and expect to sign off then.

[mmenke, 2015-04-22 18:52:18]

Started reviewing yesterday, but got distracted by something shiny...

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
File net/log/net_log_capture_mode.cc (right):

https://codereview.chromium.org/1059843002/diff/340001/net/log/net_log_captur...
net/log/net_log_capture_mode.cc:51: NetLogCaptureMode
NetLogCaptureMode::Max(NetLogCaptureMode mode1,
On 2015/04/20 21:07:19, eroman wrote:
> On 2015/04/20 15:54:38, mmenke wrote:
> > All static methods should be prefixed by "// static".
> 
> I couldn't find any description of this in the style guides. I recognize that
a
> lot of chrome code does this, but I have gotten feedback from other reviewers
> (both in Blink, and in other parts of Chrome code) not to do this. And I agree
> with them, // static is something of an empty calorie.
> 
> I have left without a // static comment, let me know if you still feel it
should
> be there.

*Sigh*...Another thing that must have been removed from the style guide when
they updated back when.  They really should have some sort of a changelog (Yea,
it's on SVN, but looking through an SVN diff than includes formatting changes
does not compare to someone bringing your attention to what has actually
changed)

https://codereview.chromium.org/1059843002/diff/400001/chrome/browser/net/chr...
File chrome/browser/net/chrome_net_log.cc (right):

https://codereview.chromium.org/1059843002/diff/400001/chrome/browser/net/chr...
chrome/browser/net/chrome_net_log.cc:24: bool
GetCaptureModeFromCommandLine(const base::CommandLine* command_line,
nit:  Think const base::CommandLine& command_line is preferred

https://codereview.chromium.org/1059843002/diff/400001/chrome/browser/net/chr...
chrome/browser/net/chrome_net_log.cc:25: net::NetLogCaptureMode* mode) {
optional:  Could just make this return a net::NetLogCaptureMode, and return the
default mode on failure.  Gets rid of the bool + pointer return type, though
does make behavior a bit less clean.

https://codereview.chromium.org/1059843002/diff/400001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/1059843002/diff/400001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:81:
* NetLogCaptureMode::Default().
IncludeCookiesAndCredentials().  But no cake.  :(

https://codereview.chromium.org/1059843002/diff/400001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java
(right):

https://codereview.chromium.org/1059843002/diff/400001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:60: *
NetLogCaptureMode::Default().
IncludeCookiesAndCredentials()

https://codereview.chromium.org/1059843002/diff/400001/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/1059843002/diff/400001/content/browser/downlo...
content/browser/download/download_item_impl.cc:1109:
base::Callback<base::Value*(net::NetLogCaptureMode)> active_data =
While you're here, maybe just net::NetLog::ParametersCallback?  Templates are
ugly. :(

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:61: bool include_private_data() const;
This really should match the corresponding constructor name.

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_captur...
File net/log/net_log_capture_mode_unittest.cc (right):

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_captur...
net/log/net_log_capture_mode_unittest.cc:13: TEST(NetLogCaptureMode, Max) {
Optional:  Think this makes the most sense last, as it kind of depends on the
correctness of the others.

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_unitte...
File net/log/net_log_unittest.cc (right):

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_unitte...
net/log/net_log_unittest.cc:51: TestNetLog net_log;
This should probably go below the constants used by the test.  Could even move
it inside the loop, and get rid of the Clear() call at the end of it.

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_unitte...
net/log/net_log_unittest.cc:56:
NetLogCaptureMode::IncludeCookiesAndCredentials()};
nit:  Should probably put these in increasing or decreasing order.

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_unitte...
net/log/net_log_unittest.cc:58: for (NetLogCaptureMode mode : kModes) {
Cool!  Didn't know range loops supported arrays, though it does rather make
sense.

[eroman, 2015-04-22 20:01:47]

https://codereview.chromium.org/1059843002/diff/400001/chrome/browser/net/chr...
File chrome/browser/net/chrome_net_log.cc (right):

https://codereview.chromium.org/1059843002/diff/400001/chrome/browser/net/chr...
chrome/browser/net/chrome_net_log.cc:24: bool
GetCaptureModeFromCommandLine(const base::CommandLine* command_line,
On 2015/04/22 18:52:17, mmenke wrote:
> nit:  Think const base::CommandLine& command_line is preferred

Done.

https://codereview.chromium.org/1059843002/diff/400001/chrome/browser/net/chr...
chrome/browser/net/chrome_net_log.cc:25: net::NetLogCaptureMode* mode) {
On 2015/04/22 18:52:17, mmenke wrote:
> optional:  Could just make this return a net::NetLogCaptureMode, and return
the
> default mode on failure.  Gets rid of the bool + pointer return type, though
> does make behavior a bit less clean.

Done.

https://codereview.chromium.org/1059843002/diff/400001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/1059843002/diff/400001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:81:
* NetLogCaptureMode::Default().
On 2015/04/22 18:52:17, mmenke wrote:
> IncludeCookiesAndCredentials().  But no cake.  :(

Actually this was intentional -- the existing comment is incorrect.
See also https://codereview.chromium.org/1093793002/ for details.

https://codereview.chromium.org/1059843002/diff/400001/components/cronet/andr...
File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java
(right):

https://codereview.chromium.org/1059843002/diff/400001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:60: *
NetLogCaptureMode::Default().
On 2015/04/22 18:52:17, mmenke wrote:
> IncludeCookiesAndCredentials()

This is intentional, the previous comment was wrong.

https://codereview.chromium.org/1059843002/diff/400001/content/browser/downlo...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/1059843002/diff/400001/content/browser/downlo...
content/browser/download/download_item_impl.cc:1109:
base::Callback<base::Value*(net::NetLogCaptureMode)> active_data =
On 2015/04/22 18:52:18, mmenke wrote:
> While you're here, maybe just net::NetLog::ParametersCallback?  Templates are
> ugly. :(

Done.

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:61: bool include_private_data() const;
On 2015/04/22 18:52:18, mmenke wrote:
> This really should match the corresponding constructor name.

Done.

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_captur...
File net/log/net_log_capture_mode_unittest.cc (right):

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_captur...
net/log/net_log_capture_mode_unittest.cc:13: TEST(NetLogCaptureMode, Max) {
On 2015/04/22 18:52:18, mmenke wrote:
> Optional:  Think this makes the most sense last, as it kind of depends on the
> correctness of the others.

Done.

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_unitte...
File net/log/net_log_unittest.cc (right):

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_unitte...
net/log/net_log_unittest.cc:51: TestNetLog net_log;
On 2015/04/22 18:52:18, mmenke wrote:
> This should probably go below the constants used by the test.  Could even move
> it inside the loop, and get rid of the Clear() call at the end of it.

Done. I did not do the Clear() change, mostly because I didn't want to change
the test too much in this refactor.

https://codereview.chromium.org/1059843002/diff/400001/net/log/net_log_unitte...
net/log/net_log_unittest.cc:56:
NetLogCaptureMode::IncludeCookiesAndCredentials()};
On 2015/04/22 18:52:18, mmenke wrote:
> nit:  Should probably put these in increasing or decreasing order.

Done.

[mmenke, 2015-04-22 20:08:35]

LGTM

https://codereview.chromium.org/1059843002/diff/400001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/1059843002/diff/400001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:81:
* NetLogCaptureMode::Default().
On 2015/04/22 20:01:47, eroman wrote:
> On 2015/04/22 18:52:17, mmenke wrote:
> > IncludeCookiesAndCredentials().  But no cake.  :(
> 
> Actually this was intentional -- the existing comment is incorrect.
> See also https://codereview.chromium.org/1093793002/ for details.

Ahh, right.  I thought I checked by that by looking at the
IncludeCookiesAndCredentials() call in url_request_context_adapter.cc, but
that's for something else entirely.

[mmenke, 2015-04-22 20:13:36]

https://codereview.chromium.org/1059843002/diff/420001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/420001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:56: // If |enabled()| is true, then _something_
is being captured.
nit (x3):  Should ||'s go around method names?  Seems pretty uncommon.

[eroman, 2015-04-22 20:24:44]

https://codereview.chromium.org/1059843002/diff/420001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/420001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:56: // If |enabled()| is true, then _something_
is being captured.
On 2015/04/22 20:13:35, mmenke wrote:
> nit (x3):  Should ||'s go around method names?  Seems pretty uncommon.

I couldn't find an explanation of "|" in any of the styleguides.

My interpretation and use of it has been that:
  |XXX|  means <code>XXX</code>

And if comments were rendered in HTML, I would expect to see code expressions
inside of this rendered in a mono font.

In terms of disambiguation parens are already pretty good at giving a visual
indication so perhaps the bars are unnecessary.

I don't much care either way. Unless you have a preference I will leave the
vertical bars in there.

[mmenke, 2015-04-22 20:30:12]

https://codereview.chromium.org/1059843002/diff/420001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/420001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:9: #include <string>
I think there should be a blank line betwee C headers and C++ ones?  At least
that's how files generally do it.

https://codereview.chromium.org/1059843002/diff/420001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:56: // If |enabled()| is true, then _something_
is being captured.
On 2015/04/22 20:24:44, eroman wrote:
> On 2015/04/22 20:13:35, mmenke wrote:
> > nit (x3):  Should ||'s go around method names?  Seems pretty uncommon.
> 
> I couldn't find an explanation of "|" in any of the styleguides.
> 
> My interpretation and use of it has been that:
>   |XXX|  means <code>XXX</code>
> 
> And if comments were rendered in HTML, I would expect to see code expressions
> inside of this rendered in a mono font.
> 
> In terms of disambiguation parens are already pretty good at giving a visual
> indication so perhaps the bars are unnecessary.
> 
> I don't much care either way. Unless you have a preference I will leave the
> vertical bars in there.


The old style guide said they had to be used around arguments to methods, and
left all other use to user discretion.  I generally put them around variable
names (private and members), and that's it.

I figure with methods, the "()" already indicates it's a method name, rather
than english text, even if there are no underscores or weird capitalization, so
it's not needed.  Anyhow, not something I feel strongly about, either way.

[eroman, 2015-04-22 20:52:05]

https://codereview.chromium.org/1059843002/diff/420001/net/log/net_log_captur...
File net/log/net_log_capture_mode.h (right):

https://codereview.chromium.org/1059843002/diff/420001/net/log/net_log_captur...
net/log/net_log_capture_mode.h:56: // If |enabled()| is true, then _something_
is being captured.
On 2015/04/22 20:30:12, mmenke wrote:
> On 2015/04/22 20:24:44, eroman wrote:
> > On 2015/04/22 20:13:35, mmenke wrote:
> > > nit (x3):  Should ||'s go around method names?  Seems pretty uncommon.
> > 
> > I couldn't find an explanation of "|" in any of the styleguides.
> > 
> > My interpretation and use of it has been that:
> >   |XXX|  means <code>XXX</code>
> > 
> > And if comments were rendered in HTML, I would expect to see code
expressions
> > inside of this rendered in a mono font.
> > 
> > In terms of disambiguation parens are already pretty good at giving a visual
> > indication so perhaps the bars are unnecessary.
> > 
> > I don't much care either way. Unless you have a preference I will leave the
> > vertical bars in there.
> 
> 
> The old style guide said they had to be used around arguments to methods, and
> left all other use to user discretion.  I generally put them around variable
> names (private and members), and that's it.
> 
> I figure with methods, the "()" already indicates it's a method name, rather
> than english text, even if there are no underscores or weird capitalization,
so
> it's not needed.  Anyhow, not something I feel strongly about, either way.

Done.

[eroman, 2015-04-22 21:11:25]

eroman@chromium.org changed reviewers:
+ asanka@chromium.org, asargent@chromium.org, bengr@chromium.org,
scherkus@chromium.org, sergeyu@chromium.org, yurys@chromium.org,
zea@chromium.org

[eroman, 2015-04-22 21:11:27]

TBR-ing the OWNERs (as this is basically just a rename outside of net/*).
No real review is necessary on your part, however as an FYI the per-file
breakdown is as follows:

asanka:
  content/browser/download/download_file_impl.cc
  content/browser/download/download_item_impl.cc
  content/browser/download/download_net_log_parameters.cc
  content/browser/download/download_net_log_parameters.h

asargent:
  chrome/browser/extensions/api/log_private/log_private_api_chromeos.cc
  extensions/browser/api/web_request/web_request_api_helpers.cc

sergeyu:
  remoting/base/vlog_net_log.cc

yurys:
  content/browser/devtools/devtools_netlog_observer.cc

bengr:
 
components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc

zea:
  google_apis/gcm/tools/mcs_probe.cc

scherkus:
  content/browser/media/media_internals_proxy.cc

[eroman, 2015-04-22 21:13:44]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-04-22 21:13:45]

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/1059843002/#ps470001
(title: "rebase for reals")

[commit-bot: I haz the power, 2015-04-22 21:16:39]

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

[commit-bot: I haz the power, 2015-04-23 00:45:58]

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

[commit-bot: I haz the power, 2015-04-23 00:46:01]

Failed to apply patch for
components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:
While running git apply --index -3 -p1;
  error: patch failed:
components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc:43
  Falling back to three-way merge...
  Applied patch to
'components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc'
with conflicts.
  U
components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc

Patch:      
components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc
Index:
components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc
diff --git
a/components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc
b/components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc
index
9612dee490e8a0fd98bdb1bab0b66e3144a733ac..2717ef8c997d80c3f13b74c802e90b500d5729a8
100644
---
a/components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc
+++
b/components/data_reduction_proxy/core/common/data_reduction_proxy_event_store.cc
@@ -43,7 +43,8 @@ scoped_ptr<base::Value> BuildDataReductionProxyEvent(
   base::TimeTicks ticks_now = base::TimeTicks::Now();
   net::NetLog::EntryData entry_data(
       type, source, phase, ticks_now, &parameters_callback);
-  net::NetLog::Entry entry(&entry_data, net::NetLog::LOG_ALL);
+  net::NetLog::Entry entry(&entry_data,
+                           net::NetLogCaptureMode::IncludeSocketBytes());
   scoped_ptr<base::Value> entry_value(entry.ToValue());
 
   return entry_value;
@@ -76,7 +77,7 @@ base::Value* EnableDataReductionProxyCallback(
     const std::string& primary_origin,
     const std::string& fallback_origin,
     const std::string& ssl_origin,
-    net::NetLog::LogLevel /* log_level */) {
+    net::NetLogCaptureMode /* capture_mode */) {
   base::DictionaryValue* dict = new base::DictionaryValue();
   dict->SetBoolean("enabled", true);
   dict->SetBoolean("primary_restricted", primary_restricted);
@@ -88,17 +89,18 @@ base::Value* EnableDataReductionProxyCallback(
 }
 
 base::Value* DisableDataReductionProxyCallback(
-    net::NetLog::LogLevel /* log_level */) {
+    net::NetLogCaptureMode /* capture_mode */) {
   base::DictionaryValue* dict = new base::DictionaryValue();
   dict->SetBoolean("enabled", false);
   return dict;
 }
 
-base::Value* UrlBypassActionCallback(const std::string& action,
-                                     const GURL& url,
-                                     int bypass_seconds,
-                                     int64 expiration_ticks,
-                                     net::NetLog::LogLevel /* log_level */) {
+base::Value* UrlBypassActionCallback(
+    const std::string& action,
+    const GURL& url,
+    int bypass_seconds,
+    int64 expiration_ticks,
+    net::NetLogCaptureMode /* capture_mode */) {
   base::DictionaryValue* dict = new base::DictionaryValue();
   dict->SetString("action", action);
   dict->SetString("url", url.spec());
@@ -113,7 +115,7 @@ base::Value* UrlBypassTypeCallback(
     const GURL& url,
     int bypass_seconds,
     int64 expiration_ticks,
-    net::NetLog::LogLevel /* log_level */) {
+    net::NetLogCaptureMode /* capture_mode */) {
   base::DictionaryValue* dict = new base::DictionaryValue();
   dict->SetInteger("bypass_type", bypass_type);
   dict->SetString("url", url.spec());
@@ -123,10 +125,11 @@ base::Value* UrlBypassTypeCallback(
   return dict;
 }
 
-base::Value* EndCanaryRequestCallback(int net_error,
-                                      int http_response_code,
-                                      bool succeeded,
-                                      net::NetLog::LogLevel /* log_level */) {
+base::Value* EndCanaryRequestCallback(
+    int net_error,
+    int http_response_code,
+    bool succeeded,
+    net::NetLogCaptureMode /* capture_mode */) {
   base::DictionaryValue* dict = new base::DictionaryValue();
   dict->SetInteger("net_error", net_error);
   dict->SetInteger("http_response_code", http_response_code);

[commit-bot: I haz the power, 2015-04-23 00:47:35]

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

[eroman, 2015-04-23 01:03:53]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-04-23 01:03:54]

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/1059843002/#ps490001
(title: "rebase again to fix a merge conflict")

[commit-bot: I haz the power, 2015-04-23 01:04:38]

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

[commit-bot: I haz the power, 2015-04-23 03:11:27]

Committed patchset #24 (id:490001)

[commit-bot: I haz the power, 2015-04-23 03:12:14]

Patchset 24 (id:??) landed as
https://crrev.com/001c374c5356777641161bb11b8dd792508381ca
Cr-Commit-Position: refs/heads/master@{#326440}