Bug 88268 – [chromium] Expose rendering statistics to WebLayerTreeView.

• Component: New Bugs
• Status: RESOLVED FIXED

[Dave Tu]

[chromium] Expose rendering statistics to WebLayerTreeView.

[Dave Tu]

Created attachment 145658 [details]
Patch

[Nat Duca]

Comment on attachment 145658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145658&action=review

> Source/Platform/chromium/public/WebLayerTreeView.h:50
> +    int implFrameCount;

any alternative names for paintFrameCount? painting is pretty overloaded. frameCount would be okay.

Put //s on each explaining what they are.

I know its annoying, but this has to be its own file.

> Source/Platform/chromium/public/WebLayerTreeView.h:181
> +    WEBKIT_EXPORT void renderingStatistics(WebRenderingStatistics&);

When we extend this to things like "time spent in paint" how is this API going to work? Are we just going to hope that the counters dont overflow? Or will we add a reset function? I dont care, but we should think it through now.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:200
> +    int implFrameNumber() const { return m_proxy->implFrameNumber(); }

I think this should have a renderingStatistics API too. The WLTVI changes should just be passthrough to CCLTH

> Source/WebCore/platform/graphics/chromium/cc/CCProxy.h:82
> +    virtual int implFrameNumber() = 0;

What happens if we want more impl-side rendering stats? E.g. time spent in draw? Would it be better to have a method on the proxy saying "fill your impl side stats into this WebRenderingStatistics struct?" That way when we add other stats we dont have to add yet-more proxy emthods

[Dave Tu]

Created attachment 146644 [details]
Patch

[Dave Tu]

Comment on attachment 145658 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145658&action=review

>> Source/Platform/chromium/public/WebLayerTreeView.h:50
>> +    int implFrameCount;
> 
> any alternative names for paintFrameCount? painting is pretty overloaded. frameCount would be okay.
> 
> Put //s on each explaining what they are.
> 
> I know its annoying, but this has to be its own file.

Done.

>> Source/Platform/chromium/public/WebLayerTreeView.h:181
>> +    WEBKIT_EXPORT void renderingStatistics(WebRenderingStatistics&);
> 
> When we extend this to things like "time spent in paint" how is this API going to work? Are we just going to hope that the counters dont overflow? Or will we add a reset function? I dont care, but we should think it through now.

For the frame counters, it's okay because it's (2^31-1)/120/60/60/24 = 207 days before it overflows at 120 fps. For "time spent in paint," probably floating point types. The Javascript API would convert them all to doubles anyway.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:200
>> +    int implFrameNumber() const { return m_proxy->implFrameNumber(); }
> 
> I think this should have a renderingStatistics API too. The WLTVI changes should just be passthrough to CCLTH

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCProxy.h:82
>> +    virtual int implFrameNumber() = 0;
> 
> What happens if we want more impl-side rendering stats? E.g. time spent in draw? Would it be better to have a method on the proxy saying "fill your impl side stats into this WebRenderingStatistics struct?" That way when we add other stats we dont have to add yet-more proxy emthods

Done.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[James Robinson]

Comment on attachment 146644 [details]
Patch

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.

[Nat Duca]

Comment on attachment 146644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146644&action=review

Gettin there! :)

>> Source/WebKit/chromium/public/WebWidget.h:227
>> +    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.

I think this makes sense on webwidget, actually. Fullscreen pepper plugins are webwidgets, remember? :/ And, we have teh composite() method on WebWidget.

obtainRenderingStats() with a comment saying it is slightly costly is sufficient for now, I think. THis is the place to explain that the call only works in accelerated mode and that calling it  in software mode will assert false.

> Source/WebKit/chromium/src/WebLayerTreeView.cpp:175
> +    stats.frameNumber = ccStats.frameNumber;

