Allow setting Cronet NetLog level to LOG_ALL

Allow setting Cronet NetLog level to NetLogCaptureMode::IncludeSocketBytes().

Add an extra parameter to Cronet startNetLogToFile API methods to allow setting the IncludeSocketBytes() logging level.

BUG=

Committed: https://crrev.com/9f27db61b2c5d62f285b43a09f0a9fccbeb14fd6
Cr-Commit-Position: refs/heads/master@{#328771}

[miloslav, 2015-04-17 14:12:27]

miloslav@google.com changed reviewers:
+ mef@chromium.org, miloslav@google.com

[miloslav, 2015-04-17 14:15:03]

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/j...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java
(right):

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:72:
public abstract void startNetLogToFile(String fileName, boolean logAll);
Would making this an int (the actual log level passed) give us more flexibility?

[eroman, 2015-04-17 21:35:09]

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

[eroman, 2015-04-17 21:35:10]

Driveby: generally looks good. There may be some conflicts with my change to log
levels (https://codereview.chromium.org/1059843002/) but they should be easy to
resolve.

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/c...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/c...
components/cronet/android/cronet_url_request_context_adapter.cc:299:
net_log_logger_->set_log_level(net::NetLog::LogLevel::LOG_ALL);
Note that I am changing this to set_capture_mode(). See
https://codereview.chromium.org/1059843002/

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/j...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:81:
* LOG_STRIP_PRIVATE_DATA or LOG_ALL.
Note that LOG_STRIP_PRIVATE_DATA will soon be named NetLogCaptureMode::Default()
instead -- https://codereview.chromium.org/1059843002/.

Also updating this comment as part of that change

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/j...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java
(right):

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:72:
public abstract void startNetLogToFile(String fileName, boolean logAll);
On 2015/04/17 14:15:03, miloslav wrote:
> Would making this an int (the actual log level passed) give us more
flexibility?
 
I am in favor of a boolean rather than an integer.

For starters, I think it is simpler to just have default+all modes exposed.
Secondly I am removing the integer based LogLevel in favor of a "CaptureMode"
class in https://codereview.chromium.org/1059843002/ so using an integer is not
going to work as well following that change.

[eroman, 2015-05-02 01:35:48]

curious what the status of this was..
BTW you may get some conflicts when syncing due to changes I made to
NetLog::LogLevel --> NetLogCaptureMode

[kallasjoki, 2015-05-06 13:56:56]

On 2015/05/02 01:35:48, eroman wrote:
> curious what the status of this was..
> BTW you may get some conflicts when syncing due to changes I made to
> NetLog::LogLevel --> NetLogCaptureMode

Status was "got pushed off the stack due to other things".

I've now synced this across your NetLog refactorings, and (hopefully) replied to
comments. Let me know if/what more work is needed.

[kallasjoki, 2015-05-06 13:58:57]

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/c...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/c...
components/cronet/android/cronet_url_request_context_adapter.cc:299:
net_log_logger_->set_log_level(net::NetLog::LogLevel::LOG_ALL);
On 2015/04/17 21:35:10, eroman wrote:
> Note that I am changing this to set_capture_mode(). See
> https://codereview.chromium.org/1059843002/

Acknowledged.

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/j...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:81:
* LOG_STRIP_PRIVATE_DATA or LOG_ALL.
On 2015/04/17 21:35:10, eroman wrote:
> Note that LOG_STRIP_PRIVATE_DATA will soon be named
NetLogCaptureMode::Default()
> instead -- https://codereview.chromium.org/1059843002/.
> 
> Also updating this comment as part of that change

Acknowledged.

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/j...
File
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java
(right):

https://codereview.chromium.org/1093793002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/HttpUrlRequestFactory.java:72:
public abstract void startNetLogToFile(String fileName, boolean logAll);
On 2015/04/17 21:35:10, eroman wrote:
> On 2015/04/17 14:15:03, miloslav wrote:
> > Would making this an int (the actual log level passed) give us more
> flexibility?
>  
> I am in favor of a boolean rather than an integer.
> 
> For starters, I think it is simpler to just have default+all modes exposed.
> Secondly I am removing the integer based LogLevel in favor of a "CaptureMode"
> class in https://codereview.chromium.org/1059843002/ so using an integer is
not
> going to work as well following that change.

I've kept it as the boolean default-or-all choice, since that seems to fill our
immediately foreseeable needs, and exposing the whole NetLogCaptureMode logic to
the Java side seems like extra complexity.

[eroman, 2015-05-06 17:34:19]

LGTM after fixing the comments

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:305: if
(log_all)
nit: I suggest putting curlies around this

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:86:
* @param logAll {@code true} to use the NetLogCaptureMode::Default()
This comment is reversed. true=IncludeSocketBytes, false=Default

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java
(right):

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:64: *
@param logAll {@code true} to use the NetLogCaptureMode::Default()
Same comment as earlier (this is reversed)

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:298: if (log_all)
I suggest putting curlies around this

[kallasjoki, 2015-05-06 17:47:08]

Addressed comments.

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
components/cronet/android/cronet_url_request_context_adapter.cc:305: if
(log_all)
On 2015/05/06 17:34:19, eroman wrote:
> nit: I suggest putting curlies around this

Done.

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java
(right):

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestContext.java:86:
* @param logAll {@code true} to use the NetLogCaptureMode::Default()
On 2015/05/06 17:34:19, eroman wrote:
> This comment is reversed. true=IncludeSocketBytes, false=Default

Done. (I got them completely flipped when adjusting for the new names.)

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java
(right):

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:64: *
@param logAll {@code true} to use the NetLogCaptureMode::Default()
On 2015/05/06 17:34:19, eroman wrote:
> Same comment as earlier (this is reversed)

Done.

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
File components/cronet/android/url_request_context_adapter.cc (right):

https://codereview.chromium.org/1093793002/diff/40001/components/cronet/andro...
components/cronet/android/url_request_context_adapter.cc:298: if (log_all)
On 2015/05/06 17:34:19, eroman wrote:
> I suggest putting curlies around this

Done.

[eroman, 2015-05-06 17:50:40]

patchset 4 LGTM

[kallasjoki, 2015-05-06 17:57:15]

The CQ bit was checked by kallasjoki@google.com

[commit-bot: I haz the power, 2015-05-06 17:58:05]

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

[xunjieli, 2015-05-06 18:00:29]

The CQ bit was unchecked by xunjieli@chromium.org

[xunjieli, 2015-05-06 18:01:22]

On 2015/05/06 17:58:05, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1093793002/60001

I think you will need a OWNER stamp for components/cronet/ as well. Since mef@
is on the reviewer list, maybe you could ping him.

[kallasjoki, 2015-05-06 18:08:21]

On 2015/05/06 18:01:22, xunjieli wrote:
> On 2015/05/06 17:58:05, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1093793002/60001
> 
> I think you will need a OWNER stamp for components/cronet/ as well. Since mef@
> is on the reviewer list, maybe you could ping him.

Okay, thanks. (New to Chromium review practices here.)

[xunjieli, 2015-05-06 20:31:57]

On 2015/05/06 18:08:21, kallasjoki wrote:
> On 2015/05/06 18:01:22, xunjieli wrote:
> > On 2015/05/06 17:58:05, I haz the power (commit-bot) wrote:
> > > CQ is trying da patch. Follow status at
> > >  https://chromium-cq-status.appspot.com/patch-status/1093793002/60001
> > 
> > I think you will need a OWNER stamp for components/cronet/ as well. Since
mef@
> > is on the reviewer list, maybe you could ping him.
> 
> Okay, thanks. (New to Chromium review practices here.)

Thanks for doing this by the way! I think it's a useful feature to have.
Overall the API change LGTM. Misha's doing the binary drop, so he'll need to fix
current embedders. But I don't think there are many places where startNetLog is
used, so fixing them one by one should be okay.

[mef, 2015-05-06 21:47:28]

Looks pretty good, thanks for doing this! 

Could you add tests that use 'logAll' == true?

https://codereview.chromium.org/1093793002/diff/60001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java
(right):

https://codereview.chromium.org/1093793002/diff/60001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:60: *
NetLogCaptureMode::Default() or NetLogCaptureMode::IncludeSocketBytes().
I would explicitly specify that IncludeSocketBytes includes cookies and
credentials, so clients are more aware of that without need to look up
IncludeSocketBytes() definition.

[kallasjoki, 2015-05-07 10:51:27]

You're welcome -- though I confess I did it for purely selfish reasons: I've
already used the with-bytes NetLog for debugging twice, and it would/will be
more convenient to have it included in stock Cronet.

I added one test with logAll=true in CronetUrlRequestContextTest, and made the
other NetLog tests assert that no bytes are logged. The detection of logged
bytes is perhaps not the most elegant/robust thing in the world (and doesn't
check for cookie/credential-stripping), but it seems to work.

https://codereview.chromium.org/1093793002/diff/60001/components/cronet/andro...
File components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java
(right):

https://codereview.chromium.org/1093793002/diff/60001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/UrlRequestContext.java:60: *
NetLogCaptureMode::Default() or NetLogCaptureMode::IncludeSocketBytes().
On 2015/05/06 21:47:28, mef wrote:
> I would explicitly specify that IncludeSocketBytes includes cookies and
> credentials, so clients are more aware of that without need to look up
> IncludeSocketBytes() definition.

Done.

[mef, 2015-05-07 15:20:40]

lgtm

lgtm, thanks!

[kallasjoki, 2015-05-07 15:47:56]

The CQ bit was checked by kallasjoki@google.com

[kallasjoki, 2015-05-07 15:47:56]

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

[commit-bot: I haz the power, 2015-05-07 15:48:18]

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

[commit-bot: I haz the power, 2015-05-07 16:06:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-07 16:07:19]

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