[webkit-reviews] review denied: [Bug 88268] [chromium] Expose rendering statistics to WebLayerTreeView. : [Attachment 146644] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Dave Tu <dtu at chromium.org>'s
request for review:
Bug 88268: [chromium] Expose rendering statistics to WebLayerTreeView.
https://bugs.webkit.org/show_bug.cgi?id=88268

Attachment 146644: Patch
https://bugs.webkit.org/attachment.cgi?id=146644&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146644&action=review


> Source/Platform/chromium/public/WebLayerTreeView.h:178
> +    // Fills in a WebRenderingStats struct containing
> +    // information about the state of the compositor.

You don't need to line wrap so aggressively (there's no 80 col limit in
WebKit). This would fit easily on one line.

> Source/Platform/chromium/public/WebLayerTreeView.h:179
> +    WEBKIT_EXPORT void renderingStats(WebRenderingStats&);

This is a very expensive call relative to other calls on this interface since
it blocks on the compositor thread. I think you should move it into the
"Debugging / dangerous" section of this header and add some documentation here
about how heavy the call is.

> Source/WebKit/chromium/public/WebRenderingStats.h:1
> +/*

If you want to use this struct in Platform/chromium/public/WebLayerTreeView.h,
you need to define this struct in Platform/chromium/public/

> Source/WebKit/chromium/public/WebRenderingStats.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

it's 2012

> Source/WebKit/chromium/public/WebRenderingStats.h:31
> +struct WebRenderingStats {

please add a constructor that zeros out the fields.  Otherwise if someone
writes the very reasonable-looking code:

{
  WebRenderingStats stats;
  myWebView->renderingStats(stats);
}

and compositing mode isn't active, the struct will just have garbage stack
values sitting in it.

> Source/WebKit/chromium/public/WebRenderingStats.h:32
> +    int frameNumber; // Number of frames that have elapsed on the main
thread.

the names and documentation here are lacking. what does "elapsed" mean?  Is
this the number of frames we've rendered? what is this counted from - the
"beginning of time", the last call to gather stats, some other time? it looks
like this is only implemented in the compositing path - please document this at
least. is this a count, or a number?

> Source/WebKit/chromium/public/WebRenderingStats.h:33
> +    int implFrameNumber; // Number of frames that have elapsed on the impl
thread.

the "impl" bit here doesn't mean much out of context (and the comment doesn't
help). perhaps this could be named something like compositorThreadFrameNumber
(or frameCount)

what happens if threaded mode isn't enabled? please document the behavior
whatever it is

> Source/WebKit/chromium/public/WebWidget.h:227
> +    // Fills in a WebRenderingStats struct containing
> +    // information about the state of the compositor.
> +    virtual void renderingStats(WebRenderingStats&) { }

this doesn't make sense on WebWidget - we only support compositing on WebViews.


renderingStats() is a bit generic for what this is - since it's only for
compositing mode, something compositor-specific in the name would be better.

Looking at the implementation, it looks like this call is relatively heavy - it
blocks the main thread until the compositor thread can respond. Someone looking
at this header in isolation might think it's perfectly fine to do something
like call this every frame just in case. I think we should document how
expensive this call is or make it cheaper.