Bug 140474

Summary: Add a way to collect scrolling performance data (viewport tile coverage) with UI-side compositing
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: New BugsAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: ossy, simon.fraser, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch thorton: review+

Description Simon Fraser (smfr) 2015-01-14 15:52:46 PST
Add a way to collect scrolling performance data (viewport tile coverage) with UI-side compositing
Comment 1 Simon Fraser (smfr) 2015-01-14 17:51:35 PST
Created attachment 244670 [details]
Patch
Comment 2 Csaba Osztrogon√°c 2015-01-15 04:03:55 PST
Comment on attachment 244670 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:58
> +#include "RemoteLayerTreeScrollingPerformanceData.h"

Please move this include to the "#if PLATFORM(COCOA)" block 
_before_ landing not to break the EFL/GTK builds. Thanks.
Comment 3 Simon Fraser (smfr) 2015-01-16 11:13:59 PST
Created attachment 244774 [details]
Patch
Comment 4 Simon Fraser (smfr) 2015-01-16 11:14:48 PST
Created attachment 244775 [details]
Patch
Comment 5 Tim Horton 2015-01-16 11:36:06 PST
Comment on attachment 244775 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/TileController.cpp:45
> +    return "TileGrid Container Layer";

While this is not perf-critical code, I know there's a better way to make a String from a literal (but I don't know what it is).

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:849
> +- (WebCore::FloatRect)visibleRectInScreenCoordinates

screen? I see no screen here.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:930
> +        scrollPerfData->didCommitLayerTree([self visibleRectInScreenCoordinates]);

dot notation?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1391
> +        scrollPerfData->didScroll([self visibleRectInScreenCoordinates]);

dot notation?

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2128
> +    return nil;
> +#else
> +    return nil;
> +#endif

This can be simplified.

> Source/WebKit2/UIProcess/Cocoa/RemoteLayerTreeScrollingPerformanceData.h:45
> +    NSArray* data(); // Array of [ time, event type, unfilled pixel count ]

star's on the wrong side

> Source/WebKit2/UIProcess/Cocoa/RemoteLayerTreeScrollingPerformanceData.h:50
> +    RemoteLayerTreeDrawingAreaProxy& m_drawingArea;

This interspersion of type declarations and function declarations and members is weird. Can this be sorted?

> Source/WebKit2/UIProcess/Cocoa/RemoteLayerTreeScrollingPerformanceData.mm:78
> +    NSMutableArray* dataArray = [NSMutableArray arrayWithCapacity:m_blankPixelCounts.size()];

star's on the wrong side!

> Source/WebKit2/UIProcess/Cocoa/RemoteLayerTreeScrollingPerformanceData.mm:90
> +static CALayer *findTileGridContainerLayer(CALayer *layer)

Should we keep its id instead of using names and crawling the tree?
Comment 6 Simon Fraser (smfr) 2015-01-20 10:41:02 PST
https://trac.webkit.org/r178723