[not for review] Add Draw entries to window Performance Timeline

[not for review] Add Draw entries to window Performance Timeline

[not for review]
Spec: http://www.w3.org/TR/performance-timeline/
"Entries" in the performance timeline can be extended by browser vendors. This CL is to create a type to use for recording timing events when a frame is drawn to the screen. This will be used to let app developers calculate their effective frames per second and time-to-frame events.

BUG=120796
R=nduca@google.com

** For related cc changes, see https://codereview.chromium.org/397443002


Sub-cl 1: https://codereview.chromium.org/392743004

[chrishtr, 2014-07-16 20:00:25]

https://codereview.chromium.org/390193003/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/390193003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:463: LayoutRect repaintRect =
repaintRectPtr ? *repaintRectPtr : viewRect();
You need to put line 463 at the top of the function, and return it unchanged in
the printing() and other fallbacks.

https://codereview.chromium.org/390193003/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderView.cpp:479: return LayoutRect();
return repaintRect;

https://codereview.chromium.org/390193003/diff/20001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (right):

https://codereview.chromium.org/390193003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1781: while (frame) {
Random nit, but I would make this a for loop.

https://codereview.chromium.org/390193003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1800:
RenderLayer::mapRectToPaintBackingCoordinates(paintInvalidationContainer, rect);
This looks right.

[enne (OOO), 2014-07-22 20:45:27]

https://codereview.chromium.org/390193003/diff/80001/Source/core/frame/Frame.cpp
File Source/core/frame/Frame.cpp (right):

https://codereview.chromium.org/390193003/diff/80001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:61: int64_t generateFrameID()
style nit: FrameID => FrameId

Does Blink have some equivalent of base::StaticAtomicSequenceNumber?

https://codereview.chromium.org/390193003/diff/80001/public/platform/WebLayer.h
File public/platform/WebLayer.h (right):

https://codereview.chromium.org/390193003/diff/80001/public/platform/WebLayer...
public/platform/WebLayer.h:190: virtual WebVector<std::pair<int64_t, WebRect> >
drawFrameRequestRects() const = 0;
Does this ever get used? It's ok to just have a setter...

[Rick Byers, 2014-08-02 00:56:36]

I love how simple this has become.  Compared to the initial prototypes, almost
all of the logic is now centralized (eg. I like
enclosingLayerForPaintInvalidationCrossingFrameBoundaries,
RenderLayer::mapRectToPaintInvalidationBacking and rectForPaintInvalidation). 
It looks like there's some good opportunities to simplify the touch hit rect
code now (although it has to deal with a number of things you don't, and also
different performance issues that might not be satisfied by the abstractions
here).

The one piece of general purpose logic that's still up at the WebViewImpl layer
(which seems too high for me) is the general notion of given a RenderObject, get
it's bounding box in the space of it's GraphicsLayer (and the GraphicsLayer
itself).  It might be worth putting that logic somewhere more common.  Although
I guess your code works only for RenderView, not arbitrary RenderObjects (and if
code was doing this with a large number of RenderObjects, it would probably want
to avoid recomputing transforms).  So maybe it's not sufficiently general
purpose to bother moving.

If a frame is inside of some non-composited scrolling area (eg. an
overflow:scroll div on a low-dpi device) I believe you're rects will be
incorrect between the time the scroll offset changes and something else triggers
layout.
 
Also, have you tested to see how your code deals with cases like frames inside
of nodes with arbitrary CSS 3d transforms applied to them?  Since you're sharing
logic with repaint I assume the computations would be correct, but may still be
worth testing.  Presumably you're going to get bounding box rects in this case
(since the frame may be an arbitrary quadrilateral in the co-ordinate space of
it's backing graphics layer).  Will that cause any issues downstream?  Eg. two
non-overlapping frames may have overlapping rects.

[abarth-chromium, 2014-08-02 17:22:59]

I think we should discuss whether we want to implement this feature on
blink-dev.  Last time I talked with Nat about it, I expressed a number of
concerns that (to my knowledge) have not been addressed.

[nduca, 2014-08-02 17:39:41]

yes adam, i have not gotten to your concerns yet. i am well aware you have them.
we are first figuring out the technical possibliities. then we will approach
webperf [before blink dev]. then blink dev.

[chrishtr, 2014-08-04 16:04:33]

On Fri, Aug 1, 2014 at 5:56 PM, <rbyers@chromium.org> wrote:

> I love how simple this has become.  Compared to the initial prototypes,
> almost
> all of the logic is now centralized (eg. I like
> enclosingLayerForPaintInvalidationCrossingFrameBoundaries,
> RenderLayer::mapRectToPaintInvalidationBacking and
> rectForPaintInvalidation).
> It looks like there's some good opportunities to simplify the touch hit
> rect
> code now (although it has to deal with a number of things you don't, and
> also
> different performance issues that might not be satisfied by the
> abstractions
> here).
>
> The one piece of general purpose logic that's still up at the WebViewImpl
> layer
> (which seems too high for me) is the general notion of given a
> RenderObject, get
> it's bounding box in the space of it's GraphicsLayer (and the GraphicsLayer
> itself).  It might be worth putting that logic somewhere more common.
>  Although
> I guess your code works only for RenderView, not arbitrary RenderObjects
> (and if
> code was doing this with a large number of RenderObjects, it would
> probably want
> to avoid recomputing transforms).  So maybe it's not sufficiently general
> purpose to bother moving.
>


FYI I am doing precisely this refactoring this week. The same information
is needed for optimizing paint, and as a side-effect it should be usable
for this use case and clean up the logic to be independent of frame
boundaries.


>
> If a frame is inside of some non-composited scrolling area (eg. an
> overflow:scroll div on a low-dpi device) I believe you're rects will be
> incorrect between the time the scroll offset changes and something else
> triggers
> layout.
>
> Also, have you tested to see how your code deals with cases like frames
> inside
> of nodes with arbitrary CSS 3d transforms applied to them?  Since you're
> sharing
> logic with repaint I assume the computations would be correct, but may
> still be
> worth testing.  Presumably you're going to get bounding box rects in this
> case
> (since the frame may be an arbitrary quadrilateral in the co-ordinate
> space of
> it's backing graphics layer).  Will that cause any issues downstream?  Eg.
> two
> non-overlapping frames may have overlapping rects.
>
>
> https://codereview.chromium.org/390193003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.