Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(66)

Issue 908453003: Blink changes to record interest rects for http://w3c.github.io/frame-timing/ (Closed)

Created:
5 years, 10 months ago by MikeB
Modified:
5 years, 2 months ago
CC:
arv+blink, blink-reviews, blink-reviews-rendering, Inactive, dglazkov+blink, Dominik Röttsches, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, pdr., Xianzhu, zoltan1, Charlie Reis, dcheng
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 15

Patch Set 2 : Code review comments (and re-sync) #

Total comments: 18

Patch Set 3 : Code Review changes #

Patch Set 4 : Unit tests for WebViewImpl.cpp #

Patch Set 5 : Add Experimental Blink Feature flag for this change #

Total comments: 17

Patch Set 6 : Move Frame Timing rect collection from WebViewImpl to FrameView. #

Total comments: 14

Patch Set 7 : Code review comments #

Total comments: 10

Patch Set 8 : Code review comments, plus update Render events to get finishTime as well. #

Total comments: 2

Patch Set 9 : Fix bad rebase #

Patch Set 10 : Correct CQ compile errors. #

Patch Set 11 : std::tuple was not found. #

Patch Set 12 : std::tuple is not available. Switching to a struct. #

Patch Set 13 : rebase #

Patch Set 14 : Stop using c++11 vector initialization. #

Patch Set 15 : rebase #

Patch Set 16 : Update LayoutTests for new properties #

Unified diffs Side-by-side diffs Delta from patch set Stats (+657 lines, -85 lines) Patch
M LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-navigated-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed-and-gced-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-navigated-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M LayoutTests/fast/dom/Window/window-properties-performance-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +3 lines, -0 lines 0 comments Download
M LayoutTests/webexposed/global-interface-listing-expected.txt View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +10 lines, -0 lines 0 comments Download
M Source/core/core.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +6 lines, -0 lines 0 comments Download
M Source/core/events/EventTypeNames.in View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/frame/Frame.h View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M Source/core/frame/Frame.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +13 lines, -0 lines 0 comments Download
M Source/core/frame/FrameView.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +6 lines, -0 lines 0 comments Download
M Source/core/frame/FrameView.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +45 lines, -0 lines 0 comments Download
M Source/core/frame/UseCounter.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/timing/Performance.h View 1 2 3 4 5 6 7 2 chunks +14 lines, -0 lines 0 comments Download
M Source/core/timing/Performance.cpp View 1 2 3 4 5 6 7 8 chunks +68 lines, -3 lines 0 comments Download
M Source/core/timing/Performance.idl View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
A + Source/core/timing/PerformanceCompositeTiming.h View 1 2 3 4 5 2 chunks +18 lines, -17 lines 0 comments Download
A + Source/core/timing/PerformanceCompositeTiming.cpp View 1 2 3 4 5 2 chunks +27 lines, -7 lines 0 comments Download
A + Source/core/timing/PerformanceCompositeTiming.idl View 1 2 3 4 1 chunk +5 lines, -17 lines 0 comments Download
M Source/core/timing/PerformanceEntry.h View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
A + Source/core/timing/PerformanceRenderTiming.h View 1 2 3 4 5 6 7 2 chunks +18 lines, -17 lines 0 comments Download
A + Source/core/timing/PerformanceRenderTiming.cpp View 1 2 3 4 5 6 7 2 chunks +27 lines, -7 lines 0 comments Download
A + Source/core/timing/PerformanceRenderTiming.idl View 1 2 3 4 1 chunk +5 lines, -17 lines 0 comments Download
M Source/platform/RuntimeEnabledFeatures.in View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M Source/web/WebViewImpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download
M Source/web/WebViewImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +25 lines, -0 lines 0 comments Download
M Source/web/tests/WebViewTest.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +242 lines, -0 lines 0 comments Download
A Source/web/tests/data/frame_timing_1.html View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
A Source/web/tests/data/frame_timing_2.html View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
A Source/web/tests/data/frame_timing_3.html View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download
A Source/web/tests/data/frame_timing_b.html View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
A Source/web/tests/data/frame_timing_inner.html View 1 2 3 1 chunk +16 lines, -0 lines 0 comments Download
A public/platform/WebFrameTimingEvent.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +42 lines, -0 lines 0 comments Download
M public/platform/WebLayer.h View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
M public/web/WebWidget.h View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +10 lines, -0 lines 0 comments Download

Messages

