Add DBusStatistics and DBusLogSource to log and show dbus stats

Add DBusStatistics and DBusLogSource to log and show dbus stats

The intention of this is to provide low overhead detailed logging to ensure that dbus call counts remain reasonable as we migrate NetworkLibrary and other systtems from src/chrome to src/chromeos.

We already have UMA stats which provide high level numbers that we can watch, but this will make detailed debugging available for advanced users and in feedback reports.


BUG=159635
For chrome/chrome_browser_chromeos.gypi:
TBR=sky@chromium.org

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167742

[satorux1, 2012-11-12 00:40:43]

looks like useful info!

http://codereview.chromium.org/11363173/diff/5001/chromeos/dbus/debug_daemon_...
File chromeos/dbus/debug_daemon_client.cc (right):

http://codereview.chromium.org/11363173/diff/5001/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.cc:573: base::Bind(callback, false, ""));
Changes in this file seem to be unrelated to the main purpose of the patch.
Maybe create a separate patch?

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:65: StatSet& stats = g_dbus_statistics->stats_;
const StatSet& ?

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h
File dbus/dbus_statistics.h (right):

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:17: class CHROME_DBUS_EXPORT DbusStatistics {
Please write some class comment.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:17: class CHROME_DBUS_EXPORT DbusStatistics {
Dbus -> DBus, to be consistent with other classes in this directory.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:36: bool Compare(const Stat& other) const;
Returns true when?

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:51: // Static methods do nothing uless Initialize() was
called.
Please also write what these functions do.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:52: static void AddSentCall(const std::string& service,
Call -> MethodCall to be a bit more descriptive?

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:55: static void AddReceivedCall(const std::string&
service,
ditto.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:62: static std::string GetAsString(ShowInString show,
bool show_per_minute);
boolean parameter like this makes the client code cryptic. (it's unclear what
'true' means). Maybe just go with one parameter like:

GetAsString(ShowOptions options);

Where ShowOptions is a bit-field of options, like 

  SHOW_INTERFACE = 1,
  SHOW_METHOD = 1 << 1,
  SHOW_PER_MINUTE = 1 << 2,

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:64: // Testing
Please write a function comment.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics_unitte...
File dbus/dbus_statistics_unittest.cc (right):

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics_unitte...
dbus/dbus_statistics_unittest.cc:86: }
Add a test for GetAsString()? Having a test for the function would be useful, as
we can see how the output from the function looks like.

http://codereview.chromium.org/11363173/diff/5001/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/11363173/diff/5001/dbus/object_proxy.cc#newcode87
dbus/object_proxy.cc:87: DbusStatistics::AddSentCall(service_name_,
This is a synchronous method call. We might want to distinguish sync and async
just like we do for the UMA histogram.

[hashimoto, 2012-11-12 04:33:16]

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_logs/dbus_log_source.cc (right):

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_logs/dbus_log_source.cc:7: #include
"base/message_loop.h"
nit: No need to include this.

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_logs/dbus_log_source.cc:17: }
nit: Add commenct "// namespace"

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_logs/dbus_log_source.cc:23:
scoped_ptr<SystemLogsResponse> response(new SystemLogsResponse);
Why do you store this object in scoped_ptr?
Doesn't "SystemLogResponse response;" work?

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right):

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:13: #include
"chrome/browser/chromeos/system_logs/dbus_log_source.h"
nit: please sort.

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:31:
data_sources_.push_back(new DbusLogSource());
nit: Please sort.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:16: }
nit: Please add "// namespace"

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h
File dbus/dbus_statistics.h (right):

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:43: DbusStatistics();
Since all public methods are static, how about hiding ctor/dtor and other
private methods of this class in the unnamed namespace of dbus_statictics.cc?
Or, how about add a static method "GetInstance()" as DBusThreadManager is doing
and make all methods non-static?

[satorux1, 2012-11-12 04:43:17]

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h
File dbus/dbus_statistics.h (right):

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:43: DbusStatistics();
On 2012/11/12 04:33:16, hashimoto wrote:
> Since all public methods are static, how about hiding ctor/dtor and other
> private methods of this class in the unnamed namespace of dbus_statictics.cc?
> Or, how about add a static method "GetInstance()" as DBusThreadManager is
doing
> and make all methods non-static?

Good ideas. I think we can hide the DbusStatistics class from .h file completely
like:

namespace dbus {
namespace statistics {

void AddSentMethodCall(...);

...

[stevenjb, 2012-11-12 19:46:40]

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_logs/dbus_log_source.cc (right):

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_logs/dbus_log_source.cc:7: #include
"base/message_loop.h"
On 2012/11/12 04:33:16, hashimoto wrote:
> nit: No need to include this.

Done.

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_logs/dbus_log_source.cc:17: }
On 2012/11/12 04:33:16, hashimoto wrote:
> nit: Add commenct "// namespace"
We usually don't bother when it's within a few lines with no whitespace.

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_logs/dbus_log_source.cc:23:
scoped_ptr<SystemLogsResponse> response(new SystemLogsResponse);
On 2012/11/12 04:33:16, hashimoto wrote:
> Why do you store this object in scoped_ptr?
> Doesn't "SystemLogResponse response;" work?

Yes actually, it looks like it can. The code that calls this is a bit confusing
(one callback type requires a scoped_ptr, but this one does not.
Done.

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_logs/system_logs_fetcher.cc (right):

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:13: #include
"chrome/browser/chromeos/system_logs/dbus_log_source.h"
On 2012/11/12 04:33:16, hashimoto wrote:
> nit: please sort.

Done.

http://codereview.chromium.org/11363173/diff/5001/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_logs/system_logs_fetcher.cc:31:
data_sources_.push_back(new DbusLogSource());
On 2012/11/12 04:33:16, hashimoto wrote:
> nit: Please sort.

Done.

http://codereview.chromium.org/11363173/diff/5001/chromeos/dbus/debug_daemon_...
File chromeos/dbus/debug_daemon_client.cc (right):

http://codereview.chromium.org/11363173/diff/5001/chromeos/dbus/debug_daemon_...
chromeos/dbus/debug_daemon_client.cc:573: base::Bind(callback, false, ""));
On 2012/11/12 00:40:43, satorux1 wrote:
> Changes in this file seem to be unrelated to the main purpose of the patch.
> Maybe create a separate patch?

Sorry, meant to mention that. These were from
https://chromiumcodereview.appspot.com/11364169/, now in ToT.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:16: }
On 2012/11/12 04:33:16, hashimoto wrote:
> nit: Please add "// namespace"
Re-factored

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:65: StatSet& stats = g_dbus_statistics->stats_;
On 2012/11/12 00:40:43, satorux1 wrote:
> const StatSet& ?

Done.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h
File dbus/dbus_statistics.h (right):

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:17: class CHROME_DBUS_EXPORT DbusStatistics {
On 2012/11/12 00:40:43, satorux1 wrote:
> Please write some class comment.

Done.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:17: class CHROME_DBUS_EXPORT DbusStatistics {
On 2012/11/12 00:40:43, satorux1 wrote:
> Please write some class comment.

Done.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:36: bool Compare(const Stat& other) const;
On 2012/11/12 00:40:43, satorux1 wrote:
> Returns true when?
Compare == < is pretty standard, but added a comment at the top of the struct.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:43: DbusStatistics();
On 2012/11/12 04:43:17, satorux1 wrote:
> On 2012/11/12 04:33:16, hashimoto wrote:
> > Since all public methods are static, how about hiding ctor/dtor and other
> > private methods of this class in the unnamed namespace of
dbus_statictics.cc?
> > Or, how about add a static method "GetInstance()" as DBusThreadManager is
> doing
> > and make all methods non-static?
> 
> Good ideas. I think we can hide the DbusStatistics class from .h file
completely
> like:
> 
> namespace dbus {
> namespace statistics {
> 
> void AddSentMethodCall(...);
> 
> ...

That makes sense. I think I started this expecting that more of this would need
to be exposed as a class.
Done.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:51: // Static methods do nothing uless Initialize() was
called.
On 2012/11/12 00:40:43, satorux1 wrote:
> Please also write what these functions do.

Done.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:52: static void AddSentCall(const std::string& service,
On 2012/11/12 00:40:43, satorux1 wrote:
> Call -> MethodCall to be a bit more descriptive?

Done.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:55: static void AddReceivedCall(const std::string&
service,
On 2012/11/12 00:40:43, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:62: static std::string GetAsString(ShowInString show,
bool show_per_minute);
On 2012/11/12 00:40:43, satorux1 wrote:
> boolean parameter like this makes the client code cryptic. (it's unclear what
> 'true' means). Maybe just go with one parameter like:
> 
> GetAsString(ShowOptions options);
> 
> Where ShowOptions is a bit-field of options, like 
> 
>   SHOW_INTERFACE = 1,
>   SHOW_METHOD = 1 << 1,
>   SHOW_PER_MINUTE = 1 << 2,
> 
>   

I find bitfields to be error-prone; C++ doesn't handle them very well. Changed
show_per_minute as a separate enum.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:64: // Testing
On 2012/11/12 00:40:43, satorux1 wrote:
> Please write a function comment.

Done.

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics_unitte...
File dbus/dbus_statistics_unittest.cc (right):

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics_unitte...
dbus/dbus_statistics_unittest.cc:86: }
On 2012/11/12 00:40:43, satorux1 wrote:
> Add a test for GetAsString()? Having a test for the function would be useful,
as
> we can see how the output from the function looks like.

I think that this would be more likely to cause breakage if we change (or
localize) the formatting then catch anything helpful. I would rather describe
the output format in the header (done).

http://codereview.chromium.org/11363173/diff/5001/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/11363173/diff/5001/dbus/object_proxy.cc#newcode87
dbus/object_proxy.cc:87: DbusStatistics::AddSentCall(service_name_,
On 2012/11/12 00:40:43, satorux1 wrote:
> This is a synchronous method call. We might want to distinguish sync and async
> just like we do for the UMA histogram.
That's makes sense. Done.

[gauravsh, 2012-11-12 21:41:57]

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics_unitte...
File dbus/dbus_statistics_unittest.cc (right):

http://codereview.chromium.org/11363173/diff/5001/dbus/dbus_statistics_unitte...
dbus/dbus_statistics_unittest.cc:86: }
On 2012/11/12 19:46:40, stevenjb (chromium) wrote:
> On 2012/11/12 00:40:43, satorux1 wrote:
> > Add a test for GetAsString()? Having a test for the function would be
useful,
> as
> > we can see how the output from the function looks like.
> 
> I think that this would be more likely to cause breakage if we change (or
> localize) the formatting then catch anything helpful. I would rather describe
> the output format in the header (done).
> 

Surely, if one was changing the GetAsString method, you would expect them to
rebase the unit test too? It is more likely that the comment gets stale as
changes are made to GetAsString. It's not a trivial implementation either -
without a test, there is no way to be sure that it works as advertised.

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:46: DCHECK(lhs != NULL && rhs != NULL);
I'd be consistent with the pointer checks in the file (so this should be
DCHECK(lhs && rhs);

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:67: // Eunum to specify which field in Stat to incrament
in AddStat
NIT: Enum, increment

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:164: return "DBusStatistics not intitialized.";
NIT: initialized

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:178: StatSet::const_iterator cur_iter = iter++;
Please do not use the postincrement operator with iterator types.

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:185: if (iter == stats.end() ||
I'd suggest inverting this conditional (and adding a continue) to get rid of one
level of nesting.

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.h
File dbus/dbus_statistics.h (right):

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:37: // Add sent/received calls to the statistcs gathering
class. These methods
NIT: statistics

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:38: // do nothing uless Initialize() was called.
NIT: unless

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics_unitte...
File dbus/dbus_statistics_unittest.cc (right):

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics_unitte...
dbus/dbus_statistics_unittest.cc:68: // incramented correctly.
NIT: incremented

http://codereview.chromium.org/11363173/diff/3013/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/11363173/diff/3013/dbus/object_proxy.cc#newcod...
dbus/object_proxy.cc:449: statistics::AddReceivedMethodCall(service_name_,
interface, member);
HandleMessage only deals with signals, and doesn't handle method calls. So this
statistic will count the number of signals, not the number of received method
calls.

[stevenjb, 2012-11-12 22:34:40]

It is likely that the output format for GetAsString will evolve over time. We
shouldn't write code that depends on the output format. Adding a unit test would
only prevent breakage if we did rely on the output format, and would imply that
the format is fixed (i.e. that there might be such dependencies).

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:46: DCHECK(lhs != NULL && rhs != NULL);
On 2012/11/12 21:41:58, gauravsh wrote:
> I'd be consistent with the pointer checks in the file (so this should be
> DCHECK(lhs && rhs); 

Done.

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:67: // Eunum to specify which field in Stat to incrament
in AddStat
On 2012/11/12 21:41:58, gauravsh wrote:
> NIT: Enum, increment

Done.

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:164: return "DBusStatistics not intitialized.";
On 2012/11/12 21:41:58, gauravsh wrote:
> NIT: initialized

Done.

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:178: StatSet::const_iterator cur_iter = iter++;
On 2012/11/12 21:41:58, gauravsh wrote:
> Please do not use the postincrement operator with iterator types.

Postincrament is exactly what we want here, (preincrament would be incorrect).

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:185: if (iter == stats.end() ||
On 2012/11/12 21:41:58, gauravsh wrote:
> I'd suggest inverting this conditional (and adding a continue) to get rid of
one
> level of nesting.
Good suggestion. Done.

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.h
File dbus/dbus_statistics.h (right):

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:37: // Add sent/received calls to the statistcs gathering
class. These methods
On 2012/11/12 21:41:58, gauravsh wrote:
> NIT: statistics

Done.

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:38: // do nothing uless Initialize() was called.
On 2012/11/12 21:41:58, gauravsh wrote:
> NIT: unless

Done.

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics_unitte...
File dbus/dbus_statistics_unittest.cc (right):

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics_unitte...
dbus/dbus_statistics_unittest.cc:68: // incramented correctly.
On 2012/11/12 21:41:58, gauravsh wrote:
> NIT: incremented

Done.

http://codereview.chromium.org/11363173/diff/3013/dbus/object_proxy.cc
File dbus/object_proxy.cc (right):

http://codereview.chromium.org/11363173/diff/3013/dbus/object_proxy.cc#newcod...
dbus/object_proxy.cc:449: statistics::AddReceivedMethodCall(service_name_,
interface, member);
On 2012/11/12 21:41:58, gauravsh wrote:
> HandleMessage only deals with signals, and doesn't handle method calls. So
this
> statistic will count the number of signals, not the number of received method
> calls. 
Well, we don't really receive method calls, but this will (usually) result in a
method call. Still, I understand the potential confusion and will rename this to
ReceviedSignal. (I'd rather track the number of signals received than move this
lower and just track the ones that result in a method call).

[gauravsh, 2012-11-12 23:07:34]

I'd say a unit test does more than help catch regressions, it also helps codify
the expected behavior/output and correctness of code, and makes it easier for
someone in the future to improve/re-factor it. 

Anyway, since this is just going to be output to the chrome://system page, and
not meant to be machine readable, this is fine. I assume you have already
locally verified that this produces the expected output?

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:178: StatSet::const_iterator cur_iter = iter++;
On 2012/11/12 22:34:40, stevenjb (chromium) wrote:
> On 2012/11/12 21:41:58, gauravsh wrote:
> > Please do not use the postincrement operator with iterator types.
> 
> Postincrament is exactly what we want here, (preincrament would be incorrect).

Sorry, I should have been clearer. Google C++/Chromium style specifies that
postincrement shouldn't be used for iterator/complex types - so I was asking to
rewrite this using preincrement.

[gauravsh, 2012-11-12 23:07:59]

LGTM modulo the postincrement nit.

[stevenjb, 2012-11-12 23:17:57]

On 2012/11/12 23:07:34, gauravsh wrote:
> I'd say a unit test does more than help catch regressions, it also helps
codify
> the expected behavior/output and correctness of code, and makes it easier for
> someone in the future to improve/re-factor it. 
> 
> Anyway, since this is just going to be output to the chrome://system page, and
> not meant to be machine readable, this is fine. I assume you have already
> locally verified that this produces the expected output?
> 
I guess that was my point, this is just log output, so changing it wouldn't ever
be considered a regression. And yes, the output has been tested locally :)

> http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc
> File dbus/dbus_statistics.cc (right):
> 
>
http://codereview.chromium.org/11363173/diff/3013/dbus/dbus_statistics.cc#new...
> dbus/dbus_statistics.cc:178: StatSet::const_iterator cur_iter = iter++;
> On 2012/11/12 22:34:40, stevenjb (chromium) wrote:
> > On 2012/11/12 21:41:58, gauravsh wrote:
> > > Please do not use the postincrement operator with iterator types.
> > 
> > Postincrament is exactly what we want here, (preincrament would be
incorrect).
> 
> Sorry, I should have been clearer. Google C++/Chromium style specifies that
> postincrement shouldn't be used for iterator/complex types - so I was asking
to
> rewrite this using preincrement.

The only rule I'm familiar with states: "When a variable is incremented (++i or
i++) or decremented (--i or i--) and the value of the expression is not used,
one must decide whether to preincrement (decrement) or postincrement
(decrement)."

In this case we are explicitly using the value of the expression.

I could break it into two lines instead, but that seems less readable to me.

[gauravsh, 2012-11-12 23:27:48]

Here's the relevant style guide section:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc...

I agree about the readability aspect. You could just move the ++iter into the
loop. And then instead of cur_iter, use next_iter=++iter, and use next_iter in
the if-conditional instead of the current iter.

[stevenjb, 2012-11-12 23:43:45]

On 2012/11/12 23:27:48, gauravsh wrote:
> Here's the relevant style guide section:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc...
> 
> I agree about the readability aspect. You could just move the ++iter into the
> loop. And then instead of cur_iter, use next_iter=++iter, and use next_iter in
> the if-conditional instead of the current iter.

Yes, that's the section I pasted from. The important part is "and the value of
the expression is not used". We do use this pattern elsewhere, although not
often.

Anyway, if it's that confusing I'll change it. Your suggestion sounds like it
would increment iter twice, but there are plenty of other ways to do this.

[gauravsh, 2012-11-12 23:49:27]

On 2012/11/12 23:43:45, stevenjb (chromium) wrote:
> On 2012/11/12 23:27:48, gauravsh wrote:
> > Here's the relevant style guide section:
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc...
> > 
> > I agree about the readability aspect. You could just move the ++iter into
the
> > loop. And then instead of cur_iter, use next_iter=++iter, and use next_iter
in
> > the if-conditional instead of the current iter.
> 
> Yes, that's the section I pasted from. The important part is "and the value of
> the expression is not used". We do use this pattern elsewhere, although not
> often.
> 
> Anyway, if it's that confusing I'll change it. Your suggestion sounds like it
> would increment iter twice, but there are plenty of other ways to do this.

Yeah, we are just reading it differently. I don't know which interpretation is
correct - so feel free to keep it as-is.

[satorux1, 2012-11-13 04:11:06]

Re unit test for GetAsString(), I'm with gauravsh@.

When modifying a non-trivial formatter like this, it's fairly easy to make
stupid mistakes like forgetting to add spaces and line breaks properly, which
end up generating broken output. It'd be helpful to be able to test the modified
output, with a unit test, rather than doing it manually from chrome://system.

http://codereview.chromium.org/11363173/diff/4017/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_logs/dbus_log_source.h (right):

http://codereview.chromium.org/11363173/diff/4017/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_logs/dbus_log_source.h:13: class DbusLogSource :
public SystemLogsSource {
Dbus -> DBus to be consistent with stuff in src/dbus

http://codereview.chromium.org/11363173/diff/4017/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

http://codereview.chromium.org/11363173/diff/4017/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:34: int sent_blocking_calls;
sent_method_calls, etc. to be more specific? We can send signals from Chrome for
instance.

http://codereview.chromium.org/11363173/diff/4017/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:71: TYPE_SENT_BLOCKING
likewise, TYPE_SENT_METHOD_CALL, etc. to be more specific?

[hashimoto, 2012-11-13 06:11:44]

http://codereview.chromium.org/11363173/diff/4017/chrome/browser/chromeos/sys...
File chrome/browser/chromeos/system_logs/dbus_log_source.cc (right):

http://codereview.chromium.org/11363173/diff/4017/chrome/browser/chromeos/sys...
chrome/browser/chromeos/system_logs/dbus_log_source.cc:15: const char
kDbusLogEntryLong[] = "dbus_details";
nit: s/Dbus/DBus/

http://codereview.chromium.org/11363173/diff/4017/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

http://codereview.chromium.org/11363173/diff/4017/dbus/dbus_statistics.cc#new...
dbus/dbus_statistics.cc:79: Stat* stat = GetStat(service, interface, method,
true);
DCHECK(stat)?

http://codereview.chromium.org/11363173/diff/4017/dbus/dbus_statistics.h
File dbus/dbus_statistics.h (right):

http://codereview.chromium.org/11363173/diff/4017/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:8: #include <set>
nit: No need to include this.

http://codereview.chromium.org/11363173/diff/4017/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:12: #include "base/time.h"
ditto.

http://codereview.chromium.org/11363173/diff/4017/dbus/dbus_statistics.h#newc...
dbus/dbus_statistics.h:79: //  LocalWords:  ShowInString FormatString
What is this?

http://codereview.chromium.org/11363173/diff/4017/dbus/dbus_statistics_unitte...
File dbus/dbus_statistics_unittest.cc (right):

http://codereview.chromium.org/11363173/diff/4017/dbus/dbus_statistics_unitte...
dbus/dbus_statistics_unittest.cc:16: virtual void SetUp() {
nit: OVERRIDE

http://codereview.chromium.org/11363173/diff/4017/dbus/dbus_statistics_unitte...
dbus/dbus_statistics_unittest.cc:20: virtual void TearDown() {
nit: OVERRIDE

[stevenjb, 2012-11-13 19:54:15]

Addressed feedback.
Added TestGetAsString and a reminder comment so that I don't forget to change
the test every time I update the output text.

https://chromiumcodereview.appspot.com/11363173/diff/4017/chrome/browser/chro...
File chrome/browser/chromeos/system_logs/dbus_log_source.cc (right):

https://chromiumcodereview.appspot.com/11363173/diff/4017/chrome/browser/chro...
chrome/browser/chromeos/system_logs/dbus_log_source.cc:15: const char
kDbusLogEntryLong[] = "dbus_details";
On 2012/11/13 06:11:44, hashimoto wrote:
> nit: s/Dbus/DBus/

Done.

https://chromiumcodereview.appspot.com/11363173/diff/4017/chrome/browser/chro...
File chrome/browser/chromeos/system_logs/dbus_log_source.h (right):

https://chromiumcodereview.appspot.com/11363173/diff/4017/chrome/browser/chro...
chrome/browser/chromeos/system_logs/dbus_log_source.h:13: class DbusLogSource :
public SystemLogsSource {
On 2012/11/13 04:11:06, satorux1 wrote:
> Dbus -> DBus to be consistent with stuff in src/dbus

Done.

https://chromiumcodereview.appspot.com/11363173/diff/4017/dbus/dbus_statistic...
File dbus/dbus_statistics.cc (right):

https://chromiumcodereview.appspot.com/11363173/diff/4017/dbus/dbus_statistic...
dbus/dbus_statistics.cc:34: int sent_blocking_calls;
On 2012/11/13 04:11:06, satorux1 wrote:
> sent_method_calls, etc. to be more specific? We can send signals from Chrome
for
> instance.

Done.

https://chromiumcodereview.appspot.com/11363173/diff/4017/dbus/dbus_statistic...
dbus/dbus_statistics.cc:71: TYPE_SENT_BLOCKING
On 2012/11/13 04:11:06, satorux1 wrote:
> likewise, TYPE_SENT_METHOD_CALL, etc. to be more specific? 

Done.

https://chromiumcodereview.appspot.com/11363173/diff/4017/dbus/dbus_statistic...
dbus/dbus_statistics.cc:79: Stat* stat = GetStat(service, interface, method,
true);
On 2012/11/13 06:11:44, hashimoto wrote:
> DCHECK(stat)?

Done.

https://chromiumcodereview.appspot.com/11363173/diff/4017/dbus/dbus_statistics.h
File dbus/dbus_statistics.h (right):

https://chromiumcodereview.appspot.com/11363173/diff/4017/dbus/dbus_statistic...
dbus/dbus_statistics.h:8: #include <set>
On 2012/11/13 06:11:44, hashimoto wrote:
> nit: No need to include this.

Done.

https://chromiumcodereview.appspot.com/11363173/diff/4017/dbus/dbus_statistic...
dbus/dbus_statistics.h:12: #include "base/time.h"
On 2012/11/13 06:11:44, hashimoto wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/11363173/diff/4017/dbus/dbus_statistic...
dbus/dbus_statistics.h:79: //  LocalWords:  ShowInString FormatString
On 2012/11/13 06:11:44, hashimoto wrote:
> What is this?
oops (spellcheck spam). done.

https://chromiumcodereview.appspot.com/11363173/diff/4017/dbus/dbus_statistic...
File dbus/dbus_statistics_unittest.cc (right):

https://chromiumcodereview.appspot.com/11363173/diff/4017/dbus/dbus_statistic...
dbus/dbus_statistics_unittest.cc:16: virtual void SetUp() {
On 2012/11/13 06:11:44, hashimoto wrote:
> nit: OVERRIDE

Done.

https://chromiumcodereview.appspot.com/11363173/diff/4017/dbus/dbus_statistic...
dbus/dbus_statistics_unittest.cc:20: virtual void TearDown() {
On 2012/11/13 06:11:44, hashimoto wrote:
> nit: OVERRIDE

Done.

[satorux1, 2012-11-13 22:31:04]

LGTM with nits.

Please also s/Dbus/DBus/g in the patch description.

http://codereview.chromium.org/11363173/diff/13/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

http://codereview.chromium.org/11363173/diff/13/dbus/dbus_statistics.cc#newco...
dbus/dbus_statistics.cc:36: int received_method_calls;
received_signals

http://codereview.chromium.org/11363173/diff/13/dbus/dbus_statistics.cc#newco...
dbus/dbus_statistics.cc:73: TYPE_RECEIVED_METHOD_CALLS,
TYPE_RECEIVED_SIGNALS

[stevenjb, 2012-11-13 22:49:01]

http://codereview.chromium.org/11363173/diff/13/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

http://codereview.chromium.org/11363173/diff/13/dbus/dbus_statistics.cc#newco...
dbus/dbus_statistics.cc:36: int received_method_calls;
On 2012/11/13 22:31:04, satorux1 wrote:
> received_signals
oops, right, fixed.

http://codereview.chromium.org/11363173/diff/13/dbus/dbus_statistics.cc#newco...
dbus/dbus_statistics.cc:73: TYPE_RECEIVED_METHOD_CALLS,
On 2012/11/13 22:31:04, satorux1 wrote:
> TYPE_RECEIVED_SIGNALS

Done.

[commit-bot: I haz the power, 2012-11-14 19:08:19]

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

[commit-bot: I haz the power, 2012-11-14 21:03:31]

Change committed as 167742