Blink changes to record interest rects for http://w3c.github.io/frame-timing/

Blink changes to record interest rects for http://w3c.github.io/frame-timing/

This patch ads the .idl interface and classes for Frame Timing.
It also adds the basic functionality for Frame timing: Taking a given Frame,
calculate the Rect on its enclosing Layer, and asks CC to notify Blink when
that Rect is rendered or composited.
It also provides the API that CC can call to record when a render or composite
event happens.

* There was a method/property that existed when I created this patch (FrameID in Frame.h/cpp) but has since been modified or removed. I don't see another way of identifying frames.

* requires https://codereview.chromium.org/1095483004/

BUG=441552
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195375

[MikeB, 2015-02-06 17:39:22]

mpb@chromium.org changed reviewers:
+ vmpstr@chromium.org

[MikeB, 2015-02-06 17:41:31]

https://codereview.chromium.org/908453003/diff/1/Source/core/frame/Frame.h
File Source/core/frame/Frame.h (right):

https://codereview.chromium.org/908453003/diff/1/Source/core/frame/Frame.h#ne...
Source/core/frame/Frame.h:102: int64_t frameID() const { return m_frameID; }
This was removed in https://codereview.chromium.org/296893009

[MikeB, 2015-02-06 17:49:35]

mpb@chromium.org changed reviewers:
+ chrishtr@chromium.org, enne@chromium.org, rbyers@chromium.org

[MikeB, 2015-02-06 17:51:24]

Hi Rick, Enne & Chris - this is a more official version of the code you reviewed
several months back in 
https://codereview.chromium.org/390193003/

It looks like there may have been a couple of subtle refactorings that happened
since then, so please let me know if I need to revisit any of this!

Thanks,
-Mike

[chrishtr, 2015-02-06 21:51:26]

I'm about to go on vacation and don't have time before that to review. I've
added a couple of more reviewers who can give feedback.

[enne (OOO), 2015-02-10 00:17:50]

Can any of this be unit tested?

https://codereview.chromium.org/908453003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/908453003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderView.cpp:336: LayoutRect
RenderView::rectForPaintInvalidation(const LayoutRect* paintInvalidationRectPtr)
const
I see your comment about this existing before and then being removed, but I
think you should investigate where it went.  I would worry about this function
becoming invisibly stale over time as paint code changes, so it'd be better if
you could reuse real functionality elsewhere rather than having this one-off.

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

https://codereview.chromium.org/908453003/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1914: static void findFrameTimingRequestRects(Page*
page)
Sorry for always bikeshedding, but find in a function name makes me think it
returns something.  Maybe "update" or "push"?

https://codereview.chromium.org/908453003/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1916: typedef WTF::HashMap<const GraphicsLayer*,
It's really too bad that Blink hasn't moved into the Chromium repo, because this
seems like a lot of data structure contortion to fit a "set all the rects" API,
when a "add one more request" would be more natural but have more overhead to go
through the bindings.  (That said, if there aren't many requests, maybe that'd
be cleaner anyway.)

https://codereview.chromium.org/908453003/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1918: GraphicsLayerFrameTimingRects;
Rects => Requests?

https://codereview.chromium.org/908453003/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1925: if (!frame->isLocalFrame())
Can you leave a comment why this is? How do requests on non-local frames get
handled?

https://codereview.chromium.org/908453003/diff/1/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/908453003/diff/1/public/web/WebWidget.h#newco...
public/web/WebWidget.h:163: enum frameTimingEventType {
style nit: The "f" should be capitalized here, right?

https://codereview.chromium.org/908453003/diff/1/public/web/WebWidget.h#newco...
public/web/WebWidget.h:169: virtual void
recordFrameTimingEvent(frameTimingEventType eventType, int64_t RectId, const
WebVector<std::pair<int, double>>& events) { }
I assume there's some future patch to hook something up to call this?

[MikeB, 2015-03-18 21:18:34]

Looking into what's unit-testable now. Thanks!

https://codereview.chromium.org/908453003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderView.cpp (right):

https://codereview.chromium.org/908453003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderView.cpp:336: LayoutRect
RenderView::rectForPaintInvalidation(const LayoutRect* paintInvalidationRectPtr)
const
I updated this to be more in-line with using properties of LayoutView and not a
dedicated member function, please take a look at WebViewImpl.cpp for details.

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

https://codereview.chromium.org/908453003/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1914: static void findFrameTimingRequestRects(Page*
page)
On 2015/02/10 00:17:49, enne wrote:
> Sorry for always bikeshedding, but find in a function name makes me think it
> returns something.  Maybe "update" or "push"?