Total messages: 71 (26 generated)
MikeB
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#newcode102 Source/core/frame/Frame.h:102: int64_t frameID() const { return m_frameID; } This was ...
5 years, 10 months ago (2015-02-06 17:41:31 UTC) #2
MikeB
Hi Rick, Enne & Chris - this is a more official version of the code ...
5 years, 10 months ago (2015-02-06 17:51:24 UTC) #4
chrishtr
I'm about to go on vacation and don't have time before that to review. I've ...
5 years, 10 months ago (2015-02-06 21:51:26 UTC) #5
enne (OOO)
Can any of this be unit tested? https://codereview.chromium.org/908453003/diff/1/Source/core/rendering/RenderView.cpp File Source/core/rendering/RenderView.cpp (right): https://codereview.chromium.org/908453003/diff/1/Source/core/rendering/RenderView.cpp#newcode336 Source/core/rendering/RenderView.cpp:336: LayoutRect RenderView::rectForPaintInvalidation(const ...
5 years, 10 months ago (2015-02-10 00:17:50 UTC) #6
MikeB
Looking into what's unit-testable now. Thanks! https://codereview.chromium.org/908453003/diff/1/Source/core/rendering/RenderView.cpp File Source/core/rendering/RenderView.cpp (right): https://codereview.chromium.org/908453003/diff/1/Source/core/rendering/RenderView.cpp#newcode336 Source/core/rendering/RenderView.cpp:336: LayoutRect RenderView::rectForPaintInvalidation(const LayoutRect* ...
5 years, 9 months ago (2015-03-18 21:18:34 UTC) #7
chrishtr
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.cpp#newcode62 Source/core/frame/Frame.cpp:62: static int64_t next = static_cast<int64_t>(currentTime() * 1000000.0); currentTime() is ...
5 years, 9 months ago (2015-03-27 01:13:03 UTC) #8
MikeB
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.cpp#newcode62 Source/core/frame/Frame.cpp:62: static int64_t next = static_cast<int64_t>(currentTime() * 1000000.0); On 2015/03/27 ...
5 years, 8 months ago (2015-04-14 18:05:46 UTC) #9
MikeB
On 2015/02/10 00:17:50, enne wrote: > Can any of this be unit tested? > > ...
5 years, 8 months ago (2015-04-16 19:43:10 UTC) #10
chrishtr
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.cpp#newcode1925 Source/web/WebViewImpl.cpp:1925: static void pushFrameTimingRequestRectsToGraphicsLayer(Page* page) Are you intending to collect ...
5 years, 8 months ago (2015-04-17 23:59:57 UTC) #11
MikeB
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.cpp#newcode1925 > ...
5 years, 8 months ago (2015-04-19 22:34:48 UTC) #12
chrishtr
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 ...
5 years, 8 months ago (2015-04-20 23:51:18 UTC) #13
chrishtr
https://codereview.chromium.org/908453003/diff/80001/Source/core/events/EventTypeNames.in File Source/core/events/EventTypeNames.in (right): https://codereview.chromium.org/908453003/diff/80001/Source/core/events/EventTypeNames.in#newcode234 Source/core/events/EventTypeNames.in:234: webkitframetimingbufferfull We don't do prefixed web features any more, ...
5 years, 7 months ago (2015-04-30 01:07:33 UTC) #14
MikeB
I didn't switch for setFrameTimingRequests to addFrameTimingRequest yet, but the other comments should all be ...
5 years, 7 months ago (2015-05-07 23:09:48 UTC) #15
chrishtr
https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/FrameView.cpp#newcode2553 Source/core/frame/FrameView.cpp:2553: GraphicsLayerFrameTimingRequests graphicsLayerTimingRequests; Add a TODO(mpb) to add the dirty ...
5 years, 7 months ago (2015-05-08 16:52:05 UTC) #16
MikeB
https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/908453003/diff/100001/Source/core/frame/FrameView.cpp#newcode2553 Source/core/frame/FrameView.cpp:2553: GraphicsLayerFrameTimingRequests graphicsLayerTimingRequests; On 2015/05/08 16:52:04, chrishtr wrote: > Add ...
5 years, 7 months ago (2015-05-08 18:37:34 UTC) #17
chrishtr
https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/FrameView.cpp#newcode4038 Source/core/frame/FrameView.cpp:4038: LayoutRect rect = localFrame->contentLayoutObject()->viewRect(); s/rect/viewRect/ https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/FrameView.cpp#newcode4040 Source/core/frame/FrameView.cpp:4040: const DeprecatedPaintLayer* ...
5 years, 7 months ago (2015-05-09 00:46:42 UTC) #18
chrishtr
https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/UseCounter.h File Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/UseCounter.h#newcode710 Source/core/frame/UseCounter.h:710: UnprefixedPerformanceClearFrameTimings = 776, Why have Unprefixed at the start? ...
5 years, 7 months ago (2015-05-09 00:51:52 UTC) #19
MikeB
https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/FrameView.cpp File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/FrameView.cpp#newcode4038 Source/core/frame/FrameView.cpp:4038: LayoutRect rect = localFrame->contentLayoutObject()->viewRect(); On 2015/05/09 00:46:42, chrishtr slow ...
5 years, 7 months ago (2015-05-11 23:10:52 UTC) #20
chrishtr
https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/UseCounter.h File Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/908453003/diff/120001/Source/core/frame/UseCounter.h#newcode710 Source/core/frame/UseCounter.h:710: UnprefixedPerformanceClearFrameTimings = 776, On 2015/05/11 23:10:51, MikeB wrote: > ...
5 years, 7 months ago (2015-05-11 23:46:44 UTC) #21
blink-reviews
Ok, done! :) Sent from my iPhone > On May 11, 2015, at 4:46 PM, ...
5 years, 7 months ago (2015-05-12 02:49:35 UTC) #22
chrishtr
https://codereview.chromium.org/908453003/diff/140001/Source/core/frame/UseCounter.h File Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/908453003/diff/140001/Source/core/frame/UseCounter.h#newcode709 Source/core/frame/UseCounter.h:709: ObjectObserve = 775, What's up with 772-775? A bad ...
5 years, 7 months ago (2015-05-12 03:17:22 UTC) #23
MikeB
https://codereview.chromium.org/908453003/diff/140001/Source/core/frame/UseCounter.h File Source/core/frame/UseCounter.h (right): https://codereview.chromium.org/908453003/diff/140001/Source/core/frame/UseCounter.h#newcode709 Source/core/frame/UseCounter.h:709: ObjectObserve = 775, On 2015/05/12 03:17:22, chrishtr slow May ...
5 years, 7 months ago (2015-05-12 17:00:09 UTC) #24
MikeB
5 years, 7 months ago (2015-05-12 17:00:11 UTC) #25
chrishtr
lgtm
5 years, 7 months ago (2015-05-12 22:19:54 UTC) #26
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/908453003/160001
5 years, 7 months ago (2015-05-12 22:21:33 UTC) #28
commit-bot: I haz the power
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/builds/44502)
5 years, 7 months ago (2015-05-12 22:33:13 UTC) #30
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/908453003/180001
5 years, 7 months ago (2015-05-12 23:15:14 UTC) #33
commit-bot: I haz the power
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)
5 years, 7 months ago (2015-05-12 23:37:57 UTC) #35
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/908453003/200001
5 years, 7 months ago (2015-05-12 23:42:15 UTC) #38
commit-bot: I haz the power
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/builds/44512)
5 years, 7 months ago (2015-05-12 23:53:03 UTC) #40
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/908453003/220001
5 years, 7 months ago (2015-05-13 18:19:39 UTC) #43
commit-bot: I haz the power
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/builds/44571)
5 years, 7 months ago (2015-05-13 18:23:51 UTC) #45
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/908453003/240001
5 years, 7 months ago (2015-05-13 18:49:00 UTC) #48
commit-bot: I haz the power
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/builds/44574)
5 years, 7 months ago (2015-05-13 19:03:47 UTC) #50
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/908453003/260001
5 years, 7 months ago (2015-05-13 23:22:57 UTC) #53
commit-bot: I haz the power
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/builds/44591) mac_blink_rel on ...
5 years, 7 months ago (2015-05-13 23:27:50 UTC) #55
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/908453003/280001
5 years, 7 months ago (2015-05-13 23:36:01 UTC) #58
MikeB
On 2015/05/12 22:19:54, chrishtr slow May 11-15 wrote: > lgtm Please take one more quick ...
5 years, 7 months ago (2015-05-13 23:58:06 UTC) #59
commit-bot: I haz the power
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/61638)
5 years, 7 months ago (2015-05-14 02:00:20 UTC) #61
chrishtr
lgtm
5 years, 7 months ago (2015-05-14 05:15:15 UTC) #62
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/908453003/280001
5 years, 7 months ago (2015-05-14 06:46:43 UTC) #64
commit-bot: I haz the power
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/61669)
5 years, 7 months ago (2015-05-14 09:07:09 UTC) #66
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/908453003/300001
5 years, 7 months ago (2015-05-14 19:13:19 UTC) #69
commit-bot: I haz the power
Committed patchset #16 (id:300001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195375
5 years, 7 months ago (2015-05-14 21:14:16 UTC) #70
Charlie Reis
5 years, 2 months ago (2015-10-01 19:56:07 UTC) #71
Message was sent while issue was closed.
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!

Powered by Google App Engine
This is Rietveld 408576698