Fixing RenderingStats vs WebRenderingStats mess

Fixing RenderingStats vs WebRenderingStats mess

We'd like to not have to plumb stats through the WebKit public API, as briefly mentioned here https://bugs.webkit.org/show_bug.cgi?id=101629 .

This patch should creates a WebRenderingStatsImpl containing a RenderingStats, and downcasts it to send it through WebKit.

BUG=None

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

[Ian Vollick, 2012-11-13 21:26:06]

I think it looks great. I only have a few nits, but Nat should probably sign off
on the approach.

https://codereview.chromium.org/11364221/diff/7001/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11364221/diff/7001/content/renderer/render_wi...
content/renderer/render_widget.cc:17: #include "cc/rendering_stats.h"
Already included in the header.

https://codereview.chromium.org/11364221/diff/7001/content/renderer/render_wi...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/11364221/diff/7001/content/renderer/render_wi...
content/renderer/render_widget.h:47: }
Don't think you need to forward declare this if you're including the header.

[hartmanng, 2012-11-13 21:42:53]

This should get around WebRenderingStats so we don't need to keep modifying the
WebKit public API.

Please take a look.

https://codereview.chromium.org/11364221/diff/7001/content/renderer/render_wi...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/11364221/diff/7001/content/renderer/render_wi...
content/renderer/render_widget.cc:17: #include "cc/rendering_stats.h"
On 2012/11/13 21:26:06, vollick wrote:
> Already included in the header.

Done.

https://codereview.chromium.org/11364221/diff/7001/content/renderer/render_wi...
File content/renderer/render_widget.h (right):

https://codereview.chromium.org/11364221/diff/7001/content/renderer/render_wi...
content/renderer/render_widget.h:47: }
On 2012/11/13 21:26:06, vollick wrote:
> Don't think you need to forward declare this if you're including the header.

Done.

[jamesr, 2012-11-13 21:44:53]

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

https://codereview.chromium.org/11364221/diff/9002/webkit/compositor_bindings...
webkit/compositor_bindings/web_layer_tree_view_impl.h:54: virtual void
renderingStats(void*) const OVERRIDE;
How is this better than downcasting?

[jamesr, 2012-11-13 22:03:56]

The real way to fix this is to have the gpu benchmark extension code have a
pointer to the cc:: type and not go through WebKit.  Until that happens, I think
passing a WebRenderingStats pointer or reference and downcasting is fine.  Going
to void* doesn't help anything.

[hartmanng, 2012-11-14 16:06:28]

If I understood our IM conversation last night, I believe this is the approach
we landed on.

PTAL

[nduca, 2012-11-14 20:51:39]

@jamesr for review. if he's happy, go ahead. Im too slammed to review.

[jamesr, 2012-11-14 21:43:02]

Can you send this to the 'win' trybot to make sure it compiles in that component
build?  I think you may be missing some exports.

I think http://code.google.com/p/chromium/issues/detail?id=156175 is the real
fix to this, but I've been busy on code yellow work so I haven't had a chance to
make much progress. If you want to take over that task (and make many things
like this much easier) let me know.

[jamesr, 2012-11-14 21:43:57]

Assuming you can get this to compile + link everywhere, the approach seems fine.

[hartmanng, 2012-11-15 14:06:23]

Having trouble running trybots, they all seem to fail immediately with "Failure
-1" and no link to results or output. Vollick somehow managed to run a win_rel
trybot for me (
http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/80329 ),
but I'll talk to maruel to try to sort out the rest of these bots.

[hartmanng, 2012-11-15 21:02:07]

On 2012/11/15 14:06:23, hartmanng wrote:
> Having trouble running trybots, they all seem to fail immediately with
"Failure
> -1" and no link to results or output. Vollick somehow managed to run a win_rel
> trybot for me (
> http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/80329
),
> but I'll talk to maruel to try to sort out the rest of these bots.

Turns out I couldn't run trybots myself since I'm not quite an official
committer yet. Vollick helped run some for me, 'win' failed browser tests, but I
doubt that has to do with this patch, probably just flake. Both win and win_rel
compiled successfully.

PTAL

[Ian Vollick, 2012-11-21 14:22:03]

On 2012/11/15 21:02:07, hartmanng wrote:
> On 2012/11/15 14:06:23, hartmanng wrote:
> > Having trouble running trybots, they all seem to fail immediately with
> "Failure
> > -1" and no link to results or output. Vollick somehow managed to run a
win_rel
> > trybot for me (
> > http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/80329
> ),
> > but I'll talk to maruel to try to sort out the rest of these bots.
> 
> Turns out I couldn't run trybots myself since I'm not quite an official
> committer yet. Vollick helped run some for me, 'win' failed browser tests, but
I
> doubt that has to do with this patch, probably just flake. Both win and
win_rel
> compiled successfully.
> 
> PTAL
lgtm, but you'll need OWNERs.

[nduca, 2012-11-23 22:35:57]

how're we doing on this, glenn?

[hartmanng, 2012-11-23 22:38:02]

On 2012/11/23 22:35:57, nduca wrote:
> how're we doing on this, glenn?

blocked on thanksgiving I think, but I'm not aware of any issues with the patch.

[jamesr, 2012-11-26 20:00:42]

lgtm

[commit-bot: I haz the power, 2012-11-26 20:01:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/11364221/6006

[commit-bot: I haz the power, 2012-11-26 22:37:45]

Change committed as 169513