Done.

https://codereview.chromium.org/908453003/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1916: typedef WTF::HashMap<const GraphicsLayer*,
I'm happy to take a stab at it, but not 100% clear on what you mean by a 
"add one more request" API. If you give me a little more info on what you have
in mind, I can see if it's feasible. Thanks!

https://codereview.chromium.org/908453003/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1918: GraphicsLayerFrameTimingRects;
On 2015/02/10 00:17:49, enne wrote:
> Rects => Requests?

Done.

https://codereview.chromium.org/908453003/diff/1/Source/web/WebViewImpl.cpp#n...
Source/web/WebViewImpl.cpp:1925: if (!frame->isLocalFrame())
If I don't have a local frame, then I don't have a GraphicsLayer, so calling the
APi doesn't make much sense. Or am I mis-interpreting what isLocalFrame means?
I'm getting GraphicsLayer from either Document or HTMLFrameOwnerElement which
both come from LocalFrame.

https://codereview.chromium.org/908453003/diff/1/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/908453003/diff/1/public/web/WebWidget.h#newco...
public/web/WebWidget.h:163: enum frameTimingEventType {
On 2015/02/10 00:17:49, enne wrote:
> style nit: The "f" should be capitalized here, right?

Done.

https://codereview.chromium.org/908453003/diff/1/public/web/WebWidget.h#newco...
public/web/WebWidget.h:169: virtual void
recordFrameTimingEvent(frameTimingEventType eventType, int64_t RectId, const
WebVector<std::pair<int, double>>& events) { }
On 2015/02/10 00:17:49, enne wrote:
> I assume there's some future patch to hook something up to call this?

https://codereview.chromium.org/914403003/

*note: there will be another follow-up patch to get the Composite events in
addition to the Render events.

[chrishtr, 2015-03-27 01:13:03]

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

https://codereview.chromium.org/908453003/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:62: static int64_t next =
static_cast<int64_t>(currentTime() * 1000000.0);
currentTime() is already in seconds, no?

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

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1969: if
(document->layoutView()->enclosingLayer()->compositingState()
Why do you need this complicated conditional instead of a simple
Layer::mapRectToPaintInvalidationBacking(document->layoutView(), ...)?

I remember we went back and forth on this in the other CL, but let's relitigate.
:)

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1987: GraphicsLayerFrameTimingRequests::iterator
glIter = glRequests.find(graphicsLayer);
s/glIter/requestIterator/

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1988: std::vector<std::pair<int64_t, WebRect>>
*glVector;
s/glVector/graphicsLayerTimingRect/

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4578: if (!mainFrameImpl() ||
!mainFrameImpl()->frameView())
In what situation do you need this conditional?

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4581: Frame* frame = m_page ? m_page->mainFrame() :
0;
Ditto.

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4588: return; // ASSERT? Other way to report error?
Is the frameID supposed to be guaranteed to exist?

Then use ASSERT_NOT_REACHED();

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4590: ASSERT(frame->isLocalFrame());
Why is this guaranteed?

https://codereview.chromium.org/908453003/diff/20001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/908453003/diff/20001/public/web/WebWidget.h#n...
public/web/WebWidget.h:164: enum FrameTimingEventType {
Add a comment linking to http://w3c.github.io/frame-timing/ for definition of
terms.

[MikeB, 2015-04-14 18:05:46]

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

https://codereview.chromium.org/908453003/diff/20001/Source/core/frame/Frame....
Source/core/frame/Frame.cpp:62: static int64_t next =
static_cast<int64_t>(currentTime() * 1000000.0);
On 2015/03/27 01:13:02, chrishtr wrote:
> currentTime() is already in seconds, no?

Yes, it's making "next" start off at a very large number, seeded by the time. It
will increment by "1" though, for each frame. If it used "seconds" then there's
a very real chance of collisions.

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

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1969: if
(document->layoutView()->enclosingLayer()->compositingState()
On 2015/03/27 01:13:02, chrishtr wrote:
> Why do you need this complicated conditional instead of a simple
> Layer::mapRectToPaintInvalidationBacking(document->layoutView(), ...)?
> 
> I remember we went back and forth on this in the other CL, but let's
relitigate.
> :)

I can't find us actually going back and forth last time.. 
https://codereview.chromium.org/390193003/diff/20001/Source/web/WebViewImpl.c...
Looks like you just said it looked more or less correct. 

This was replicating functionality in a different projection routine. Maybe
things have been cleaned up in the time since I started this project? Happy to
try the simpler API and if anything turns up in testing, we can start doing
things the more complicated way.

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1987: GraphicsLayerFrameTimingRequests::iterator
glIter = glRequests.find(graphicsLayer);
On 2015/03/27 01:13:02, chrishtr wrote:
> s/glIter/requestIterator/

Done.

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1988: std::vector<std::pair<int64_t, WebRect>>
*glVector;
On 2015/03/27 01:13:03, chrishtr wrote:
> s/glVector/graphicsLayerTimingRect/

Done.

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4578: if (!mainFrameImpl() ||
!mainFrameImpl()->frameView())
On 2015/03/27 01:13:03, chrishtr wrote:
> In what situation do you need this conditional?

Most of the routines in this file (at least, the ones I was using for
inspiration) made this check. Upon further review, they seem to actually USE
mainFrameImpl()->frameView() for an explicit call (most of the time) so I should
be ok to drop it.

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4581: Frame* frame = m_page ? m_page->mainFrame() :
0;
On 2015/03/27 01:13:03, chrishtr wrote:
> Ditto.

For this one, I don't feel comfortable dereferencing a pointer if I don't know
100% that it won't be null. So if m_page is null, I just set frame = 0.

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4588: return; // ASSERT? Other way to report error?
On 2015/03/27 01:13:03, chrishtr wrote:
> Is the frameID supposed to be guaranteed to exist?
> 
> Then use ASSERT_NOT_REACHED();

No, there is a possibility that the frame has since been removed from the DOM
and I should just return.

https://codereview.chromium.org/908453003/diff/20001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:4590: ASSERT(frame->isLocalFrame());
On 2015/03/27 01:13:03, chrishtr wrote:
> Why is this guaranteed?

No longer required. I was using localframe-only APIs to get the performance
timeline. Looks like that's been refactored since.

https://codereview.chromium.org/908453003/diff/20001/public/web/WebWidget.h
File public/web/WebWidget.h (right):

https://codereview.chromium.org/908453003/diff/20001/public/web/WebWidget.h#n...
public/web/WebWidget.h:164: enum FrameTimingEventType {
On 2015/03/27 01:13:03, chrishtr wrote:
> Add a comment linking to http://w3c.github.io/frame-timing/ for definition of
> terms.

Done.

[MikeB, 2015-04-16 19:43:10]

On 2015/02/10 00:17:50, enne wrote:
> Can any of this be unit tested?
> 
>

Unit tests added. PTAL
Thanks!
-Mike

[chrishtr, 2015-04-17 23:59:57]

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

https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1925: static void
pushFrameTimingRequestRectsToGraphicsLayer(Page* page)
Are you intending to collect here the sizes of all of the graphics layers? cc
already knows the sizes of them, so why do you need to do this? All I can see
being necessary is to send the frame id for the graphics layer, right?

[MikeB, 2015-04-19 22:34:48]

On 2015/04/17 23:59:57, chrishtr wrote:
>
https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.cpp
> File Source/web/WebViewImpl.cpp (right):
> 
>
https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
> Source/web/WebViewImpl.cpp:1925: static void
> pushFrameTimingRequestRectsToGraphicsLayer(Page* page)
> Are you intending to collect here the sizes of all of the graphics layers? cc
> already knows the sizes of them, so why do you need to do this? All I can see
> being necessary is to send the frame id for the graphics layer, right?

I'm intending to collect the size of all the frames, projected onto the graphics
layers. One graphics layer may have multiple frames that cover part of it. CC
doesn't seem to know or care the distinction.
Later in CC I need to intersect the visible area with any frames that may or may
not be visible on the graphics layers being drawn.

[chrishtr, 2015-04-20 23:51:18]

On 2015/04/19 at 22:34:48, mpb wrote:
> On 2015/04/17 23:59:57, chrishtr wrote:
> >
https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.cpp
> > File Source/web/WebViewImpl.cpp (right):
> > 
> >
https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
> > Source/web/WebViewImpl.cpp:1925: static void
> > pushFrameTimingRequestRectsToGraphicsLayer(Page* page)
> > Are you intending to collect here the sizes of all of the graphics layers?
cc
> > already knows the sizes of them, so why do you need to do this? All I can
see
> > being necessary is to send the frame id for the graphics layer, right?
> 
> I'm intending to collect the size of all the frames, projected onto the
graphics layers. One graphics layer may have multiple frames that cover part of
it. CC doesn't seem to know or care the distinction.
> Later in CC I need to intersect the visible area with any frames that may or
may not be visible on the graphics layers being drawn.

(Note: in the below, there are two kinds of "frame" referenced. Let's define
"HTML Frames" to mean means root document/iframe and "Animation Frame" to mean
the time spent performing "rendering & compositing" code before drawing new
things on the screen.)

I see. You're trying to figure out which HTML Frames were actually rastered. I
don't think this is such a great idea as implemented because it incurs overhead
and complexity, while only solving part of the problem as far as I can see (see
below).

The aim of the Frame Timing API is to measure the time spent executing an
Animation Frame. This could include some combination of Blink and cc code,
depending on the type of animation occurring (some examples are given in
http://w3c.github.io/frame-timing/#introduction). Did you have a plan for how to
implement all the cases correctly? Before we proceed with this I think all of
them need to be thought through. I'm happy to have a VC to discuss. Sorry if
this feels like we are going in circles, but I think this is a complicated topic
that needs a clean design.

[chrishtr, 2015-04-30 01:07:33]

https://codereview.chromium.org/908453003/diff/80001/Source/core/events/Event...
File Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/908453003/diff/80001/Source/core/events/Event...
Source/core/events/EventTypeNames.in:234: webkitframetimingbufferfull
We don't do prefixed web features any more, this should be
frametimingbufferfull.

https://codereview.chromium.org/908453003/diff/80001/Source/core/timing/Perfo...
File Source/core/timing/Performance.h (right):

https://codereview.chromium.org/908453003/diff/80001/Source/core/timing/Perfo...
Source/core/timing/Performance.h:85: void webkitClearFrameTimings();
Remove webkit prefixes.

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

https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1927: typedef WTF::HashMap<const GraphicsLayer*,
Move this function to FrameView, which will avoid the for loop over Frame.

https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1931: GraphicsLayerFrameTimingRequests glRequests;
s/glRequests/graphicsLayerTimingRequests/

https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1940: LocalFrame* localframe = toLocalFrame(frame);
s/localframe/localFrame/

https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1948:
DeprecatedPaintLayer::mapRectToPaintInvalidationBacking(
I think you should be able to simply write:

DeprecatedPaintLayer::mapRectToPaintInvalidationBacking(localFrame->contentRenderer(),
 localFrame>enclosingLayerForPaintInvalidationCrossingFrameBoundaries(),
localFrame->viewRect());

and

GraphicsLayer* graphicsLayer =
localFrame>enclosingLayerForPaintInvalidationCrossingFrameBoundaries()->graphicsLayerBacking()


and avoid lines 1947-1970

https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1983: for
(GraphicsLayerFrameTimingRequests::const_iterator iter = glRequests.begin();
iter != glRequests.end(); ++iter) {
Change WebLayer to have a method addFrameTimingRequest instead of
setFrameTimingRequests so that this code is compatible with moving to FrameView.

https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2002:
pushFrameTimingRequestRectsToGraphicsLayer(m_page.get());
I think this should go in FrameView::updateLayoutAndStyleForPainting instead.
ScrollingCoordinator fulfills a similar function to plumb touch event rects to
graphics layers.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

You should also use a dirty bit to avoid this work, similar again to what
ScrollingCoordinator does.

[MikeB, 2015-05-07 23:09:48]

I didn't switch for setFrameTimingRequests to addFrameTimingRequest yet, but the
other comments should all be addressed. PTAL. Thanks!

https://codereview.chromium.org/908453003/diff/80001/Source/core/events/Event...
File Source/core/events/EventTypeNames.in (right):

https://codereview.chromium.org/908453003/diff/80001/Source/core/events/Event...
Source/core/events/EventTypeNames.in:234: webkitframetimingbufferfull
On 2015/04/30 01:07:33, chrishtr wrote:
> We don't do prefixed web features any more, this should be
> frametimingbufferfull.

Done.

https://codereview.chromium.org/908453003/diff/80001/Source/core/timing/Perfo...
File Source/core/timing/Performance.h (right):

https://codereview.chromium.org/908453003/diff/80001/Source/core/timing/Perfo...
Source/core/timing/Performance.h:85: void webkitClearFrameTimings();
On 2015/04/30 01:07:33, chrishtr wrote:
> Remove webkit prefixes.

Done.

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

https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1927: typedef WTF::HashMap<const GraphicsLayer*,
On 2015/04/30 01:07:33, chrishtr wrote:
> Move this function to FrameView, which will avoid the for loop over Frame.

Done.

https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1931: GraphicsLayerFrameTimingRequests glRequests;
On 2015/04/30 01:07:33, chrishtr wrote:
> s/glRequests/graphicsLayerTimingRequests/

Done.

https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1940: LocalFrame* localframe = toLocalFrame(frame);
On 2015/04/30 01:07:33, chrishtr wrote:
> s/localframe/localFrame/

Done.

https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1948:
DeprecatedPaintLayer::mapRectToPaintInvalidationBacking(
On 2015/04/30 01:07:33, chrishtr wrote:
> I think you should be able to simply write:
> 
>
DeprecatedPaintLayer::mapRectToPaintInvalidationBacking(localFrame->contentRenderer(),
>  localFrame>enclosingLayerForPaintInvalidationCrossingFrameBoundaries(),
> localFrame->viewRect());
> 
> and
> 
> GraphicsLayer* graphicsLayer =
>
localFrame>enclosingLayerForPaintInvalidationCrossingFrameBoundaries()->graphicsLayerBacking()
> 
> 
> and avoid lines 1947-1970

enclosingLayerForPaintInvalidationCrossingFrameBoundaries is not a property on
blink::LocalFrame. It only lives on DeprecatedPaintLayer, so I have to keep the
->enclosingLayer()

LocalFrame also doesn't have a viewRect() property. Does this sound about right?

LayoutRect rect = localFrame->contentRenderer()->viewRect();

DeprecatedPaintLayer::mapRectToPaintInvalidationBacking(
   localFrame->contentRenderer(),
   localFrame->contentRenderer()->containerForPaintInvalidation(),
   rect);

const GraphicsLayer* graphicsLayer =
localFrame->contentRenderer()->enclosingLayer()->enclosingLayerForPaintInvalidationCrossingFrameBoundaries()->graphicsLayerBacking();

https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:1983: for
(GraphicsLayerFrameTimingRequests::const_iterator iter = glRequests.begin();
iter != glRequests.end(); ++iter) {
On 2015/04/30 01:07:33, chrishtr wrote:
> Change WebLayer to have a method addFrameTimingRequest instead of
> setFrameTimingRequests so that this code is compatible with moving to
FrameView.

Not yet done. Will see about making this change once code is moved to FrameView.

https://codereview.chromium.org/908453003/diff/80001/Source/web/WebViewImpl.c...
Source/web/WebViewImpl.cpp:2002:
pushFrameTimingRequestRectsToGraphicsLayer(m_page.get());
On 2015/04/30 01:07:33, chrishtr wrote:
> I think this should go in FrameView::updateLayoutAndStyleForPainting instead.
> ScrollingCoordinator fulfills a similar function to plumb touch event rects to
> graphics layers.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> You should also use a dirty bit to avoid this work, similar again to what
> ScrollingCoordinator does.

Done, except for the dirty bit.

[chrishtr, 2015-05-08 16:52:05]

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:2553: GraphicsLayerFrameTimingRequests
graphicsLayerTimingRequests;
Add a TODO(mpb) to add the dirty bit.

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:4022: GraphicsLayerFrameTimingRequests&
graphicsLayerTimingRequests)
Put this on the same line. Blink has no 80-column restriction..

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:4025: frame =
frame->tree().traverseNext(m_frame.get())) {
same here IMO. Consider reducing lines in the rest of this method also.

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:4038:
->enclosingLayerForPaintInvalidationCrossingFrameBoundaries();
containerForPaintInvalidation() is the same as this, you can reuse the
computation from line 4033. In fact this method should probably be removed,
I'll make a note for myself.

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:4046:
graphicsLayerTimingRequests.find(graphicsLayer);
I think you can just write:

graphicsLayerTimingRequests[graphicsLayer].push_back(std::make_pair(frame->frameID(),
enclosingIntRect(rect)));

the [] operator on a hash map makes a value if there was previously no entry
with the given key.

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:4058: if (frame != m_frame.get()) {
Factor into two methods, one which does the recursion and one which fills in
hash map values. There are other recursions just like that in this file, such as
invalidateTreeIfNeededRecursive that you can copy the pattern from.

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:237: void updateFrameTimingRequestsIfNeeded();
Make this private.

[MikeB, 2015-05-08 18:37:34]

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:2553: GraphicsLayerFrameTimingRequests
graphicsLayerTimingRequests;
On 2015/05/08 16:52:04, chrishtr wrote:
> Add a TODO(mpb) to add the dirty bit.

Done.

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:4022: GraphicsLayerFrameTimingRequests&
graphicsLayerTimingRequests)
On 2015/05/08 16:52:04, chrishtr wrote:
> Put this on the same line. Blink has no 80-column restriction..

Done.

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:4025: frame =
frame->tree().traverseNext(m_frame.get())) {
On 2015/05/08 16:52:04, chrishtr wrote:
> same here IMO. Consider reducing lines in the rest of this method also.

Done.

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:4038:
->enclosingLayerForPaintInvalidationCrossingFrameBoundaries();
On 2015/05/08 16:52:04, chrishtr wrote:
> containerForPaintInvalidation() is the same as this, you can reuse the
> computation from line 4033. In fact this method should probably be removed,
> I'll make a note for myself.

Done.

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:4046:
graphicsLayerTimingRequests.find(graphicsLayer);
On 2015/05/08 16:52:04, chrishtr wrote:
> I think you can just write:
> 
>
graphicsLayerTimingRequests[graphicsLayer].push_back(std::make_pair(frame->frameID(),
> enclosingIntRect(rect)));
> 
> the [] operator on a hash map makes a value if there was previously no entry
> with the given key.

error: no viable overloaded operator[] for type
'GraphicsLayerFrameTimingRequests' (aka 'HashMap<const blink::GraphicsLayer *,
std::vector<std::pair<int64_t, WebRect> > >')

I think that might be true of hash_map but not HashMap.

Although it looks like HashMap::add will do nothing if the key is already
present, so I think I can get away with the one-liner:

graphicsLayerTimingRequests.add(graphicsLayer,std::vector<std::pair<int64_t,
WebRect>>()).storedValue->value.push_back(std::make_pair(frame->frameID(),
enclosingIntRect(rect)));

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:4058: if (frame != m_frame.get()) {
On 2015/05/08 16:52:04, chrishtr wrote:
> Factor into two methods, one which does the recursion and one which fills in
> hash map values. There are other recursions just like that in this file, such
as
> invalidateTreeIfNeededRecursive that you can copy the pattern from.

Done.

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.h:237: void updateFrameTimingRequestsIfNeeded();
On 2015/05/08 16:52:05, chrishtr wrote:
> Make this private.

Done.

[chrishtr, 2015-05-09 00:46:42]

https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:4038: LayoutRect rect =
localFrame->contentLayoutObject()->viewRect();
s/rect/viewRect/

https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:4040: const DeprecatedPaintLayer* layer =
container->enclosingLayer();
Some more nits:

Remove the layer local variable, since it's only used once. Rename container to
paintInvalidationContainer for clarity.

[chrishtr, 2015-05-09 00:51:52]

https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.h:710: UnprefixedPerformanceClearFrameTimings =
776,
Why have Unprefixed at the start?

https://codereview.chromium.org/908453003/diff/120001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (left):

https://codereview.chromium.org/908453003/diff/120001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:1928: 
Spurious edit.

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

https://codereview.chromium.org/908453003/diff/120001/public/platform/WebLaye...
public/platform/WebLayer.h:202: virtual void setFrameTimingRequests(const
WebVector<std::pair<int64_t, WebRect>>&) = 0;
Add a comment referencing the feature spec, similar to what you put in
WebWidget.

[MikeB, 2015-05-11 23:10:52]

https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:4038: LayoutRect rect =
localFrame->contentLayoutObject()->viewRect();
On 2015/05/09 00:46:42, chrishtr slow May 11-15 wrote:
> s/rect/viewRect/

Done.

https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.h:710: UnprefixedPerformanceClearFrameTimings =
776,
On 2015/05/09 00:51:52, chrishtr slow May 11-15 wrote:
> Why have Unprefixed at the start?

Because I saw 
        UnprefixedPerformanceTimeline = 65,
        UnprefixedUserTiming = 67,
etc.
Happy to not have the 'Unprefixed' part if it's unnecessary.
I can just consolidate it as PerformanceFrameTiming and not have a separate one
for clear* and set*buffersize too.

https://codereview.chromium.org/908453003/diff/120001/Source/web/WebViewImpl.cpp
File Source/web/WebViewImpl.cpp (left):

https://codereview.chromium.org/908453003/diff/120001/Source/web/WebViewImpl....
Source/web/WebViewImpl.cpp:1928: 
On 2015/05/09 00:51:52, chrishtr slow May 11-15 wrote:
> Spurious edit.

Done.

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

https://codereview.chromium.org/908453003/diff/120001/public/platform/WebLaye...
public/platform/WebLayer.h:202: virtual void setFrameTimingRequests(const
WebVector<std::pair<int64_t, WebRect>>&) = 0;
On 2015/05/09 00:51:52, chrishtr slow May 11-15 wrote:
> Add a comment referencing the feature spec, similar to what you put in
> WebWidget.

Done.

[chrishtr, 2015-05-11 23:46:44]

https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.h:710: UnprefixedPerformanceClearFrameTimings =
776,
On 2015/05/11 23:10:51, MikeB wrote:
> On 2015/05/09 00:51:52, chrishtr slow May 11-15 wrote:
> > Why have Unprefixed at the start?
> 
> Because I saw 
>         UnprefixedPerformanceTimeline = 65,
>         UnprefixedUserTiming = 67,
> etc.
> Happy to not have the 'Unprefixed' part if it's unnecessary.
> I can just consolidate it as PerformanceFrameTiming and not have a separate
one
> for clear* and set*buffersize too.

Looks like this is because PrefixedPerformanceTimeline used to be enum value 66
(try a code search to see remnants).
You can just remove Unprefixed, since this complication doesn't apply to you.

I agree that combining them into a single UseCounter for PerformanceFrameTiming
is better, since we just want to count the usage of the overall feature.

[blink-reviews, 2015-05-12 02:49:35]

Ok, done! :)

Sent from my iPhone

> On May 11, 2015, at 4:46 PM, chrishtr@chromium.org wrote:
> 
> 
>
https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/UseCo...
> File Source/core/frame/UseCounter.h (right):
> 
>
https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/UseCo...
> Source/core/frame/UseCounter.h:710:
> UnprefixedPerformanceClearFrameTimings = 776,
>> On 2015/05/11 23:10:51, MikeB wrote:
>> On 2015/05/09 00:51:52, chrishtr slow May 11-15 wrote:
>> > Why have Unprefixed at the start?
> 
>> Because I saw
>>         UnprefixedPerformanceTimeline = 65,
>>         UnprefixedUserTiming = 67,
>> etc.
>> Happy to not have the 'Unprefixed' part if it's unnecessary.
>> I can just consolidate it as PerformanceFrameTiming and not have a
> separate one
>> for clear* and set*buffersize too.
> 
> Looks like this is because PrefixedPerformanceTimeline used to be enum
> value 66 (try a code search to see remnants).
> You can just remove Unprefixed, since this complication doesn't apply to
> you.
> 
> I agree that combining them into a single UseCounter for
> PerformanceFrameTiming is better, since we just want to count the usage
> of the overall feature.
> 
> https://codereview.chromium.org/908453003/

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

[chrishtr, 2015-05-12 03:17:22]

https://codereview.chromium.org/908453003/diff/140001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/908453003/diff/140001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.h:709: ObjectObserve = 775,
What's up with 772-775? A bad rebase?

[MikeB, 2015-05-12 17:00:09]

https://codereview.chromium.org/908453003/diff/140001/Source/core/frame/UseCo...
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/908453003/diff/140001/Source/core/frame/UseCo...
Source/core/frame/UseCounter.h:709: ObjectObserve = 775,
On 2015/05/12 03:17:22, chrishtr slow May 11-15 wrote:
> What's up with 772-775? A bad rebase?

Ah, I misunderstood the git diff conflict resolution. That was added but removed
in https://codereview.chromium.org/1135753002/

[chrishtr, 2015-05-12 22:19:54]

lgtm

[MikeB, 2015-05-12 22:21:03]

The CQ bit was checked by mpb@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-12 22:21:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908453003/160001

[commit-bot: I haz the power, 2015-05-12 22:33:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-12 22:33:13]

Dry run: Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[MikeB, 2015-05-12 23:15:05]

The CQ bit was checked by mpb@chromium.org to run a CQ dry run

[MikeB, 2015-05-12 23:15:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/908453003/#ps180001
(title: "Correct CQ compile errors.")

[commit-bot: I haz the power, 2015-05-12 23:15:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908453003/180001

[commit-bot: I haz the power, 2015-05-12 23:37:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-12 23:37:57]

Dry run: Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/55145)

[MikeB, 2015-05-12 23:42:03]

The CQ bit was checked by mpb@chromium.org to run a CQ dry run

[MikeB, 2015-05-12 23:42:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/908453003/#ps200001
(title: "std::tuple was not found.")

[commit-bot: I haz the power, 2015-05-12 23:42:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908453003/200001

[commit-bot: I haz the power, 2015-05-12 23:53:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-12 23:53:03]

Dry run: Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[MikeB, 2015-05-13 18:19:22]

The CQ bit was checked by mpb@chromium.org to run a CQ dry run

[MikeB, 2015-05-13 18:19:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/908453003/#ps220001
(title: "std::tuple is not available. Switching to a struct.")

[commit-bot: I haz the power, 2015-05-13 18:19:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908453003/220001

[commit-bot: I haz the power, 2015-05-13 18:23:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-13 18:23:51]

Dry run: Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[MikeB, 2015-05-13 18:48:39]

The CQ bit was checked by mpb@chromium.org to run a CQ dry run

[MikeB, 2015-05-13 18:48:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/908453003/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-13 18:49:00]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908453003/240001

[commit-bot: I haz the power, 2015-05-13 19:03:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-13 19:03:47]

Dry run: Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[MikeB, 2015-05-13 23:22:29]

The CQ bit was checked by mpb@chromium.org to run a CQ dry run

[MikeB, 2015-05-13 23:22:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/908453003/#ps260001
(title: "Stop using c++11 vector initialization.")

[commit-bot: I haz the power, 2015-05-13 23:22:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908453003/260001

[commit-bot: I haz the power, 2015-05-13 23:27:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-13 23:27:50]

Dry run: Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/55250)

[MikeB, 2015-05-13 23:35:08]

The CQ bit was checked by mpb@chromium.org to run a CQ dry run

[MikeB, 2015-05-13 23:35:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/908453003/#ps280001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-13 23:36:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908453003/280001

[MikeB, 2015-05-13 23:58:06]

On 2015/05/12 22:19:54, chrishtr slow May 11-15 wrote:
> lgtm

Please take one more quick look?
c++11 wasn't available on the CQ tests for mac, so I had to go and remove my use
of std::tuple and std::vector's initialization list.
I added a simple struct in public/platform/WebFrameTimingEvent.h.
Thanks!

[commit-bot: I haz the power, 2015-05-14 02:00:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-14 02:00:20]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[chrishtr, 2015-05-14 05:15:15]

lgtm

[MikeB, 2015-05-14 06:46:28]

The CQ bit was checked by mpb@chromium.org

[commit-bot: I haz the power, 2015-05-14 06:46:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908453003/280001

[commit-bot: I haz the power, 2015-05-14 09:07:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-14 09:07:09]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[MikeB, 2015-05-14 19:13:00]

The CQ bit was checked by mpb@chromium.org

[MikeB, 2015-05-14 19:13:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/908453003/#ps300001
(title: "Update LayoutTests for new properties")

[commit-bot: I haz the power, 2015-05-14 19:13:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/908453003/300001

[commit-bot: I haz the power, 2015-05-14 21:14:16]

Committed patchset #16 (id:300001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195375

[Charlie Reis, 2015-10-01 19:56:07]

On 2015/05/14 21:14:16, commit-bot: I haz the power wrote:
> Committed patchset #16 (id:300001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=195375

> * There was a method/property that existed when I created this patch (FrameID
in Frame.h/cpp) but has since been modified or removed. I don't see another way
of identifying frames.

Frame::frameID was removed intentionally in
https://codereview.chromium.org/296893009 because it's not compatible with
out-of-process iframes.

Please coordinate with dcheng@ on how to remove it in https://crbug.com/537864. 
Thanks!