Remove NetLog::GetCaptureMode() and NetLogCaptureMode::None()

Remove NetLog::GetCaptureMode() and NetLogCaptureMode::None()

  * In place of GetCaptureMode() use NetLog::IsCapturing().
  * NetLogCaptureMode::None() is no longer needed now.

This clears up some confusion since there are no longer multiple sources of truth for what the capture mode is (per observer vs per log). The removal of NetLogCaptureMode::None() similarly removes confusion over being able to add an observer with a capture mode of None.

BUG=484762
TBR=asanka@chromium.org

Committed: https://crrev.com/24bc6a1df2048ee8feeb1d837cdfddd2e9d0cde1
Cr-Commit-Position: refs/heads/master@{#328589}

[eroman, 2015-05-05 19:13:10]

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

[mmenke, 2015-05-05 19:45:15]

Quick comments.  Unsure if I'll have time for a full pass today.

Like that we're no longer exposing the internal value of the capture mode
object.  A definitely improvement.

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

https://codereview.chromium.org/1122483004/diff/1/net/log/net_log.cc#newcode258
net/log/net_log.cc:258: observer->capture_mode_ = capture_mode;
Do we ever explicitly disallow observers from watching the NetLog with a
CaptureMode of None?  I suppose we may not care about that case.

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

https://codereview.chromium.org/1122483004/diff/1/net/log/net_log.h#newcode297
net/log/net_log.h:297: // Called whenever an observer is added or removed, to
update has_observers_.
|observers_| is more consistent with the other comments in this file.

https://codereview.chromium.org/1122483004/diff/1/net/log/net_log.h#newcode365
net/log/net_log.h:365: bool HasObservers() const;
Maybe IsLogging?  I don't think consumers of the BoundNetLog really need to know
why we're logging events.  Also, when we get rid of observers, it won't need to
be renamed, though admittedly, we may want to remove this method at that point,
anyways.

[eroman, 2015-05-05 22:54:38]

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

https://codereview.chromium.org/1122483004/diff/1/net/log/net_log.cc#newcode258
net/log/net_log.cc:258: observer->capture_mode_ = capture_mode;
On 2015/05/05 19:45:15, mmenke wrote:
> Do we ever explicitly disallow observers from watching the NetLog with a
> CaptureMode of None?  I suppose we may not care about that case.

I updated the CL and just removed NetLogCaptureMode::None() all together, so
problem gone I guess.

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

https://codereview.chromium.org/1122483004/diff/1/net/log/net_log.h#newcode297
net/log/net_log.h:297: // Called whenever an observer is added or removed, to
update has_observers_.
On 2015/05/05 19:45:15, mmenke wrote:
> |observers_| is more consistent with the other comments in this file.

Done (I presume you meant |has_observers_|)

https://codereview.chromium.org/1122483004/diff/1/net/log/net_log.h#newcode365
net/log/net_log.h:365: bool HasObservers() const;
On 2015/05/05 19:45:15, mmenke wrote:
> Maybe IsLogging?  I don't think consumers of the BoundNetLog really need to
know
> why we're logging events.  Also, when we get rid of observers, it won't need
to
> be renamed, though admittedly, we may want to remove this method at that
point,
> anyways.

Renamed to IsCapturing().
Which I think is in line with "NetLogCaptureMode != None" of old naming.

[mmenke, 2015-05-06 15:14:42]

Looks good!  Good call on getting rid of None().

https://codereview.chromium.org/1122483004/diff/80001/chrome/browser/net/net_...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/1122483004/diff/80001/chrome/browser/net/net_...
chrome/browser/net/net_log_temp_file_unittest.cc:201: 
Since you removed the check here, we have no check that we're actually setting
the log level, as opposed to the NetLogTemp file just claiming it did.  Maybe
add a protected method to NetLogTempFile to get write_to_file_observer_'s
CaptureMode?

https://codereview.chromium.org/1122483004/diff/80001/net/log/net_log_capture...
File net/log/net_log_capture_mode_unittest.cc (right):

https://codereview.chromium.org/1122483004/diff/80001/net/log/net_log_capture...
net/log/net_log_capture_mode_unittest.cc:27:
NetLogCaptureMode::Default().ToInternalValueForTesting());
Hrm...Do we even care about testing this any more?  Now that we just copy them,
and have nothing more complicated, we may be able to just get rid of
ToInternalValueForTesting(), unless you think we'll be doing something more
complicated in the future.

https://codereview.chromium.org/1122483004/diff/80001/net/log/net_log_unittes...
File net/log/net_log_unittest.cc (left):

https://codereview.chromium.org/1122483004/diff/80001/net/log/net_log_unittes...
net/log/net_log_unittest.cc:351: net_log.GetCaptureMode());
optoinal:  May want to replace all of these with IsCapturing(), out of paranoia.

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

