[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 #147196|review?                     |review-
               Flag|                            |




--- Comment #10 from James Robinson <jamesr at chromium.org>  2012-06-13 17:22:03 PST ---
(From update of attachment 147196)
View in context: https://bugs.webkit.org/attachment.cgi?id=147196&action=review

> Source/Platform/chromium/public/WebRenderingStats.h:32
> +    int frameNumber; // Number of commits that have occurred since the process started. Only applicable in accelerated mode.

"commits" is a concept that is internal the compositor and not something that I think makes a lot of sense to generally expose. The inspector uses this concept and refers to it simply as "frames".

Note that we're always going to make calls out to the embedder on each main-thread "frame" operation, since it needs it for rate control and synchronization with plugins etc etc.  These calls work in accelerated and non accelerated mode already (although they aren't entirely unified), so I'm wondering if we really need WebKit API to expose it or if the embedder should just keep track if they are interested.

> Source/Platform/chromium/public/WebRenderingStats.h:33
> +    int implFrameNumber; // Number of composites that have occurred since the process started. In single-threaded mode, this is the same as frameNumber. Only applicable in accelerated mode.

The comment is nice but it makes me think that the variable name is poor, though - could we make this more obvious?  Other instrumentation code (see WebWidgetClient) talks about compositor frames in terms of drawing and swapping as two operation.  Perhaps implFrameNumber -> framesComposited, compositorFramesSwapped, something like that?

The note about single-threaded mode is confusing (the caller of this interface shouldn't care whether the compositor is in single-threaded mode or not) and I think it's also incorrect - I believe in single threaded mode we sometimes start making a frame, incrementing frameNumber, and then later abort since we have no damage.  I would just delete that part and instead try to make the variables names clear enough so that a user could understand why the two numbers might be different.

> Source/WebKit/chromium/WebKit.gyp:322
> +                'public/platform/WebRenderingStats.h',

don't need this

> Source/WebKit/chromium/public/WebWidget.h:228
> +    // TODO(dtu): Do something useful in software mode.

in WebKit, this is a FIXME: with no name not a TODO()

I would personally leave the comment out

If this is only going to make in composited mode, then it really should be on WebView - we only support compositing on WebViews. If we plan to write other implementations based on things other than the compositor then leaving it up on WebWidget is reasonable - is that what you had in mind, Nat?

> Source/WebKit/chromium/public/platform/WebRenderingStats.h:26
> +#include "../../../../Platform/chromium/public/WebRenderingStats.h"

I don't think we need to have this header at all - normally we only have forwarding headers like this if we have code that's already using the header in the WebKit/chromium/public/platform/ location, but in this case if somebody wants to use this header they can get it from the Platform location

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