Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 11312069: Plumbing for rendering stats subscriber. (Closed)

Created:
6 years, 11 months ago by hartmanng
Modified:
6 years, 11 months ago
CC:
chromium-reviews, jam, apatrick_chromium, joi+watch-content_chromium.org, Aaron Boodman, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, cc-bugs_chromium.org, Ian Vollick
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : ifdef for 3-sided patch #

Total comments: 4

Patch Set 3 : #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+52 lines, -2 lines) Patch
M content/renderer/gpu/gpu_benchmarking_extension.cc View 4 chunks +39 lines, -2 lines 1 comment Download
M content/renderer/render_widget.h View 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/render_widget.cc View 1 chunk +7 lines, -0 lines 0 comments Download
M webkit/compositor_bindings/web_layer_tree_view_impl.h View 1 2 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
hartmanng
Please take a look. Specifically, enne: Please review web_layer_tree_view_impl.h jamesr: Please review render_widget.h and render_widget.cc ...
6 years, 11 months ago (2012-11-02 20:16:27 UTC) #1
hartmanng
Sorry for the spam, I realize I accidentally left "NOT READY FOR REVIEW" in my ...
6 years, 11 months ago (2012-11-02 21:05:42 UTC) #2
danakj
https://codereview.chromium.org/11312069/diff/2001/webkit/compositor_bindings/web_layer_tree_view_impl.h File webkit/compositor_bindings/web_layer_tree_view_impl.h (right): https://codereview.chromium.org/11312069/diff/2001/webkit/compositor_bindings/web_layer_tree_view_impl.h#newcode53 webkit/compositor_bindings/web_layer_tree_view_impl.h:53: virtual void startRecordingRenderingStats() const OVERRIDE { } If you ...
6 years, 11 months ago (2012-11-02 21:15:18 UTC) #3
hartmanng
https://codereview.chromium.org/11312069/diff/2001/webkit/compositor_bindings/web_layer_tree_view_impl.h File webkit/compositor_bindings/web_layer_tree_view_impl.h (right): https://codereview.chromium.org/11312069/diff/2001/webkit/compositor_bindings/web_layer_tree_view_impl.h#newcode53 webkit/compositor_bindings/web_layer_tree_view_impl.h:53: virtual void startRecordingRenderingStats() const OVERRIDE { } On 2012/11/02 ...
6 years, 11 months ago (2012-11-05 16:09:30 UTC) #4
danakj
lgtm, but i'm not owners in any of these places and @jamesr should review the ...
6 years, 11 months ago (2012-11-05 16:19:37 UTC) #5
jamesr
The stats are all additive, so can't you achieve the exact same thing by calling ...
6 years, 11 months ago (2012-11-05 18:12:21 UTC) #6
apatrick_chromium
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_benchmarking_extension.cc File content/renderer/gpu/gpu_benchmarking_extension.cc ...
6 years, 11 months ago (2012-11-05 20:33:17 UTC) #7
hartmanng
6 years, 11 months ago (2012-11-06 15:09:25 UTC) #8
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.

Powered by Google App Engine
This is Rietveld 408576698