https://codereview.chromium.org/1122483004/diff/80001/net/log/net_log_util.cc...
net/log/net_log_util.cc:527: NetLog::Entry entry(&entry_data,
NetLogCaptureMode::Default());
Suggest a comment that GetRequestStateAsValue ignores the capture mode.

https://codereview.chromium.org/1122483004/diff/80001/net/log/test_net_log.h
File net/log/test_net_log.h (right):

https://codereview.chromium.org/1122483004/diff/80001/net/log/test_net_log.h#...
net/log/test_net_log.h:27: NetLogCaptureMode GetCaptureMode() const;
Why do we need this?

[eroman, 2015-05-06 17:01:05]

https://codereview.chromium.org/1122483004/diff/80001/chrome/browser/net/net_...
File chrome/browser/net/net_log_temp_file_unittest.cc (right):

https://codereview.chromium.org/1122483004/diff/80001/chrome/browser/net/net_...
chrome/browser/net/net_log_temp_file_unittest.cc:201: 
On 2015/05/06 15:14:41, mmenke wrote:
> Since you removed the check here, we have no check that we're actually setting
> the log level, as opposed to the NetLogTemp file just claiming it did.  Maybe
> add a protected method to NetLogTempFile to get write_to_file_observer_'s
> CaptureMode?

Oops my bad (I meant to add a check back for this)!
Done.

https://codereview.chromium.org/1122483004/diff/80001/net/log/net_log_capture...
File net/log/net_log_capture_mode_unittest.cc (right):

https://codereview.chromium.org/1122483004/diff/80001/net/log/net_log_capture...
net/log/net_log_capture_mode_unittest.cc:27:
NetLogCaptureMode::Default().ToInternalValueForTesting());
On 2015/05/06 15:14:42, mmenke wrote:
> Hrm...Do we even care about testing this any more?  Now that we just copy
them,
> and have nothing more complicated, we may be able to just get rid of
> ToInternalValueForTesting(), unless you think we'll be doing something more
> complicated in the future.

We can get rid of ToInternalValueForTesting() but it will require massaging
net_log_unittest.cc into compliance. I will leave this to a follow-up CL.

https://codereview.chromium.org/1122483004/diff/80001/net/log/net_log_unittes...
File net/log/net_log_unittest.cc (left):

https://codereview.chromium.org/1122483004/diff/80001/net/log/net_log_unittes...
net/log/net_log_unittest.cc:351: net_log.GetCaptureMode());
On 2015/05/06 15:14:42, mmenke wrote:
> optoinal:  May want to replace all of these with IsCapturing(), out of
paranoia.

Done.

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

https://codereview.chromium.org/1122483004/diff/80001/net/log/net_log_util.cc...
net/log/net_log_util.cc:527: NetLog::Entry entry(&entry_data,
NetLogCaptureMode::Default());
On 2015/05/06 15:14:42, mmenke wrote:
> Suggest a comment that GetRequestStateAsValue ignores the capture mode.

Done.

https://codereview.chromium.org/1122483004/diff/80001/net/log/test_net_log.h
File net/log/test_net_log.h (right):

https://codereview.chromium.org/1122483004/diff/80001/net/log/test_net_log.h#...
net/log/test_net_log.h:27: NetLogCaptureMode GetCaptureMode() const;
On 2015/05/06 15:14:42, mmenke wrote:
> Why do we need this?

Done (removed)

[mmenke, 2015-05-06 17:07:09]

LGTM

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

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

[eroman, 2015-05-06 17:22:35]

TBR asanka for:
  content/browser/download/download_file_impl.cc
  content/browser/download/download_item_impl.cc

[eroman, 2015-05-06 17:26:11]

The CQ bit was checked by eroman@chromium.org

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

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

[eroman, 2015-05-06 17:36:35]

The CQ bit was unchecked by eroman@chromium.org

[eroman, 2015-05-06 17:37:12]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-05-06 17:37:13]

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/1122483004/#ps120001
(title: "Add explicit casts bool <--> int to appease windows compiler")

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

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

[commit-bot: I haz the power, 2015-05-06 18:27:06]

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

[commit-bot: I haz the power, 2015-05-06 18:27:07]

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

[eroman, 2015-05-06 18:41:14]

The CQ bit was checked by eroman@chromium.org

[eroman, 2015-05-06 18:41:14]

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/1122483004/#ps140001
(title: "second attempt at fixing int --> bool warning on windows")

[commit-bot: I haz the power, 2015-05-06 18:42:34]

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

[commit-bot: I haz the power, 2015-05-06 19:56:14]

Committed patchset #8 (id:140001)

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

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