Plumbing for rendering stats subscriber.

Plumbing for rendering stats subscriber.

vollick is working on a subscriber-based approach to replace our current getRenderingStats() (see http://codereview.chromium.org/11198005). This is the chromium side of the plumbing required to be able to call his methods from our benchmarking javascript harness.

BUG=155136

[hartmanng, 2012-11-02 20:16:27]

Please take a look.

Specifically,

enne: Please review web_layer_tree_view_impl.h
jamesr: Please review render_widget.h and render_widget.cc
apatrick: Please review gpu_benchmarking_extension.cc

Thanks!

[hartmanng, 2012-11-02 21:05:42]

Sorry for the spam, I realize I accidentally left "NOT READY FOR REVIEW" in my
description previously. Just to be clear, this is ready for review.

[danakj, 2012-11-02 21:15:18]

https://codereview.chromium.org/11312069/diff/2001/webkit/compositor_bindings...
File webkit/compositor_bindings/web_layer_tree_view_impl.h (right):

https://codereview.chromium.org/11312069/diff/2001/webkit/compositor_bindings...
webkit/compositor_bindings/web_layer_tree_view_impl.h:53: virtual void
startRecordingRenderingStats() const OVERRIDE { }
If you land the webkit side first, and don't make them pure virtual, then you
can probably get away without any ifdef magic?

https://codereview.chromium.org/11312069/diff/2001/webkit/compositor_bindings...
webkit/compositor_bindings/web_layer_tree_view_impl.h:55: #endif /* ifdef
RENDERING_STATS_SUBSCRIBER */
nit: // RENDERING_STATS_SUBSCRIBER

(c++ style)

[hartmanng, 2012-11-05 16:09:30]

https://codereview.chromium.org/11312069/diff/2001/webkit/compositor_bindings...
File webkit/compositor_bindings/web_layer_tree_view_impl.h (right):

https://codereview.chromium.org/11312069/diff/2001/webkit/compositor_bindings...
webkit/compositor_bindings/web_layer_tree_view_impl.h:53: virtual void
startRecordingRenderingStats() const OVERRIDE { }
On 2012/11/02 21:15:19, danakj wrote:
> If you land the webkit side first, and don't make them pure virtual, then you
> can probably get away without any ifdef magic?

Unless there's a strong consensus that this is the better way to do it, I'd
prefer to leave it as-is for now. I feel like this method "should" be pure
virtual, if nothing else it's consistent with the existing renderingStats()
method.

Plus I will have a 3rd patch regardless to plumb in the final javascript parts
(which can't land until after http://codereview.chromium.org/11198005 lands).

https://codereview.chromium.org/11312069/diff/2001/webkit/compositor_bindings...
webkit/compositor_bindings/web_layer_tree_view_impl.h:55: #endif /* ifdef
RENDERING_STATS_SUBSCRIBER */
On 2012/11/02 21:15:19, danakj wrote:
> nit: // RENDERING_STATS_SUBSCRIBER
> 
> (c++ style)

Done.

[danakj, 2012-11-05 16:19:37]

lgtm, but i'm not owners in any of these places and @jamesr should review the
api changes.

[jamesr, 2012-11-05 18:12:21]

The stats are all additive, so can't you achieve the exact same thing by calling
getRenderingStats() twice and subtracting the two?  The additional entry point
doesn't add any new functionality and seems generally less useful since you
can't get stats since startup with it.

[apatrick_chromium, 2012-11-05 20:33:17]

I defer to jamesr. gpu_benchmarking_extension.cc LGTM for style except for one
nit.

https://codereview.chromium.org/11312069/diff/8001/content/renderer/gpu/gpu_b...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/11312069/diff/8001/content/renderer/gpu/gpu_b...
content/renderer/gpu/gpu_benchmarking_extension.cc:172: !args[0]->IsBoolean())
nit: this condition expression would fit on one line.

[hartmanng, 2012-11-06 15:09:25]

On 2012/11/05 18:12:21, jamesr wrote:
> The stats are all additive, so can't you achieve the exact same thing by
calling
> getRenderingStats() twice and subtracting the two?

The idea with this new approach is that it could be hooked up directly to
scrollGestures to make the start/end times more accurate, since currently our
start/end calls start in javascript and go through at least 1 thread boundary,
adding some lag that could mess up the results. (Sorry, I obviously should have
explained that better)

However, your comments have sparked a bit of discussion between me, vollick, and
nduca, and we're reconsidering how best to plumb this through (and if we can
make use of the existing getRenderingStats plumbing). Closing the issue as it
looks like this approach is maybe not preferred anymore.

Thanks to all the reviewers for your time and comments.