Re-factor network_event_log

Re-factor network_event_log

BUG=239652
For webui:
TBR=gauravsh@chromium.org, pneubeck@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=200632

[gauravsh, 2013-05-13 00:34:50]

https://codereview.chromium.org/14876021/diff/2001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/about_network.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/about_network.cc:299: base::StringTokenizer
tok(query, "/");
Update the comment for AboutNetwork in about_network.h.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
File chromeos/network/network_event_log.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:23: int log_level,
Why is this an int instead of a log-level? std::min and comparisons should still
work if use the enum type here.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:27: std::string ToString(const
std::string& format) const;
Comment for the format of |format|?

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:100: int max_level,
Why is this not a LogLevel?

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:141: // Iterate backwards through the
list, decramenting the offset for entries
decrementing

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:146: if (--offset == 0)
BUG? Wouldn't this end up decrementing the offset more than necessary? If I have
5 log entries such as:
1,INFO
2,ERROR
3,INFO
4,ERROR
5,INFO

GetAsString(OLDEST_FIRST, .., ERROR, 1) should return "4,ERROR"

However, according to this code:
line 138:  sets |offset| = 5 - 1 = 4
line 146:  --offset executed 3 times (# of log entries at the INFO level)
line 151: Final |offset| = 4 - 3 = 1
lines 152-155 would then return: 
  2,ERROR
  4,ERROR
...which is incorrect.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
File chromeos/network/network_event_log.h (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:56: CHROMEOS_EXPORT void AddEntry(const
char* file,
When would one use this directly? If this is not meant to be used directly, lets
move this to an internal namespace to make that clearer.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:71: // Only events with |log_level| >=
|max_level| are included in the output.
This should be |log_level| <= |max_level|.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:79: CHROMEOS_EXPORT std::string
DBusValueAsString(const base::Value& value);
NetworkEventLog is an odd place for this. I think this should be in its own
utility module with other helpers that work on *Value-s. (e.g. 
ManagedState::Get*Value , ConvertListValueToStringVector in network_state, etc.)

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:89: // Non-essencial debugging events
Non-essential

[pneubeck (no reviews), 2013-05-13 13:16:25]

https://codereview.chromium.org/14876021/diff/2001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/about_network.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/about_network.cc:62: std::string
GetHeaderEventLogInfo(bool debug) {
This function's name is cryptic to me.

https://codereview.chromium.org/14876021/diff/2001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/about_network.cc:75:
network_event_log::GetAsString(network_event_log::NEWEST_FIRST,
creating html content from insecure sources (like network name) is very
unhealthy. This has to use appropriate escaping. This is also true for <pre>'s
content.

https://codereview.chromium.org/14876021/diff/2001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/about_network.cc:299: base::StringTokenizer
tok(query, "/");
shouldn't this be separated by '&' ?

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/managed_s...
File chromeos/network/managed_state.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/managed_s...
chromeos/network/managed_state.cc:68: if (*out_value == new_value)
Not related to the log changes.
Either update the issue description or split the CL. I'd prefer the latter.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
File chromeos/network/network_event_log.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:31: std::string file;
base::Location

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:89: typedef std::deque<LogEntry>
LogEntryList;
depending on how frequent you expect log calls, this is something to maybe fix
in the future. A circular buffer using a std::vector would avoid all the heap
allocations.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:198: std::string filestr;
FilePath::BaseName()

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:207: VLOG(1) <<
entry.ToString("time,file,desc");
this should respect the log level.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
File chromeos/network/network_event_log.h (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:65: // can be combined with or without
separators, e.g. "time,desc".
you're parsing the format string per entry. Since the rational behind this log
was high-frequent events: Please replace these strings by much more efficient
bit flags (in an enum) which can be bit-wise OR-ed and AND-ed.

(if there is a strong argument to continue with strings:
remove "without separators" and mention a specific separator. Otherwise this is
very fragile wrt. future changes.)

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:79: CHROMEOS_EXPORT std::string
DBusValueAsString(const base::Value& value);
On 2013/05/13 00:34:51, gauravsh wrote:
> NetworkEventLog is an odd place for this. I think this should be in its own
> utility module with other helpers that work on *Value-s. (e.g. 
> ManagedState::Get*Value , ConvertListValueToStringVector in network_state,
etc.)

There is already:
 operator<< in values.h
and
 JsonWriter

This function should at least delegate to one of these.
The method name is unnecessarily restrictive.

+1 to place this into a helper file that is as near to base/values.h as
possible, like base/values_util.h

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:96: __FILE__, __LINE__, log_level, event,
description)
should be FROM_HERE

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
File chromeos/network/network_event_log_unittest.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log_unittest.cc:169:
AddEventsAtLevel(network_event_log::LOG_LEVEL_DEBUG, 1);
please add two more groups, so that there are two separated groups (also not at
the border) which are filtered out in the subsequent tests. something like:

a:0 b:2 c:0 d:0 e:1 f:0  ==(last 2, =< 0)==> d:0 f:0

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_h...
File chromeos/network/network_handler_callbacks.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_h...
chromeos/network/network_handler_callbacks.cc:38: NET_LOG_ERROR(error,
error_message);
the file/line that is logged by NET_LOG_ERROR will not be helpful in this case
and is not a replacement for |module|.
Instead |module| should be replaced by base::Location and callers should use
FROM_HERE.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:58: std::string
GetManagedStateLogName(const chromeos::ManagedState* network) {
either the function name and argument type are wrong or the argument-name.

rename 'network' -> 'state'

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:300:
NET_LOG_EVENT("SetConnectingNetwork", GetNameFromPath(service_path));
Does GetNameFromPath really have to exposed in the header? This is the only
place where it's used (after fixing my other comment)? Do we need it at all?

If so, restrict it at least to Networks, so that the ambiguity of service paths
("is it a network or a device?") is gone.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:392:
NET_LOG_DEBUG("PropertiesReceived", GetNameFromPath(path));
should use GetManagedStateLogName(managed) instead.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:609: const std::string& path) const {
after fixing my other comment, this is only used for networks. no need to guess.

[stevenjb, 2013-05-13 19:36:38]

PTAL

https://codereview.chromium.org/14876021/diff/2001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/about_network.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/about_network.cc:62: std::string
GetHeaderEventLogInfo(bool debug) {
On 2013/05/13 13:16:25, pneubeck wrote:
> This function's name is cryptic to me.
A suggestion would help since it's obviously less cryptic to me :) 
Changed to GetEventLogSection.

https://codereview.chromium.org/14876021/diff/2001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/about_network.cc:75:
network_event_log::GetAsString(network_event_log::NEWEST_FIRST,
On 2013/05/13 13:16:25, pneubeck wrote:
> creating html content from insecure sources (like network name) is very
> unhealthy. This has to use appropriate escaping. This is also true for <pre>'s
> content.
Yeah, this is long overdue to be replaced with WebUI. Right after we fix
settings...
Good catch, added escaping.

https://codereview.chromium.org/14876021/diff/2001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/about_network.cc:299: base::StringTokenizer
tok(query, "/");
On 2013/05/13 13:16:25, pneubeck wrote:
> shouldn't this be separated by '&' ?
We should really have a proper UI here. This is a quick and dirty hack, I wanted
to remain consistent with the /X for refresh. Comment updated.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/managed_s...
File chromeos/network/managed_state.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/managed_s...
chromeos/network/managed_state.cc:68: if (*out_value == new_value)
On 2013/05/13 13:16:25, pneubeck wrote:
> Not related to the log changes.
> Either update the issue description or split the CL. I'd prefer the latter.

It is related since the only place that really cares whether we set a value to
an existing one is the logging, to avoid spam.
Since this does affect the frequency of observer calls,
I'll go ahead and split it out and add a test.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
File chromeos/network/network_event_log.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:23: int log_level,
On 2013/05/13 00:34:51, gauravsh wrote:
> Why is this an int instead of a log-level? std::min and comparisons should
still
> work if use the enum type here.

Done.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:27: std::string ToString(const
std::string& format) const;
On 2013/05/13 00:34:51, gauravsh wrote:
> Comment for the format of |format|?
Changed to use bools

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:31: std::string file;
On 2013/05/13 13:16:25, pneubeck wrote:
> base::Location
Again, more complexity than I think we want, see ShillErrorCallbackFunction
comment.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:89: typedef std::deque<LogEntry>
LogEntryList;
On 2013/05/13 13:16:25, pneubeck wrote:
> depending on how frequent you expect log calls, this is something to maybe fix
> in the future. A circular buffer using a std::vector would avoid all the heap
> allocations.
That doesn't really seem like a good code complexity vs. efficiency tradeoff; if
STL had a ring buffer I'd use it, but I really don't see this as a significant
cost.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:100: int max_level,
On 2013/05/13 00:34:51, gauravsh wrote:
> Why is this not a LogLevel?

Done.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:141: // Iterate backwards through the
list, decramenting the offset for entries
On 2013/05/13 00:34:51, gauravsh wrote:
> decrementing

Done.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:146: if (--offset == 0)
On 2013/05/13 00:34:51, gauravsh wrote:
> BUG? Wouldn't this end up decrementing the offset more than necessary? If I
have
> 5 log entries such as:
> 1,INFO
> 2,ERROR
> 3,INFO
> 4,ERROR
> 5,INFO
> 
> GetAsString(OLDEST_FIRST, .., ERROR, 1) should return "4,ERROR"
> 
> However, according to this code:
> line 138:  sets |offset| = 5 - 1 = 4
> line 146:  --offset executed 3 times (# of log entries at the INFO level)
> line 151: Final |offset| = 4 - 3 = 1
> lines 152-155 would then return: 
>   2,ERROR
>   4,ERROR
> ...which is incorrect.

You're right, I need to check the shown events against max_events. Will add a
test to cover this.
Done.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:198: std::string filestr;
On 2013/05/13 13:16:25, pneubeck wrote:
> FilePath::BaseName()

Done.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:207: VLOG(1) <<
entry.ToString("time,file,desc");
On 2013/05/13 13:16:25, pneubeck wrote:
> this should respect the log level.

Done.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
File chromeos/network/network_event_log.h (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:56: CHROMEOS_EXPORT void AddEntry(const
char* file,
On 2013/05/13 00:34:51, gauravsh wrote:
> When would one use this directly? If this is not meant to be used directly,
lets
> move this to an internal namespace to make that clearer.
The unit tests call it directly. Moved to 'internal'.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:65: // can be combined with or without
separators, e.g. "time,desc".
On 2013/05/13 13:16:25, pneubeck wrote:
> you're parsing the format string per entry. Since the rational behind this log
> was high-frequent events: Please replace these strings by much more efficient
> bit flags (in an enum) which can be bit-wise OR-ed and AND-ed.
> 
> (if there is a strong argument to continue with strings:
> remove "without separators" and mention a specific separator. Otherwise this
is
> very fragile wrt. future changes.)
Made the optimization to only parse the string in GetAsString. Changed to an
explicit separator.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:71: // Only events with |log_level| >=
|max_level| are included in the output.
On 2013/05/13 00:34:51, gauravsh wrote:
> This should be |log_level| <= |max_level|.
> 

Done.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:79: CHROMEOS_EXPORT std::string
DBusValueAsString(const base::Value& value);
On 2013/05/13 13:16:25, pneubeck wrote:
> On 2013/05/13 00:34:51, gauravsh wrote:
> > NetworkEventLog is an odd place for this. I think this should be in its own
> > utility module with other helpers that work on *Value-s. (e.g. 
> > ManagedState::Get*Value , ConvertListValueToStringVector in network_state,
> etc.)
> 
> There is already:
>  operator<< in values.h
> and
>  JsonWriter
> 
> This function should at least delegate to one of these.
> The method name is unnecessarily restrictive.
> 
> +1 to place this into a helper file that is as near to base/values.h as
> possible, like base/values_util.h

Done.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:89: // Non-essencial debugging events
On 2013/05/13 00:34:51, gauravsh wrote:
> Non-essential

Done.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:96: __FILE__, __LINE__, log_level, event,
description)
On 2013/05/13 13:16:25, pneubeck wrote:
> should be FROM_HERE
FROM_HERE uses tracked_objects; I think that is more complexity than we want or
require for logging (see comment in ShillErrorCallbackFunction).

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
File chromeos/network/network_event_log_unittest.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log_unittest.cc:169:
AddEventsAtLevel(network_event_log::LOG_LEVEL_DEBUG, 1);
On 2013/05/13 13:16:25, pneubeck wrote:
> please add two more groups, so that there are two separated groups (also not
at
> the border) which are filtered out in the subsequent tests. something like:
> 
> a:0 b:2 c:0 d:0 e:1 f:0  ==(last 2, =< 0)==> d:0 f:0

Not clear what you meant by a,b,c,etc, but I separated the max test, made it
less trivial, and fixed the code to address the logic error that gaurav pointed
out.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_h...
File chromeos/network/network_handler_callbacks.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_h...
chromeos/network/network_handler_callbacks.cc:38: NET_LOG_ERROR(error,
error_message);
On 2013/05/13 13:16:25, pneubeck wrote:
> the file/line that is logged by NET_LOG_ERROR will not be helpful in this case
> and is not a replacement for |module|.
> Instead |module| should be replaced by base::Location and callers should use
> FROM_HERE.

I considered that, but I really think that is overkill. We pretty rarely make
the same shill call from multiple places; I do not think there is likely to be
any ambiguity. We can always add info to the callback and call
network_event_log::AddEntry directly if we need to.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:58: std::string
GetManagedStateLogName(const chromeos::ManagedState* network) {
On 2013/05/13 13:16:25, pneubeck wrote:
> either the function name and argument type are wrong or the argument-name.
> 
> rename 'network' -> 'state'

Done.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:300:
NET_LOG_EVENT("SetConnectingNetwork", GetNameFromPath(service_path));
On 2013/05/13 13:16:25, pneubeck wrote:
> Does GetNameFromPath really have to exposed in the header? This is the only
> place where it's used (after fixing my other comment)? Do we need it at all?
> 
> If so, restrict it at least to Networks, so that the ambiguity of service
paths
> ("is it a network or a device?") is gone.
Eliminated GetNameFromPath

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:392:
NET_LOG_DEBUG("PropertiesReceived", GetNameFromPath(path));
On 2013/05/13 13:16:25, pneubeck wrote:
> should use GetManagedStateLogName(managed) instead.

Done.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:609: const std::string& path) const {
On 2013/05/13 13:16:25, pneubeck wrote:
> after fixing my other comment, this is only used for networks. no need to
guess.
Eliminated GetNameFromPath

[gauravsh, 2013-05-14 00:01:39]

https://codereview.chromium.org/14876021/diff/31002/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/about_network.h (right):

https://codereview.chromium.org/14876021/diff/31002/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/about_network.h:15: // refresh interval and
whther to include debug log entries.
whether

https://codereview.chromium.org/14876021/diff/31002/chromeos/network/network_...
File chromeos/network/network_event_log.cc (right):

https://codereview.chromium.org/14876021/diff/31002/chromeos/network/network_...
chromeos/network/network_event_log.cc:195: if (++shown_events >= max_events)
You want an else here, or this will bail too early*. shown_events should only be
incremented if the entry wouldn't be skipped.

You could consider rewriting the code by just keeping track of |shown_events|
and then before breaking out of the loop, computing the offset based on the
current position of the iterator. That's probably more readable and less error
prone.

[*Can be verified by the following addition to
NetworkEventLogTest.TestMaxEvents:

output = network_event_log::GetAsString(
      network_event_log::OLDEST_FIRST, "",
      network_event_log::LOG_LEVEL_ERROR, 2);
EXPECT_EQ("error2\nerror4\n", output);
]

https://codereview.chromium.org/14876021/diff/31002/chromeos/network/network_...
File chromeos/network/network_event_log.h (right):

https://codereview.chromium.org/14876021/diff/31002/chromeos/network/network_...
chromeos/network/network_event_log.h:57: // NOTE: Generally use NET_LOG instead.
Probably don't need this note anymore.

https://codereview.chromium.org/14876021/diff/31002/chromeos/network/network_...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/14876021/diff/31002/chromeos/network/network_...
chromeos/network/network_state_handler_unittest.cc:278: // Changing a service to
the exsiting value should not trigger an update.
existing

[stevenjb, 2013-05-14 00:27:42]

https://codereview.chromium.org/14876021/diff/31002/chrome/browser/ui/webui/c...
File chrome/browser/ui/webui/chromeos/about_network.h (right):

https://codereview.chromium.org/14876021/diff/31002/chrome/browser/ui/webui/c...
chrome/browser/ui/webui/chromeos/about_network.h:15: // refresh interval and
whther to include debug log entries.
On 2013/05/14 00:01:39, gauravsh wrote:
> whether

Done.

https://codereview.chromium.org/14876021/diff/31002/chromeos/network/network_...
File chromeos/network/network_event_log.cc (right):

https://codereview.chromium.org/14876021/diff/31002/chromeos/network/network_...
chromeos/network/network_event_log.cc:195: if (++shown_events >= max_events)
On 2013/05/14 00:01:39, gauravsh wrote:
> You want an else here, or this will bail too early*. shown_events should only
be
> incremented if the entry wouldn't be skipped.
> 
> You could consider rewriting the code by just keeping track of |shown_events|
> and then before breaking out of the loop, computing the offset based on the
> current position of the iterator. That's probably more readable and less error
> prone.
> 
> [*Can be verified by the following addition to
> NetworkEventLogTest.TestMaxEvents:
> 
> output = network_event_log::GetAsString(
>       network_event_log::OLDEST_FIRST, "",
>       network_event_log::LOG_LEVEL_ERROR, 2);
> EXPECT_EQ("error2\nerror4\n", output);
> ]

Bah, where's my brain, must be Monday. I was thinking else when I added it...

Yeah, not a huge fan of iterator subtraction, but might be more clear... Added a
couple more tests, but please triple check my logic...

Done.

https://codereview.chromium.org/14876021/diff/31002/chromeos/network/network_...
File chromeos/network/network_event_log.h (right):

https://codereview.chromium.org/14876021/diff/31002/chromeos/network/network_...
chromeos/network/network_event_log.h:57: // NOTE: Generally use NET_LOG instead.
On 2013/05/14 00:01:39, gauravsh wrote:
> Probably don't need this note anymore.

I think it's still worth noting.

https://codereview.chromium.org/14876021/diff/31002/chromeos/network/network_...
File chromeos/network/network_state_handler_unittest.cc (right):

https://codereview.chromium.org/14876021/diff/31002/chromeos/network/network_...
chromeos/network/network_state_handler_unittest.cc:278: // Changing a service to
the exsiting value should not trigger an update.
On 2013/05/14 00:01:39, gauravsh wrote:
> existing

Done.

[gauravsh, 2013-05-14 23:01:20]

lgtm

https://codereview.chromium.org/14876021/diff/53002/chromeos/network/managed_...
File chromeos/network/managed_state.cc (right):

https://codereview.chromium.org/14876021/diff/53002/chromeos/network/managed_...
chromeos/network/managed_state.cc:63: bool new_value;
I think you need to rebase since this is now part of a separate change.

https://codereview.chromium.org/14876021/diff/53002/chromeos/network/network_...
File chromeos/network/network_event_log.cc (right):

https://codereview.chromium.org/14876021/diff/53002/chromeos/network/network_...
chromeos/network/network_event_log.cc:185: // Iterate backwards through the
list, decrementing the offset for entries
update the comment

[stevenjb, 2013-05-15 16:20:42]

Rebased.
pneubeck@, PTAL

https://codereview.chromium.org/14876021/diff/53002/chromeos/network/network_...
File chromeos/network/network_event_log.cc (right):

https://codereview.chromium.org/14876021/diff/53002/chromeos/network/network_...
chromeos/network/network_event_log.cc:185: // Iterate backwards through the
list, decrementing the offset for entries
On 2013/05/14 23:01:20, gauravsh wrote:
> update the comment

Done.

[pneubeck (no reviews), 2013-05-16 08:21:06]

Mostly minor issues that you should judge yourself.
lgtm.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
File chromeos/network/network_event_log.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.cc:31: std::string file;
On 2013/05/13 19:36:39, stevenjb (chromium) wrote:
> On 2013/05/13 13:16:25, pneubeck wrote:
> > base::Location
> Again, more complexity than I think we want, see ShillErrorCallbackFunction
> comment.

With base::Location, the filename would not have to be copied for each entry.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
File chromeos/network/network_event_log.h (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_e...
chromeos/network/network_event_log.h:96: __FILE__, __LINE__, log_level, event,
description)
On 2013/05/13 19:36:39, stevenjb (chromium) wrote:
> On 2013/05/13 13:16:25, pneubeck wrote:
> > should be FROM_HERE
> FROM_HERE uses tracked_objects;
What do you mean by "uses"? tracked_objects is the namespace.

> I think that is more complexity than we want or
> require for logging (see comment in ShillErrorCallbackFunction).
Which complexity are you referring to?
E.g. the number of arguments is reduced => there is less to pass around => less
code complexity.


However, this is not essential. Feel free to leave it.

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_h...
File chromeos/network/network_handler_callbacks.cc (right):

https://codereview.chromium.org/14876021/diff/2001/chromeos/network/network_h...
chromeos/network/network_handler_callbacks.cc:38: NET_LOG_ERROR(error,
error_message);
On 2013/05/13 19:36:39, stevenjb (chromium) wrote:
> On 2013/05/13 13:16:25, pneubeck wrote:
> > the file/line that is logged by NET_LOG_ERROR will not be helpful in this
case
> > and is not a replacement for |module|.
> > Instead |module| should be replaced by base::Location and callers should use
> > FROM_HERE.
> 
> I considered that, but I really think that is overkill. We pretty rarely make
> the same shill call from multiple places; I do not think there is likely to be
> any ambiguity. We can always add info to the callback and call
> network_event_log::AddEntry directly if we need to.
> 

I don't see a overkill on the perfomance side: the copy of a base::Location
should be negligible compared to the string operations.

Code complexity wise: arguable.

Since you seem to use the log messages currently more than I, please feel free
to leave it as is.

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
File chromeos/network/network_event_log.cc (right):

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
chromeos/network/network_event_log.cc:69: line += net::EscapeForHTML(
why use EscapeForHTML if !format_html?
maybe make one IF block for format_html and !format_html, the interleaving of
html looks confusing.

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
chromeos/network/network_event_log.cc:75: line += "<i>";
italic/bold without show_desc shouldn't print anything but does?

!show_desc && format_html => <b></b>

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
File chromeos/network/network_event_log_unittest.cc (right):

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
chromeos/network/network_event_log_unittest.cc:17: 
could be network_event_log namespace, simplifying the rest of the file.

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
chromeos/network/network_event_log_unittest.cc:204:
network_event_log::OLDEST_FIRST, "",
please test the combination with NEWEST_FIRST as well.

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
chromeos/network/network_state_handler.cc:433: if (!vstr.empty())
write should never fail. empty should be a valid value, hiding it may lead to
confusion.
IIRC, write accepts an argument to use quotes for strings, which might be handy
here.

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
chromeos/network/network_state_handler.cc:456: if (!vstr.empty())
same here.

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/shill_pr...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/shill_pr...
chromeos/network/shill_property_handler.cc:209: if (!vstr.empty())
same here.

[stevenjb, 2013-05-16 17:09:53]

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
File chromeos/network/network_event_log.cc (right):

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
chromeos/network/network_event_log.cc:69: line += net::EscapeForHTML(
On 2013/05/16 08:21:07, pneubeck wrote:
> why use EscapeForHTML if !format_html?
> maybe make one IF block for format_html and !format_html, the interleaving of
> html looks confusing.
Done.

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
chromeos/network/network_event_log.cc:75: line += "<i>";
On 2013/05/16 08:21:07, pneubeck wrote:
> italic/bold without show_desc shouldn't print anything but does?
> 
> !show_desc && format_html => <b></b>
'event' is always shown. I'll separate out the "text" (event+desc) section to
make this more readable.

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
File chromeos/network/network_event_log_unittest.cc (right):

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
chromeos/network/network_event_log_unittest.cc:17: 
On 2013/05/16 08:21:07, pneubeck wrote:
> could be network_event_log namespace, simplifying the rest of the file.

I prefer to explicitly qualify global namespaced methods, even in the unit test,
but I think for the types you're right.
Done.

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
chromeos/network/network_event_log_unittest.cc:204:
network_event_log::OLDEST_FIRST, "",
On 2013/05/16 08:21:07, pneubeck wrote:
> please test the combination with NEWEST_FIRST as well.

Done.

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/network_...
chromeos/network/network_state_handler.cc:433: if (!vstr.empty())
On 2013/05/16 08:21:07, pneubeck wrote:
> write should never fail. empty should be a valid value, hiding it may lead to
> confusion.
> IIRC, write accepts an argument to use quotes for strings, which might be
handy
> here.
It could be an empty string, and I did not see that option (except 'pretty'
which could be multi-line).
I added '' for empty strings and wrapped this back in a utility function, but
using JSONWriter.

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/shill_pr...
File chromeos/network/shill_property_handler.cc (right):

https://codereview.chromium.org/14876021/diff/67001/chromeos/network/shill_pr...
chromeos/network/shill_property_handler.cc:209: if (!vstr.empty())
On 2013/05/16 08:21:07, pneubeck wrote:
> same here.
ditto

[commit-bot: I haz the power, 2013-05-16 17:10:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/14876021/80001

[commit-bot: I haz the power, 2013-05-16 17:21:59]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-16 18:25:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/14876021/80001

[commit-bot: I haz the power, 2013-05-16 21:29:56]

Retried try job too often on win_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[stevenjb, 2013-05-16 21:56:13]

Committed patchset #9 manually as r200632 (presubmit successful).