Update the animation callback histogram to measure only the delay between post and invocation

Update the animation callback histogram to measure only the delay between post and invocation

This avoids abusing a histogram to keep both counts and times (this histogram stores only times) and lets us see the full distribution.

BUG=71256
TEST=

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

[jar (doing other things), 2011-02-18 07:31:29]

I'm convinced that the histograms are what we talked about... so LGTM.

I had a few nits below that would have made reading the code a lot easier for
me, and you might consider the suggested changes.

http://codereview.chromium.org/6547001/diff/1/chrome/renderer/render_widget.cc
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/6547001/diff/1/chrome/renderer/render_widget.c...
chrome/renderer/render_widget.cc:486: if (animation_update_pending_) {
nit: Suggest an early return, rather than indenting the rest of the method.

http://codereview.chromium.org/6547001/diff/1/chrome/renderer/render_widget.c...
chrome/renderer/render_widget.cc:504: if (animation_update_pending_) {
nit: This would be much easier to read if you used an early return consistently,
and avoided indenting the rest of the code in this method. 

For example, you could replace line 508 with:
if (!animation_update_pending)
  return;

http://codereview.chromium.org/6547001/diff/1/chrome/renderer/render_widget.c...
chrome/renderer/render_widget.cc:507: animation_floor_time_ = now +
base::TimeDelta::FromMilliseconds(16);
FYI: It is interesting that this will "drift" later and later, when we arrive
here.  I would have expected something more like:
animation_floor_time_ = animation_floor_time_ + 
  base::TimeDelta::FromMilliseconds(16).

http://codereview.chromium.org/6547001/diff/1/chrome/renderer/render_widget.c...
chrome/renderer/render_widget.cc:515: webwidget_->animate();
nit: Here again, a return statement would mean that we wouldn't need an else
clause on line 516

http://codereview.chromium.org/6547001/diff/1/chrome/renderer/render_widget.c...
chrome/renderer/render_widget.cc:516: } else if (!animation_task_posted_) {
nit: Again, I'd find it more readable to see:
if (animation_task_posted)
  return;

so that the following code is not indented.

http://codereview.chromium.org/6547001/diff/1/chrome/renderer/render_widget.c...
chrome/renderer/render_widget.cc:525: int64 delay = (animation_floor_time_ -
now).InMillisecondsRoundedUp();
Comment: It is interesting that delay could be as small as 1 ms.  Considering
that the resolution of the time is rarely that fine, it is not clear that
setting such a small delay is helpful (though I know you're worried about early
firing, rather than late firing).

[jamesr, 2011-02-18 20:42:05]

On 2011/02/18 07:31:29, jar wrote:
> I'm convinced that the histograms are what we talked about... so LGTM.
> 
> I had a few nits below that would have made reading the code a lot easier for
> me, and you might consider the suggested changes.
> 

Thanks!  WebKit work must be softening my brain :)

[jamesr, 2011-02-18 20:43:24]

>
http://codereview.chromium.org/6547001/diff/1/chrome/renderer/render_widget.c...
> chrome/renderer/render_widget.cc:525: int64 delay = (animation_floor_time_ -
> now).InMillisecondsRoundedUp();
> Comment: It is interesting that delay could be as small as 1 ms.  Considering
> that the resolution of the time is rarely that fine, it is not clear that
> setting such a small delay is helpful (though I know you're worried about
early
> firing, rather than late firing).

This is a good point, but I'm not yet sure if having any particular delay other
than 1ms makes more or less sense.  My hope is that seeing this distribution in
the wild will make it clearer what the proper behavior is.