[webkit-reviews] review denied: [Bug 88268] [chromium] Expose rendering statistics to WebLayerTreeView. : [Attachment 147196] 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 147196: Patch
https://bugs.webkit.org/attachment.cgi?id=147196&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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