[Webkit-unassigned] [Bug 88268] [chromium] Expose rendering statistics to WebLayerTreeView.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=88268


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #146644|review?                     |review-
               Flag|                            |




--- Comment #6 from James Robinson <jamesr at chromium.org>  2012-06-08 18:15:10 PST ---
(From update of attachment 146644)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.