Implement LoadLog, and hook up HostResolverImpl to LoadLog....

Implement LoadLog, and hook up HostResolverImpl to LoadLog.

The functionality in load_log_util.h is currently unused, but since it motivates the implementation of LoadLog, figured it should be included with this review.

Note that I decided against defining the loggable event types as LoadStates, since I wanted more flexibility in adding crazy stuff, and they really seem too implementation specific to be in LoadStates.

BUG=http://crbug.com/14478
TEST=net_unittests

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

[eroman, 2009-08-12 22:08:31]

Apologies this is kinda long.
I recommend browsing the changes in the following order:

  load_log_util.h
  load_log_util_unittest.cc 
  host_resolver_impl.cc
  load_log.h

(and the rest in whatever order).

[darin (slow to review), 2009-08-13 21:50:29]

Looks good overall.

http://codereview.chromium.org/165404/diff/29/40
File net/base/load_log.h (right):

http://codereview.chromium.org/165404/diff/29/40#newcode29
Line 29: kNone,
I remember a style discussion about not adopting kFoo for enums and just
sticking instead with FOO for consistency with the rest of our codebase.

http://codereview.chromium.org/165404/diff/29/40#newcode66
Line 66: static void AddEvent(LoadLog* log, base::TimeTicks t, EventType event)
{
it seems like computing the time could be done inside this fucntion.  maybe
there could be variants on AddEvent, BeginEvent, and EndEvent for the few cases
where a consumer wants to log a timestamp that doesn't correspond to "now"

http://codereview.chromium.org/165404/diff/29/40#newcode97
Line 97: EventList events_;
maybe it would be better to just allocate events_ inline as a C-style array? 
you could also use StackVector from base/stack_container.h if you still want it
to be like a std::vector.

http://codereview.chromium.org/165404/diff/29/38
File net/base/load_log_event_type_list.h (right):

http://codereview.chromium.org/165404/diff/29/38#newcode14
Line 14: EVENT_TYPE(kLogTruncated)
nit: LOG_TRUNCATED since these are enums.

http://codereview.chromium.org/165404/diff/29/36
File net/base/load_log_util.cc (right):

http://codereview.chromium.org/165404/diff/29/36#newcode47
Line 47: kSpacesPerIndentation) + (max_type_width - event_str.size());
nit: 4 space indent for line continuation

[eroman, 2009-08-13 23:05:10]

http://codereview.chromium.org/165404/diff/29/40
File net/base/load_log.h (right):

http://codereview.chromium.org/165404/diff/29/40#newcode29
Line 29: kNone,
On 2009/08/13 21:50:29, darin wrote:
> I remember a style discussion about not adopting kFoo for enums and just
> sticking instead with FOO for consistency with the rest of our codebase.

Done.

I changed the two enums to:

  PHASE_*
  TYPE_*

(In fact I prefer the old enum-style; I find it more readable)

http://codereview.chromium.org/165404/diff/29/40#newcode66
Line 66: static void AddEvent(LoadLog* log, base::TimeTicks t, EventType event)
{
On 2009/08/13 21:50:29, darin wrote:
> it seems like computing the time could be done inside this fucntion.  maybe
> there could be variants on AddEvent, BeginEvent, and EndEvent for the few
cases
> where a consumer wants to log a timestamp that doesn't correspond to "now"

Done.

Yes, that is definitely where it should be done for the static wrappers (also
avoids the time lookup when LoadLog* is NULL).

http://codereview.chromium.org/165404/diff/29/40#newcode97
Line 97: EventList events_;
On 2009/08/13 21:50:29, darin wrote:
> maybe it would be better to just allocate events_ inline as a C-style array? 
> you could also use StackVector from base/stack_container.h if you still want
it
> to be like a std::vector.

I tried switching to StackVector but it seems to hang in the destructor. How
about I tackle this as a follow-up ?

[darin (slow to review), 2009-08-13 23:55:05]

http://codereview.chromium.org/165404/diff/29/40
File net/base/load_log.h (right):

http://codereview.chromium.org/165404/diff/29/40#newcode97
Line 97: EventList events_;
It's optional.  The code is fine as is.  I only mentioned StackVector since you
were taking care to do reserve(max/2).

[darin (slow to review), 2009-08-13 23:57:49]

LGTM

http://codereview.chromium.org/165404/diff/1021/69
File net/base/load_log_util.h (right):

http://codereview.chromium.org/165404/diff/1021/69#newcode87
Line 87: LoadLogUtil() {}
nit: DISALLOW_IMPLICIT_CONSTRUCTORS also does the job.

[eroman, 2009-08-14 03:02:05]

committed.

http://codereview.chromium.org/165404/diff/1021/69
File net/base/load_log_util.h (right):

http://codereview.chromium.org/165404/diff/1021/69#newcode87
Line 87: LoadLogUtil() {}
On 2009/08/13 23:57:49, darin wrote:
> nit: DISALLOW_IMPLICIT_CONSTRUCTORS also does the job.

Done.