/me wonders out loud if we have an accepted technique for converting structs, e.g. by having the WebRenderingStats have conversion functions on it. Walk over to jamesr's desk and ask him if we have a way to do this cleaner please?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1455
> +{

You should ASSERT_NOT_REACHED in the case that this is called in non-accelerated mode.

[Dave Tu]

Created attachment 147196 [details]
Patch

[Dave Tu]

Comment on attachment 146644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146644&action=review

>> Source/Platform/chromium/public/WebLayerTreeView.h:178
>> +    // 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.

Done.

>> 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.

Done.

>> 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/

Done.

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

Done.

>> 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.

Done.

>> 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?

It's since the beginning of the CCLayerTreeHost. Is that the same as the "beginning of time?" The beginning of the GPU process?

What's the difference between "count" and "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

Isn't the "impl thread" just whatever thread compositing is happening on?

I'm not yet familiar with what terminology is specific and what is vague. If a "frame" is a delimiter between time intervals or an instantaneous event, then the "elapsed frames" would be the number of events that have occurred. In this case, the event for "frameNumber" is the number of commits that have happened (number of calls to CCLayerTreeHost::finishCommitOnImplThread) and the event for "implFrameNumber" is the number of composites that have happened (number of calls to CCLayerTreeHostImpl::drawLayers). Do these terms make sense in this context, or are they only applicable within the compositor code and should be invisible at this layer?

>>> Source/WebKit/chromium/public/WebWidget.h:227
>>> +    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.
> 
> I think this makes sense on webwidget, actually. Fullscreen pepper plugins are webwidgets, remember? :/ And, we have teh composite() method on WebWidget.
> 
> obtainRenderingStats() with a comment saying it is slightly costly is sufficient for now, I think. THis is the place to explain that the call only works in accelerated mode and that calling it  in software mode will assert false.

I intend to extend the method in the future with non-compositing stats. I'm not sure yet whether it will reuse the existing WebRenderingStats struct fields. Maybe a TODO is needed?

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1455
>> +{
> 
> You should ASSERT_NOT_REACHED in the case that this is called in non-accelerated mode.

Won't that just cause Chrome to crash? As written, this method just does nothing in non-accelerated mode. Since I'm adding a constructor to WebRenderingStats, the benchmarking extension can check the fields, and exclude them from Javascript if they haven't been set.

[James Robinson]

Comment on attachment 147196 [details]
Patch

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

[Nat Duca]

Comment on attachment 147196 [details]
Patch

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.

Lets make this be the number of animationFrames we've run. E.g. any time we call requestAnimationFrame, we call it a frame.

>> 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.

Good point. Lets name this as numFramesSentToScreen. This is exactly equal to the number of swapbuffers we issued.

>> 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?

Put it on the webview.

[Nat Duca]

> > You should ASSERT_NOT_REACHED in the case that this is called in non-accelerated mode.
> 
> Won't that just cause Chrome to crash? As written, this method just does nothing in non-accelerated mode. Since I'm adding a constructor to WebRenderingStats, the benchmarking extension can check the fields, and exclude them from Javascript if they haven't been set.

Hopefuly not. chrome wil be making this call, so you can have chrome not call down to webkit in software mode, and instead fill in the same structure with the software mode statistics. That is to say, make a renderview method with the same sort of api --- renderingStatistics() that itself says "if composited mode, ask webview for stats, else have the render_widget fill it in.

[Dave Tu]

Created attachment 147696 [details]
Patch

[Dave Tu]

Created attachment 147697 [details]
Patch

[Dave Tu]

Comment on attachment 147196 [details]
Patch

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.
> 
> Lets make this be the number of animationFrames we've run. E.g. any time we call requestAnimationFrame, we call it a frame.

Done.

>>> 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.
> 
> Good point. Lets name this as numFramesSentToScreen. This is exactly equal to the number of swapbuffers we issued.

Done.

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

Done.

>>> 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?
> 
> Put it on the webview.

Done.

>> 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

Done.

[Nat Duca]

Comment on attachment 147697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147697&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:326
> +    stats.numAnimationFrames = frameNumber();

How about renaming the vars here too, instead of having m_framenNumber, you have m_animationFrameNumber or something. And updating the impl side to have m_sourceAnimationFrameNumber. You should also update that when we call m_client->animate -- check both the single thread and ccthread proxy to understand when that happens. FrameNumber as measured now isn't 1:1 with calls to composite, which is where we currently bump frameNumber.

> Source/WebKit/chromium/public/WebView.h:364
> +    // Fills in a WebRenderingStats struct containing information about the state of the compositor.

There's a GPU section in the webview i think. Look for graphcisContext3D

> Source/WebKit/chromium/src/WebViewImpl.cpp:1461
> +    if (!m_layerTreeView.isNull())

You still need an assert_not_reached in software mode. This is the member variable m_isAcceleratedRenderignActive or something like that.

[James Robinson]

Comment on attachment 147697 [details]
Patch

Nat speaks wisdom so R- for now. I think with his comments addressed we'll be good to land, however.

[Dave Tu]

Created attachment 148464 [details]
Patch

[Dave Tu]

Comment on attachment 147697 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147697&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:326
>> +    stats.numAnimationFrames = frameNumber();
> 
> How about renaming the vars here too, instead of having m_framenNumber, you have m_animationFrameNumber or something. And updating the impl side to have m_sourceAnimationFrameNumber. You should also update that when we call m_client->animate -- check both the single thread and ccthread proxy to understand when that happens. FrameNumber as measured now isn't 1:1 with calls to composite, which is where we currently bump frameNumber.

Done.

>> Source/WebKit/chromium/public/WebView.h:364
>> +    // Fills in a WebRenderingStats struct containing information about the state of the compositor.
> 
> There's a GPU section in the webview i think. Look for graphcisContext3D

Done.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1461
>> +    if (!m_layerTreeView.isNull())
> 
> You still need an assert_not_reached in software mode. This is the member variable m_isAcceleratedRenderignActive or something like that.

Done.

[Dave Tu]

Created attachment 148477 [details]
Patch

[WebKit Review Bot]

Comment on attachment 148477 [details]
Patch

Attachment 148477 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12999079

[Nat Duca]

Comment on attachment 148477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148477&action=review

LGTM, but verify the tests. Did you sort out whether there's a clean way to output WebRenderingStats into JS value objects?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-262
> -    m_frameNumber++;

I bet this breaks a few cclayertreehost tests. build and run webkit_unit_tests to verify.  you can probably fix the tests.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:327
> +    m_proxy->renderingStats(stats);

bikeshed in me still questions this method name. Should the method name explain that its filling in only impl-side fields on the stats structure? E.g. implSideRenderingStats()?

[Dave Tu]

Created attachment 148706 [details]
Patch

[Dave Tu]

Comment on attachment 148477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148477&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:-262
>> -    m_frameNumber++;
> 
> I bet this breaks a few cclayertreehost tests. build and run webkit_unit_tests to verify.  you can probably fix the tests.

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:327
>> +    m_proxy->renderingStats(stats);
> 
> bikeshed in me still questions this method name. Should the method name explain that its filling in only impl-side fields on the stats structure? E.g. implSideRenderingStats()?

Done.

[Nat Duca]

Comment on attachment 148706 [details]
Patch

Please pull out the WebSerializedXX forwarding header change to a new patch and upload that separately. That needs review on its own.

This patch looks ready to go once you land that.

[Adam Barth]

Comment on attachment 148706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148706&action=review

> Source/Platform/chromium/public/WebRenderingStats.h:56
> +    WebSerializedScriptValue value()
> +    {
> +        v8::Handle<v8::Object> stats = v8::Object::New();
> +        if (numAnimationFrames)
> +            stats->Set(v8::String::New("numAnimationFrames"),
> +                              v8::Integer::New(numAnimationFrames),
> +                              v8::ReadOnly);
> +        if (numFramesSentToScreen)
> +            stats->Set(v8::String::New("numFramesSentToScreen"),
> +                              v8::Integer::New(numFramesSentToScreen),
> +                              v8::ReadOnly);
> +        return WebSerializedScriptValue::serialize(stats);
> +    }

I don't undertand why this function is here.  This doesn't look right.

[Adam Barth]

As discussed in Bug 89691, WebSerializedScriptValue shouldn't be moved into Platform.

[Dave Tu]

(In reply to comment #26)
> (From update of attachment 148706 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148706&action=review
> 
> > Source/Platform/chromium/public/WebRenderingStats.h:56
> > +    WebSerializedScriptValue value()
> > +    {
> > +        v8::Handle<v8::Object> stats = v8::Object::New();
> > +        if (numAnimationFrames)
> > +            stats->Set(v8::String::New("numAnimationFrames"),
> > +                              v8::Integer::New(numAnimationFrames),
> > +                              v8::ReadOnly);
> > +        if (numFramesSentToScreen)
> > +            stats->Set(v8::String::New("numFramesSentToScreen"),
> > +                              v8::Integer::New(numFramesSentToScreen),
> > +                              v8::ReadOnly);
> > +        return WebSerializedScriptValue::serialize(stats);
> > +    }
> 
> I don't undertand why this function is here.  This doesn't look right.

It's here because we plan on adding a bunch of additional fields to WebRenderingStats in the future, and moving the v8 object creation into WebKit allows us to do that without any changes on the Chromium side, as in https://chromiumcodereview.appspot.com/10536080/

[Adam Barth]

The WebKit API is not the correct place to implement JavaScript bindings.

[Nat Duca]

Hey Adam,

Ignoring the fact that the actual value() implementaiton is in the header rather than an impl file in src for a moment, can we get your advice on the following basic design problem?

We have a webkit-side data structure with stats in it. We want to export that data structure to javascript, but the interface out to JS is via a chrome-side extension, e.g. content/renderer/gpu/gpu_benchmarking_extension. That extension calls into webview to get the rendering stats structure, converts it to a v8::value and returns it out to javascript. The chromium-side code does not need to do anything with this data structure, just get it into v8.

The thing I'm trying to avoid is us having to make a change to chromium every time we change this webkit-side data. E.g. if I add a field, I want to avoid having to also change the v8 serialization code of this structure in chrome. Or, similarly, if I change a field, having to do a 3 way set of commits to make the change happen.

SerializedScriptValue is promising here because it allows us to exchange values between webkit and a chrome-side v8 extension without the chrome side having to do the WebType->v8 serialization.

Any alternative ideas on how to do this?

[Nat Duca]

Note, this is based on the WebIDBCursor::value design, which seems to do the same basic thing, but with the caveat that the chromium-side code is in src/webkit/glue and thus able to access WebKit/chromium/platform --- content/ doesn't seem to be able to see this directory. ~puzzled hat~

[Adam Barth]

> Ignoring the fact that the actual value() implementaiton is in the header rather than an impl file in src for a moment, can we get your advice on the following basic design problem?

Sorry.  Sorry for being brief earlier.  I was a bit rushed.

> We have a webkit-side data structure with stats in it. We want to export that data structure to javascript, but the interface out to JS is via a chrome-side extension, e.g. content/renderer/gpu/gpu_benchmarking_extension. That extension calls into webview to get the rendering stats structure, converts it to a v8::value and returns it out to javascript. The chromium-side code does not need to do anything with this data structure, just get it into v8.

Got it.

> The thing I'm trying to avoid is us having to make a change to chromium every time we change this webkit-side data. E.g. if I add a field, I want to avoid having to also change the v8 serialization code of this structure in chrome. Or, similarly, if I change a field, having to do a 3 way set of commits to make the change happen.
> 
> SerializedScriptValue is promising here because it allows us to exchange values between webkit and a chrome-side v8 extension without the chrome side having to do the WebType->v8 serialization.
> 
> Any alternative ideas on how to do this?

The fact that you're exposing this data structure to the GPU benchmarking extension in an implementation detail of Chromium that should be hidden from WebKit.  From WebKit's perspective, this data structure is just like many of other data structures we exchange with the embedder that aren't translated into JavaScript APIs in Chromium.

One trick we could try is to create a macro that iterates over all the properties of the data structure.  Chromium could then use this macro to generate JavaScript bindings.  We use this trick internally in WebCore a bunch, but I don't think we've ever used it in the API.  If that sounds appealing, we should check with the other API reviewers to make sure they're happy with it.

> Note, this is based on the WebIDBCursor::value design, which seems to do the same basic thing, but with the caveat that the chromium-side code is in src/webkit/glue and thus able to access WebKit/chromium/platform --- content/ doesn't seem to be able to see this directory. ~puzzled hat~

My understanding is that we're going to remove the use of WebSerializedScriptValue in WebIDBCursor::value in favor of just using WebData.  From Chromium's perspective, it's just an opaque sequence of bytes.  WebCore happens to interpret those bytes as a serialized script value, but Chromium doesn't care about that.

[Nat Duca]

So what I'm hearing is that the plan is to not have a way to a WebKit data type for a JS value. If thats the case, bummer, but I can work around it. I just wanted to use one if it did exist.

[Dave Tu]

Created attachment 149091 [details]
Patch

[James Robinson]

Comment on attachment 149091 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149091&action=review

R=me

> Source/WebKit/chromium/src/WebLayerTreeView.cpp:39
> +#include "public/WebRenderingStats.h"

nit: should be #include <public/WebRenderingStats.h>

> Source/WebKit/chromium/src/WebViewImpl.cpp:734
> +    if (!isAcceleratedCompositingActive())
> +        ASSERT_NOT_REACHED();

we'd normally spell this: ASSERT(isAcceleratedCompositingActive());

[Dave Tu]

Created attachment 149101 [details]
Patch

[WebKit Review Bot]

Comment on attachment 149101 [details]
Patch

Clearing flags on attachment: 149101

Committed r121064: <http://trac.webkit.org/changeset/121064>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

This broke the Chromium Mac build due to a class/struct mismatch:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac%20Builder/builds/26523

I'm rolling it out.

[Kenneth Russell]

Reverted r121064 for reason:

Broke Chromium Mac build.

Committed r121069: <http://trac.webkit.org/changeset/121069>

[Dave Tu]

Created attachment 149135 [details]
Patch

[James Robinson]

Comment on attachment 149135 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149135&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCProxy.h:37
> +struct CCRenderingStats;

please re-sort

[Dave Tu]

Created attachment 149140 [details]
Patch

[WebKit Review Bot]

Comment on attachment 149140 [details]
Patch

Rejecting attachment 149140 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
unk #3 succeeded at 960 (offset 34 lines).
Hunk #4 succeeded at 970 (offset 34 lines).
Hunk #5 FAILED at 1009.
Hunk #6 FAILED at 1040.
Hunk #7 succeeded at 2235 (offset -73 lines).
Hunk #8 succeeded at 2243 (offset -73 lines).
2 out of 8 hunks FAILED -- saving rejects to file Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'James Robi..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13032566

[Dave Tu]

Created attachment 149356 [details]
Patch

[Dave Tu]

Created attachment 149580 [details]
Patch

[Dave Tu]

Merged, need CQ.

[Dana Jansens]

(In reply to comment #47)
> Merged, need CQ.

If you stick the reviewer's name in the changelogs, then I can CQ it for you, otherwise youll have to get R+ again to pass the CQ.

[Dave Tu]

Created attachment 149595 [details]
Patch

[Dana Jansens]

Comment on attachment 149595 [details]
Patch

if you set R+ the bot's going to get upset.

just use --no-review --request-commit when you upload to not set R anything, and the CQ? flag. :)

[WebKit Review Bot]

Comment on attachment 149595 [details]
Patch

Clearing flags on attachment: 149595

Committed r121288: <http://trac.webkit.org/changeset/121288>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.