New issue
Advanced search Search tips

Issue 869924 link

Starred by 5 users
Status: Started
Owner:
maxlg@chromium.org
Cc:
tdres...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

[FCP++] Implement FCP++ metrics

Project Member Reported by maxlg@google.com, Aug 1 
First Contentful Paint was introduced to measure page loading speed, but it often fires before users see anything on the page.

We need a better metric more correlated to user experience of page speed. Therefore, we propose the following metrics:

1. Largest Text Paint
2. Largest Image Paint
3. Last Text Paint
4. Last Image Paint
Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Aug 17 

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/11e54546640000
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 13 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c

commit 33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Thu Sep 13 18:59:41 2018

[FCP++] Implement Largest Text Paint and Last Text Paint

This CL implements Largest Text Paint and Last Text Paint.

Largest Text Paint timing measures how long to paint the largest text
element within viewport from navigation start, intending to proxy
main visible content delivery speed.

Last Text Paint timing measures how long to paint the last text within
viewport from navigation start, intending to proxy all visible
contents' delivery speed.

Bug: 869924
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I8b36fc1ec2e75998e00f6140446b326047c72140
Reviewed-on: https://chromium-review.googlesource.com/1158853
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591100}
[modify] https://crrev.com/33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c/third_party/blink/renderer/core/frame/local_frame_view.h
[modify] https://crrev.com/33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c/third_party/blink/renderer/core/layout/layout_object.cc
[modify] https://crrev.com/33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c/third_party/blink/renderer/core/paint/BUILD.gn
[add] https://crrev.com/33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c/third_party/blink/renderer/core/paint/paint_tracker.cc
[add] https://crrev.com/33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c/third_party/blink/renderer/core/paint/paint_tracker.h
[modify] https://crrev.com/33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc
[add] https://crrev.com/33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
[add] https://crrev.com/33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c/third_party/blink/renderer/core/paint/text_paint_timing_detector.h
[add] https://crrev.com/33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c/third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc
[modify] https://crrev.com/33a7b8f5e6947a8c2ecf13f7e353ee0bad0a5e3c/third_party/blink/renderer/platform/runtime_enabled_features.json5
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 18 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c6e6875823fb306e08baf8af3deb246b3c88ee9e

commit c6e6875823fb306e08baf8af3deb246b3c88ee9e
Author: Kent Tamura <tkent@chromium.org>
Date: Tue Sep 18 08:48:14 2018

Reduce the size of local_frame_view.h

local_frame_view.h is used in 1,500+ compilation units, and
crrev.com/591100 increased its pre-processed size by 3.57MB.  This CL
fixes it.

Bug: 242216, 869924
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I3dd48467d027f56655bb796984756ee7c25f86cd
Reviewed-on: https://chromium-review.googlesource.com/1229873
Reviewed-by: Daniel Bratell <bratell@opera.com>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591976}
[modify] https://crrev.com/c6e6875823fb306e08baf8af3deb246b3c88ee9e/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/c6e6875823fb306e08baf8af3deb246b3c88ee9e/third_party/blink/renderer/core/frame/local_frame_view.h
[modify] https://crrev.com/c6e6875823fb306e08baf8af3deb246b3c88ee9e/third_party/blink/renderer/core/layout/layout_object.cc
[modify] https://crrev.com/c6e6875823fb306e08baf8af3deb246b3c88ee9e/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 18 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e62b9095fe750f3dad440f98c5775c4f6ac4f165

commit e62b9095fe750f3dad440f98c5775c4f6ac4f165
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Tue Sep 18 19:04:35 2018

[FCP++] Largest/Last Text Paint nits

1. Register Swap promise only when there is no ongoing swap promise.
2. Add tailing underscore to private attributes.
3. Restructure the while loop in FindLargestPaintCandidate and
FindLastPaintCandidate.

Bug: 869924
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iaf19947eb3ef098114f2ab2ca7a4d6cdcec42758
Reviewed-on: https://chromium-review.googlesource.com/1229413
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592130}
[modify] https://crrev.com/e62b9095fe750f3dad440f98c5775c4f6ac4f165/third_party/blink/renderer/core/paint/paint_tracker.cc
[modify] https://crrev.com/e62b9095fe750f3dad440f98c5775c4f6ac4f165/third_party/blink/renderer/core/paint/paint_tracker.h
[modify] https://crrev.com/e62b9095fe750f3dad440f98c5775c4f6ac4f165/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
[modify] https://crrev.com/e62b9095fe750f3dad440f98c5775c4f6ac4f165/third_party/blink/renderer/core/paint/text_paint_timing_detector.h
[modify] https://crrev.com/e62b9095fe750f3dad440f98c5775c4f6ac4f165/third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 6 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b8d194606ea18af8a3d3c3a9dec528e29a109997

commit b8d194606ea18af8a3d3c3a9dec528e29a109997
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Sat Oct 06 01:51:18 2018

[FCP++] Implement Largest Image Paint and Last Image Paint

Largest Image Paint and Last Image Paint are implemented as an effort
to finda better alternative to First Contentful Paint.

Largest Image Paint measures the time when the largest image is first
painted after it's fully loaded.

Last Image Paint measures the time when the last image is first painted
after it's fully loaded.

luci.chromium.try:linux_layout_tests_slimming_paint_v2;
master.tryserver.blink:linux_trusty_blink_rel

Bug: 869924
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Id35b4c257c434a457cf0f9c7447e2f4326f68a68
Reviewed-on: https://chromium-review.googlesource.com/c/1191818
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597394}
[modify] https://crrev.com/b8d194606ea18af8a3d3c3a9dec528e29a109997/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/b8d194606ea18af8a3d3c3a9dec528e29a109997/third_party/blink/renderer/core/paint/BUILD.gn
[add] https://crrev.com/b8d194606ea18af8a3d3c3a9dec528e29a109997/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[add] https://crrev.com/b8d194606ea18af8a3d3c3a9dec528e29a109997/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
[add] https://crrev.com/b8d194606ea18af8a3d3c3a9dec528e29a109997/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc
[modify] https://crrev.com/b8d194606ea18af8a3d3c3a9dec528e29a109997/third_party/blink/renderer/core/paint/paint_tracker.cc
[modify] https://crrev.com/b8d194606ea18af8a3d3c3a9dec528e29a109997/third_party/blink/renderer/core/paint/paint_tracker.h
[modify] https://crrev.com/b8d194606ea18af8a3d3c3a9dec528e29a109997/third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 8 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/35856a4a555dd19abe8765a7bca5a7470075f70b

commit 35856a4a555dd19abe8765a7bca5a7470075f70b
Author: Daniel Bratell <bratell@opera.com>
Date: Mon Oct 08 10:17:09 2018

Renamed some duplicate symbols in Blink paint timing code

image_paint_timing_detector.cc and text_paint_timing_detector.cc
are similar and had identically named helper functions. This
broke some jumbo builds. This patch renames the helper functions
to have unique names.

Bug: 869924
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I0a42ef4f647ea0b5bfb80bf9dc363a0650424efd
Reviewed-on: https://chromium-review.googlesource.com/c/1268135
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Commit-Queue: Daniel Bratell <bratell@opera.com>
Cr-Commit-Position: refs/heads/master@{#597517}
[modify] https://crrev.com/35856a4a555dd19abe8765a7bca5a7470075f70b/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/35856a4a555dd19abe8765a7bca5a7470075f70b/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 16 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8d628983e3a3dcd5174f1793ea5345febec04d89

commit 8d628983e3a3dcd5174f1793ea5345febec04d89
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Tue Oct 16 14:40:49 2018

[FCP++] Allow WebPerformance to report Largest/LastImagePaint

In order to report Largest/LastImagePaint to UKM, this CL is to plumb
Largest/LastImagePaint to WebPerformance, but a follow-up CL has to be
done to pick up the metric results from the browser side.

Before this CL, the metrics report the result by dumping a trace event.
This CL adds a new way of reporting. When the metrics fire new results.
The results will be stored in the detector class. At the same time,
the detector will notify web performance of the new result so that
the web performance will pick up the metric results.

This CL also refactors the OnLargest/LastImagePaintDetected function in
ImagePaintTimingDetector for better readability.

Bug: 869924
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I9b198b8bacac52e5dae66ea3ab9c2bedc56cfb8f
Reviewed-on: https://chromium-review.googlesource.com/c/1280844
Reviewed-by: Steve Kobes <skobes@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599976}
[modify] https://crrev.com/8d628983e3a3dcd5174f1793ea5345febec04d89/third_party/blink/public/web/web_performance.h
[modify] https://crrev.com/8d628983e3a3dcd5174f1793ea5345febec04d89/third_party/blink/renderer/core/exported/web_performance.cc
[modify] https://crrev.com/8d628983e3a3dcd5174f1793ea5345febec04d89/third_party/blink/renderer/core/frame/local_frame_view.h
[modify] https://crrev.com/8d628983e3a3dcd5174f1793ea5345febec04d89/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/8d628983e3a3dcd5174f1793ea5345febec04d89/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
[modify] https://crrev.com/8d628983e3a3dcd5174f1793ea5345febec04d89/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc
[modify] https://crrev.com/8d628983e3a3dcd5174f1793ea5345febec04d89/third_party/blink/renderer/core/paint/paint_tracker.cc
[modify] https://crrev.com/8d628983e3a3dcd5174f1793ea5345febec04d89/third_party/blink/renderer/core/paint/paint_tracker.h
[modify] https://crrev.com/8d628983e3a3dcd5174f1793ea5345febec04d89/third_party/blink/renderer/core/timing/performance_timing.cc
[modify] https://crrev.com/8d628983e3a3dcd5174f1793ea5345febec04d89/third_party/blink/renderer/core/timing/performance_timing.h
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 19 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/acb93eec8f2710f7ff0e2675563206149d3080c4

commit acb93eec8f2710f7ff0e2675563206149d3080c4
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Fri Oct 19 15:47:01 2018

[FCP++] Report LargestImagePaint==0 when nodes are all removed

The original implementation does not report an update to performance timing
when the number of nodes is reduced to 0. This CL is to correct this behavior.

Bug: 869924
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Iff578e793965ae89f0e865f175335bb9a318fb9c
Reviewed-on: https://chromium-review.googlesource.com/c/1289702
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601163}
[modify] https://crrev.com/acb93eec8f2710f7ff0e2675563206149d3080c4/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/acb93eec8f2710f7ff0e2675563206149d3080c4/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
[modify] https://crrev.com/acb93eec8f2710f7ff0e2675563206149d3080c4/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 22 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cbdbf10d7b72e2cccc2c3620abe04dff055a2e5a

commit cbdbf10d7b72e2cccc2c3620abe04dff055a2e5a
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Mon Oct 22 18:21:20 2018

[FCP++] Reduce one duplicate DidChangePerformanceTiming for image paint detector

Currently DidChangePerformanceTiming will be invoked twice if both largest-
and last-image candidate are detected. This Cl removes one invocation by using
a bool.

Bug: 869924
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I8342b268d67b4371522370fc2fc45fc8021a6ed9
Reviewed-on: https://chromium-review.googlesource.com/c/1293762
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601656}
[modify] https://crrev.com/cbdbf10d7b72e2cccc2c3620abe04dff055a2e5a/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 22 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b0e730f77658db22bbfebc860a261f1a09badf6b

commit b0e730f77658db22bbfebc860a261f1a09badf6b
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Mon Oct 22 22:20:04 2018

[FCP++] Allow WebPerformance to report Largest/LastTextPaint

In order to report Largest/LastTextPaint to UKM, this CL is to plumb
Largest/LastTextPaint to WebPerformance, but a follow-up CL has to be
done to pick up the metric results from the browser side.

Before this CL, the metrics report the result by dumping a trace event.
This CL adds a new way of reporting. When the metrics fire new results.
The results will be stored in the detector class. At the same time,
the detector will notify web performance of the new result so that
the web performance will pick up the metric results.

Bug: 869924
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Id4c888b725d931cff24a44f19c017404ad9daad1
Reviewed-on: https://chromium-review.googlesource.com/c/1293752
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601749}
[modify] https://crrev.com/b0e730f77658db22bbfebc860a261f1a09badf6b/third_party/blink/public/web/web_performance.h
[modify] https://crrev.com/b0e730f77658db22bbfebc860a261f1a09badf6b/third_party/blink/renderer/core/exported/web_performance.cc
[modify] https://crrev.com/b0e730f77658db22bbfebc860a261f1a09badf6b/third_party/blink/renderer/core/paint/paint_tracker.cc
[modify] https://crrev.com/b0e730f77658db22bbfebc860a261f1a09badf6b/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
[modify] https://crrev.com/b0e730f77658db22bbfebc860a261f1a09badf6b/third_party/blink/renderer/core/paint/text_paint_timing_detector.h
[modify] https://crrev.com/b0e730f77658db22bbfebc860a261f1a09badf6b/third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc
[modify] https://crrev.com/b0e730f77658db22bbfebc860a261f1a09badf6b/third_party/blink/renderer/core/timing/performance_timing.cc
[modify] https://crrev.com/b0e730f77658db22bbfebc860a261f1a09badf6b/third_party/blink/renderer/core/timing/performance_timing.h
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 23 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03bb0f890c9446435b197f4ff1d5a12b65b04d51

commit 03bb0f890c9446435b197f4ff1d5a12b65b04d51
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Tue Oct 23 23:02:50 2018

[FCP++] Add Largest Image Paint to UKM and UMA

We need to report Largest Image Paint to UKM and UMA. This CL
uses mojo to send the metric from web performance (Blink) to the
page load metrics (Browser). The browser then reports the metric
to UKM and UMA.

Privacy review: https://docs.google.com/document/d/1LJnD-INFo7UnvPyB6C-tzpJ3BLK9rYA2SbnwTcsyeTs/edit#

Bug: 869924
Change-Id: I7825634630f924c6016c3d24cf295b33acd6ba3b
Reviewed-on: https://chromium-review.googlesource.com/c/1283474
Reviewed-by: Martin Barbella <mbarbella@chromium.org>
Reviewed-by: Gayane Petrosyan <gayane@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602155}
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.h
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/chrome/browser/page_load_metrics/page_load_metrics_observer.h
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/chrome/browser/page_load_metrics/page_load_tracker.cc
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/chrome/common/page_load_metrics/page_load_metrics.mojom
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/chrome/common/page_load_metrics/page_load_timing.cc
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/tools/metrics/histograms/histograms.xml
[modify] https://crrev.com/03bb0f890c9446435b197f4ff1d5a12b65b04d51/tools/metrics/ukm/ukm.xml
Project Member

Comment 13 by bugdroid1@chromium.org, Oct 24 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ff4163102f8b4904d74e0649862862ddaa8d2d24

commit ff4163102f8b4904d74e0649862862ddaa8d2d24
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Wed Oct 24 15:46:19 2018

[FCP++] Fix inconsistency between PageLoadTimingStatus and enums.xml

We introduced a bug in cl/1283474. The added item should have been to the end
of PageLoadTimingStatus, and corresponding change should have been made to
enums.xml. And, the added item was actually not necessary. This Cl remove the
introduced item.

Bug: 869924
Change-Id: Idedde9b86f82ea027cb7924369364f068559a82e
Reviewed-on: https://chromium-review.googlesource.com/c/1297743
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602351}
[modify] https://crrev.com/ff4163102f8b4904d74e0649862862ddaa8d2d24/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
Project Member

Comment 14 by bugdroid1@chromium.org, Oct 25 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dc03f2edecafed5c3c672627dd9c2e3e48cdd7b0

commit dc03f2edecafed5c3c672627dd9c2e3e48cdd7b0
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Thu Oct 25 15:49:56 2018

[FCP++] Update Last/Largest Text Paint to null when all nodes are removed

In the original implementation, Last/Last Text Paint only update the
candidate when they are not null. However, an edge case is that when
all nodes are removed from the page. For this scenario, we should
update the candidate to be null accordingly. This CL is to include this
edge case.

Bug: 869924
Change-Id: I9d82cf77e26f06fb7bc68452414a0aa2bc27ec49
Reviewed-on: https://chromium-review.googlesource.com/c/1298338
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602721}
[modify] https://crrev.com/dc03f2edecafed5c3c672627dd9c2e3e48cdd7b0/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
[modify] https://crrev.com/dc03f2edecafed5c3c672627dd9c2e3e48cdd7b0/third_party/blink/renderer/core/paint/text_paint_timing_detector.h
[modify] https://crrev.com/dc03f2edecafed5c3c672627dd9c2e3e48cdd7b0/third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc
Project Member

Comment 15 by bugdroid1@chromium.org, Oct 25 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/60619ceecbd9af54a5aa9ece5234312e33fe48ad

commit 60619ceecbd9af54a5aa9ece5234312e33fe48ad
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Thu Oct 25 22:56:56 2018

[FCP++] Add Last Image Paint to UKM and UMA

We need to report Last Image Paint to UKM and UMA. This CL
uses mojo to send the metric from web performance (Blink) to the
page load metrics (Browser). The browser then reports the metric
to UKM and UMA.

Bug: 869924
Change-Id: I680576ec2940558ed38910e2b4e2ff695a41ea30
Reviewed-on: https://chromium-review.googlesource.com/c/1298109
Reviewed-by: Martin Barbella <mbarbella@chromium.org>
Reviewed-by: Gayane Petrosyan <gayane@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602913}
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/chrome/browser/page_load_metrics/page_load_metrics_observer.h
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/chrome/browser/page_load_metrics/page_load_tracker.cc
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/chrome/common/page_load_metrics/page_load_metrics.mojom
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/chrome/common/page_load_metrics/page_load_timing.cc
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/tools/metrics/histograms/histograms.xml
[modify] https://crrev.com/60619ceecbd9af54a5aa9ece5234312e33fe48ad/tools/metrics/ukm/ukm.xml
Project Member

Comment 16 by bugdroid1@chromium.org, Oct 29 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b571b9315306b1d958c4185bb2ed3a486c160bd6

commit b571b9315306b1d958c4185bb2ed3a486c160bd6
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Mon Oct 29 18:05:18 2018

[FCP++] Add Largest/Last Text Paint to UKM, UMA

We need to report Last Text Paint and Largest Text Paint to UKM
and UMA. This CL uses mojo to send the metric from web performance
(Blink) to the page load metrics (Browser). The browser then
reports the metric to UKM and UMA.

Bug: 869924
Change-Id: I44ac017cd06df6dc5bae264442f27900a027bbe7
Reviewed-on: https://chromium-review.googlesource.com/c/1301966
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Gayane Petrosyan <gayane@chromium.org>
Reviewed-by: Bryan McQuade <bmcquade@chromium.org>
Reviewed-by: Martin Barbella <mbarbella@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603558}
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.h
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer.cc
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/chrome/browser/page_load_metrics/observers/ukm_page_load_metrics_observer_unittest.cc
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/chrome/browser/page_load_metrics/page_load_metrics_observer.h
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.cc
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/chrome/browser/page_load_metrics/page_load_metrics_update_dispatcher.h
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/chrome/browser/page_load_metrics/page_load_tracker.cc
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/chrome/common/page_load_metrics/page_load_metrics.mojom
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/chrome/common/page_load_metrics/page_load_timing.cc
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/chrome/renderer/page_load_metrics/metrics_render_frame_observer.cc
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/tools/metrics/histograms/histograms.xml
[modify] https://crrev.com/b571b9315306b1d958c4185bb2ed3a486c160bd6/tools/metrics/ukm/ukm.xml
Project Member

Comment 17 by bugdroid1@chromium.org, Oct 30 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2d6fff6e1e7a9910c92ce851e1d5f6f43ff5b99c

commit 2d6fff6e1e7a9910c92ce851e1d5f6f43ff5b99c
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Tue Oct 30 17:28:09 2018

[FCP++] Support video image and SVG image in Last/LargestImagePaint

Last Image Paint and Largest Image Paint concern about the load time of images
because they are highly relevant to user experiences. Initially, the metrics
only take <image> into account. As video image and svg image are equally
important, we should consider video poster image and SVG image as well.

Video image example:
<video poster="http://example.com/nonexistant.gif"></video>

SVG image example:
<svg id='svg'>
  <image xlink:href="http://example.com/nonexistant.jpg"/>
</svg>

Bug: 869924
Change-Id: Id414d236296829c4423e1f69dc140f54cb91444f
Reviewed-on: https://chromium-review.googlesource.com/c/1305753
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603943}
[modify] https://crrev.com/2d6fff6e1e7a9910c92ce851e1d5f6f43ff5b99c/third_party/blink/renderer/core/html/media/html_video_element.h
[modify] https://crrev.com/2d6fff6e1e7a9910c92ce851e1d5f6f43ff5b99c/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/2d6fff6e1e7a9910c92ce851e1d5f6f43ff5b99c/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
[modify] https://crrev.com/2d6fff6e1e7a9910c92ce851e1d5f6f43ff5b99c/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc
[modify] https://crrev.com/2d6fff6e1e7a9910c92ce851e1d5f6f43ff5b99c/third_party/blink/renderer/core/paint/paint_tracker.cc
[modify] https://crrev.com/2d6fff6e1e7a9910c92ce851e1d5f6f43ff5b99c/third_party/blink/renderer/core/svg/svg_image_element.h
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 5 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8414e5e74db5397689dbdc0ce53d2f9af19f3b43

commit 8414e5e74db5397689dbdc0ce53d2f9af19f3b43
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Mon Nov 05 17:50:25 2018

[FCP++Image] Assign frame index to record when image is loaded

The initial implementation has a bug that the frame index is assigned to the
record when a record is created. This is incorrect. This should happen when
the image is loaded instead, as frame index is used to indicate which frame a
loaded image belongs to in order for the swap time to be assigned to the right
records. This CL is to correct this bug.

Bug: 869924
Change-Id: I4a371183d66ed33b7ac87cb9a570e7221a2a7c4f
Reviewed-on: https://chromium-review.googlesource.com/c/1316663
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605371}
[modify] https://crrev.com/8414e5e74db5397689dbdc0ce53d2f9af19f3b43/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/8414e5e74db5397689dbdc0ce53d2f9af19f3b43/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc
Project Member

Comment 19 by bugdroid1@chromium.org, Nov 5 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/44e0b0ba6ea98bd88e7c41abc1a987bd53c5d6d7

commit 44e0b0ba6ea98bd88e7c41abc1a987bd53c5d6d7
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Mon Nov 05 22:30:58 2018

[FCP++, LastImage] Redefine "last" as last load from last added

Last Image Paint takes the last image based on the order of first paint index.
In the initial implementation, the first paint index was the time the image is
attached to DOM tree. As an alternative, the first paint index can also be the
time the image is loaded. By observation, we find that it's more natural for
users to regard the last image as the last one being loaded other than the last
one being attached to the tree, as the latter may not be visible to users yet.

Therefore, this CL is to change the definition of "last" in Last Image Paint to
the last image being loaded.

Bug: 869924
Change-Id: Ib5d0addf10c32668b38297c2efe5e73b7e1e7a33
Reviewed-on: https://chromium-review.googlesource.com/c/1316360
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605497}
[modify] https://crrev.com/44e0b0ba6ea98bd88e7c41abc1a987bd53c5d6d7/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/44e0b0ba6ea98bd88e7c41abc1a987bd53c5d6d7/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc

Comment 20 by st...@souders.org, Nov 9 

Would be great if this took background images into consideration if it's not already.

Comment 21 by maxlg@google.com, Nov 9 

Yep! It's part of the plan. But the major issue around it is performance.

Background image lives as a style property. It can be attached to many different objects. And we need to loop through each BackgroundLayers to find whether any of them has background image.

So it seems more steps to take than image or video poster image. But I am not sure about the real performance impact. I can try implementing it first and take a look.
Project Member

Comment 22 by bugdroid1@chromium.org, Nov 13 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/952b1a871fd7b3b4a629e253bb6eb21b681c60ba

commit 952b1a871fd7b3b4a629e253bb6eb21b681c60ba
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Tue Nov 13 18:58:26 2018

[FCP++] Refactor Image Paint Detector

This CL refactors the codes of Image Paint Detecor. This will make it easier to
support background image for the metric.

Specifically, this CL will do the following things:
1. Move GetImageUrl() into an anonymous function.
2. Simplify the logic of IsJustLoaded(). Take out the IsLoad() part from it and move
it into an anonymous function.

Background image support will be done by extending these two functions.

Bug: 869924
Change-Id: Idfe4794612e1d5daa71afb03f8191700ce3d65bb
Reviewed-on: https://chromium-review.googlesource.com/c/1333990
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607682}
[modify] https://crrev.com/952b1a871fd7b3b4a629e253bb6eb21b681c60ba/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/952b1a871fd7b3b4a629e253bb6eb21b681c60ba/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
Project Member

Comment 24 by bugdroid1@chromium.org, Nov 15 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40935b1b80a72b8eaca1aab6801244e1d7a6317a

commit 40935b1b80a72b8eaca1aab6801244e1d7a6317a
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Thu Nov 15 17:05:30 2018

[FCP++] Support background image in Image Paint Timing

For largest, last image paint timing, we have supported img tag, svg image,
and video image poster, but we haven't included background image. The purpose
of this CL is to include background-image in the image paint timing.

The background image that FCP++ cares about is contentful background image,
which are used as content by web pages, as opposed to "backgroundful"
background image, which are used for background purpose by web pages.

We have a few heuristics to rule out non-contentful background images:
* If a background image is attached to <body>, <html>, then it's less likely to
be contentful.
* If a background image does not have image resources, then it's not contentful.

Background image differs from other supported image in various aspects:
* Background images are attached to the style of objects.
* One object can have multiple background images.

Accordingly, we define "loaded" for an object attaching background-images as
all of its background images have been loaded.

These aspects are reflected in this implementation.

Bug: 869924
Change-Id: Icc4962420e7c8ba213c9f3f1124a413c4b545957
Reviewed-on: https://chromium-review.googlesource.com/c/1334332
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608404}
[modify] https://crrev.com/40935b1b80a72b8eaca1aab6801244e1d7a6317a/third_party/blink/renderer/core/layout/layout_object.h
[modify] https://crrev.com/40935b1b80a72b8eaca1aab6801244e1d7a6317a/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/40935b1b80a72b8eaca1aab6801244e1d7a6317a/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
[modify] https://crrev.com/40935b1b80a72b8eaca1aab6801244e1d7a6317a/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc
[modify] https://crrev.com/40935b1b80a72b8eaca1aab6801244e1d7a6317a/third_party/blink/renderer/core/paint/paint_tracker.cc
[modify] https://crrev.com/40935b1b80a72b8eaca1aab6801244e1d7a6317a/third_party/blink/renderer/core/style/style_fetched_image.h
Project Member

Comment 25 by bugdroid1@chromium.org, Nov 15 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/adb4651a7ed32135395d66b3efeb75714b1d94fb

commit adb4651a7ed32135395d66b3efeb75714b1d94fb
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Thu Nov 15 21:40:04 2018

[FCP++] Rename PaintTracking flag to FCPPlusPlus

Rename PaintTracking flag to FirstContentfulPaintPlusPlus. The flag is meant to
serve specifically to FCP++ metrics. So the naming should reflect this goal.

This CL renames the PaintTracking flag in runtime_enabled_features.

Bug: 869924
Change-Id: Ib3cbd8ac21ab962e87626edf74e13dfc8fde486e
Reviewed-on: https://chromium-review.googlesource.com/c/1338237
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608524}
[modify] https://crrev.com/adb4651a7ed32135395d66b3efeb75714b1d94fb/third_party/blink/renderer/core/layout/layout_object.cc
[modify] https://crrev.com/adb4651a7ed32135395d66b3efeb75714b1d94fb/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc
[modify] https://crrev.com/adb4651a7ed32135395d66b3efeb75714b1d94fb/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc
[modify] https://crrev.com/adb4651a7ed32135395d66b3efeb75714b1d94fb/third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc
[modify] https://crrev.com/adb4651a7ed32135395d66b3efeb75714b1d94fb/third_party/blink/renderer/platform/runtime_enabled_features.json5
Project Member

Comment 26 by bugdroid1@chromium.org, Nov 17 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4e243fc4a328acee8354c73e7c9df2facac9f3f7

commit 4e243fc4a328acee8354c73e7c9df2facac9f3f7
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Sat Nov 17 00:13:02 2018

[FCP++] Enable FCP++ metrics via finch experiment

Create a Finch experiment to turn on FCP++ metrics.

We will monitor the heart beat metrics to guarantee that FCP++ won't
introduce regressions.

Bug: 869924
Change-Id: I68b7e446c4e49bc173dc1eccf6e88a9284f90bcd
Reviewed-on: https://chromium-review.googlesource.com/c/1336555
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609039}
[modify] https://crrev.com/4e243fc4a328acee8354c73e7c9df2facac9f3f7/content/child/runtime_features.cc
[modify] https://crrev.com/4e243fc4a328acee8354c73e7c9df2facac9f3f7/testing/variations/fieldtrial_testing_config.json
[modify] https://crrev.com/4e243fc4a328acee8354c73e7c9df2facac9f3f7/third_party/blink/common/features.cc
[modify] https://crrev.com/4e243fc4a328acee8354c73e7c9df2facac9f3f7/third_party/blink/public/common/features.h
[modify] https://crrev.com/4e243fc4a328acee8354c73e7c9df2facac9f3f7/third_party/blink/public/platform/web_runtime_features.h
[modify] https://crrev.com/4e243fc4a328acee8354c73e7c9df2facac9f3f7/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/4e243fc4a328acee8354c73e7c9df2facac9f3f7/third_party/blink/renderer/platform/exported/web_runtime_features.cc
Project Member

Comment 27 by bugdroid1@chromium.org, Nov 19 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8e7b749e81cd0e3371a57bce6b554489c16b1971

commit 8e7b749e81cd0e3371a57bce6b554489c16b1971
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Mon Nov 19 19:21:43 2018

[FCP++] Image Paint Timing: node removed between callback registration and
invocation

In the current implementation, we assumed that whenever callback is invoked,
there must be at least one records needing to assign swap time to itself.
However, this ignored the possibily that nodes may be deleted between a callback
is registered and invoked.

Because the assumption is violated, we should remove the dcheck.

Bug: 869924
Change-Id: Ifdfda96fd2260f85add1c2a78ad48cff22cd0dbf
Reviewed-on: https://chromium-review.googlesource.com/c/1340966
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609388}
[modify] https://crrev.com/8e7b749e81cd0e3371a57bce6b554489c16b1971/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/8e7b749e81cd0e3371a57bce6b554489c16b1971/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc
Project Member

Comment 29 by 42576172...@developer.gserviceaccount.com, Nov 22 

😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/15db9e3de40000

All of the runs failed. The most common error (1/20 runs) was:
IOError: [Errno 2] No such file or directory: '/b/s/w/itvix3DB/tmpw6vRsctelemetry/histograms.json'
Project Member

Comment 30 by bugdroid1@chromium.org, Nov 22 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f430ff4b5932ac96c95c228446dec1e9ef98f865

commit f430ff4b5932ac96c95c228446dec1e9ef98f865
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Thu Nov 22 21:22:05 2018

[FCP++] Renaming paint-tracker as paint-timing-detector

The code base uses PaintTracker as the class to deal with FirstContentfulPaint++
logic. As it's specific for FCP++, we should give it a more specific name.
PaintTimingDetector will be more clear about its intention.

Bug: 869924
Change-Id: I194eeac2b40d1db3e719332e8a7cfe707a881b0e
Reviewed-on: https://chromium-review.googlesource.com/c/1348250
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610489}
[modify] https://crrev.com/f430ff4b5932ac96c95c228446dec1e9ef98f865/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/f430ff4b5932ac96c95c228446dec1e9ef98f865/third_party/blink/renderer/core/frame/local_frame_view.h
[modify] https://crrev.com/f430ff4b5932ac96c95c228446dec1e9ef98f865/third_party/blink/renderer/core/layout/layout_object.cc
[modify] https://crrev.com/f430ff4b5932ac96c95c228446dec1e9ef98f865/third_party/blink/renderer/core/paint/BUILD.gn
[modify] https://crrev.com/f430ff4b5932ac96c95c228446dec1e9ef98f865/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/f430ff4b5932ac96c95c228446dec1e9ef98f865/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc
[rename] https://crrev.com/f430ff4b5932ac96c95c228446dec1e9ef98f865/third_party/blink/renderer/core/paint/paint_timing_detector.cc
[rename] https://crrev.com/f430ff4b5932ac96c95c228446dec1e9ef98f865/third_party/blink/renderer/core/paint/paint_timing_detector.h
[modify] https://crrev.com/f430ff4b5932ac96c95c228446dec1e9ef98f865/third_party/blink/renderer/core/paint/pre_paint_tree_walk.cc
[modify] https://crrev.com/f430ff4b5932ac96c95c228446dec1e9ef98f865/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
[modify] https://crrev.com/f430ff4b5932ac96c95c228446dec1e9ef98f865/third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc
[modify] https://crrev.com/f430ff4b5932ac96c95c228446dec1e9ef98f865/third_party/blink/renderer/core/timing/performance_timing.cc
[modify] https://crrev.com/f430ff4b5932ac96c95c228446dec1e9ef98f865/third_party/blink/renderer/core/timing/performance_timing.h
Project Member

Comment 31 by bugdroid1@chromium.org, Nov 23 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5f5daa120e13faa4e68bd307bf9bec5111b4590b

commit 5f5daa120e13faa4e68bd307bf9bec5111b4590b
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Fri Nov 23 21:18:44 2018

[FCP++] Refactor CalculateVisualSize in TextPaint and ImagePaint

TextPaintTimingDetector and ImagePaintTimingDetector have the same codes to
calculate the visual size of a rect. We can move this common part into
PaintTimingDetector.

This CL also refactor the logic in CalculateVisualSize for simplification.
The new signature return the size value instead of the rect. By do this, we can
save the logic of getting size from rect in RecordText and RecordImage.

Bug: 869924
Change-Id: Icc2b88db90c1816e24f8ba7446ada701996fc652
Reviewed-on: https://chromium-review.googlesource.com/c/1348859
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610668}
[modify] https://crrev.com/5f5daa120e13faa4e68bd307bf9bec5111b4590b/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/5f5daa120e13faa4e68bd307bf9bec5111b4590b/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
[modify] https://crrev.com/5f5daa120e13faa4e68bd307bf9bec5111b4590b/third_party/blink/renderer/core/paint/image_paint_timing_detector_test.cc
[modify] https://crrev.com/5f5daa120e13faa4e68bd307bf9bec5111b4590b/third_party/blink/renderer/core/paint/paint_timing_detector.cc
[modify] https://crrev.com/5f5daa120e13faa4e68bd307bf9bec5111b4590b/third_party/blink/renderer/core/paint/paint_timing_detector.h
[modify] https://crrev.com/5f5daa120e13faa4e68bd307bf9bec5111b4590b/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
[modify] https://crrev.com/5f5daa120e13faa4e68bd307bf9bec5111b4590b/third_party/blink/renderer/core/paint/text_paint_timing_detector.h
Project Member

Comment 32 by bugdroid1@chromium.org, Nov 28 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cb1defbbca58ec09638f38c510fe36fc3dafe30f

commit cb1defbbca58ec09638f38c510fe36fc3dafe30f
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Wed Nov 28 20:22:48 2018

[FCP++] Simplify reactivation logic in TextPaint, ImagePaint

The design of FCP++ have several containers that store the node ids of objects.
In case the size of them will grow unlimitedly, we set a cap for these
containers.

The original implementation uses a counter to count the number of nodes. As a
better approach, we can use the sum of the sizes of the containers for the
same purpose.

The original implementation uses "recorded_node_count_ > kTextNodeNumberLimit"
as a condition. The condition was still checked each time after the condition
has been met. As A better approach, we can have "is_recording_" flag to
indicate whether the container still accepts new entries.

Bug: 869924
Change-Id: Ifc03e5f782e16bab65f6f024d9f73974051ca421
Reviewed-on: https://chromium-review.googlesource.com/c/1349477
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611849}
[modify] https://crrev.com/cb1defbbca58ec09638f38c510fe36fc3dafe30f/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/cb1defbbca58ec09638f38c510fe36fc3dafe30f/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
[modify] https://crrev.com/cb1defbbca58ec09638f38c510fe36fc3dafe30f/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
[modify] https://crrev.com/cb1defbbca58ec09638f38c510fe36fc3dafe30f/third_party/blink/renderer/core/paint/text_paint_timing_detector.h
Project Member

Comment 33 by bugdroid1@chromium.org, Nov 29 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7699027001775612e0bd04d0c8849cb77b231e5

commit d7699027001775612e0bd04d0c8849cb77b231e5
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Thu Nov 29 23:58:36 2018

[FCP++] TextPaint: inconsistency for node removal while pending swap time

In the current implementation, it's possible to create inconsistency while
a node waiting to be assigned with a swap time is removed. When this happens,
the detector still creates a record of the removed text, but it shouldn't, as
the created record could become the candidate metric result.

The cause is that when a record is assigned with a swap time, there is no check
of whether the node has been removed.

To fix this issue, we should mark the node as deleted when the node is removed
from the dom tree, and add the checking of validity when we assign the swap time
to the record.

The CL implements this fix.

Bug: 869924
Change-Id: Ie331d40fbad585cf32828c1b99136056d429c5b0
Reviewed-on: https://chromium-review.googlesource.com/c/1354130
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Liquan (Max) Gǔ <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612444}
[modify] https://crrev.com/d7699027001775612e0bd04d0c8849cb77b231e5/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
[modify] https://crrev.com/d7699027001775612e0bd04d0c8849cb77b231e5/third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc

Comment 34 by maxlg@chromium.org, Dec 12

Summary: [FCP++] Implement FCP++ metrics (was: [FCP++] Need a more accurate contentful paint metric)
Project Member

Comment 35 by bugdroid1@chromium.org, Jan 10 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aec1fc8002a2b18ce3e641c52ddc96e613ce27ed

commit aec1fc8002a2b18ce3e641c52ddc96e613ce27ed
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Thu Jan 10 16:39:59 2019

[FCP++] TextPaint: refactor tests to remove text field as dependency

Originally we had "text" as a string in TextRecord in TextPaintTimingDetector.
But we found that the text string is a big memory cost when text records add up.
So we intend to remove it in
https://chromium-review.googlesource.com/c/chromium/src/+/1401361. In order to
split the change, we write this change to remove the text field from being a
dependency in test.

We've also refactored the tests to facilitate the change and also make the tests
more readable.

Bug: 869924
Change-Id: Ibdfb0a0426bf2693fcb4aec9c9d78fcb4100a1a4
Reviewed-on: https://chromium-review.googlesource.com/c/1404062
Reviewed-by: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621601}
[modify] https://crrev.com/aec1fc8002a2b18ce3e641c52ddc96e613ce27ed/third_party/blink/renderer/core/paint/text_paint_timing_detector_test.cc
Project Member

Comment 36 by bugdroid1@chromium.org, Jan 10 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e96f86b33f529ae70abc407f6e0093cbb4d95477

commit e96f86b33f529ae70abc407f6e0093cbb4d95477
Author: Liquan(Max) Gu <maxlg@chromium.org>
Date: Thu Jan 10 21:44:25 2019

[FCP++] Hide debug info behind a flag

FCP++ causes regression to memory. Two of the main culprits are the text
strings and the image urls it contains. These two pieces of information only
serve for debugging purpose, so we had better to hide them while we do not
debug the feature.

In order to do that, we guard the fields with a NDEBUG flag, the storing of
text string and image url will only be enabled when the flag is enabled.

Bug: 869924
Change-Id: Ib56678e1316ef8ba19d5ae22104967f0952a014a
Reviewed-on: https://chromium-review.googlesource.com/c/1401361
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Steve Kobes <skobes@chromium.org>
Reviewed-by: Timothy Dresser <tdresser@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621742}
[modify] https://crrev.com/e96f86b33f529ae70abc407f6e0093cbb4d95477/third_party/blink/renderer/core/paint/image_paint_timing_detector.cc
[modify] https://crrev.com/e96f86b33f529ae70abc407f6e0093cbb4d95477/third_party/blink/renderer/core/paint/image_paint_timing_detector.h
[modify] https://crrev.com/e96f86b33f529ae70abc407f6e0093cbb4d95477/third_party/blink/renderer/core/paint/text_paint_timing_detector.cc
[modify] https://crrev.com/e96f86b33f529ae70abc407f6e0093cbb4d95477/third_party/blink/renderer/core/paint/text_paint_timing_detector.h
►

Sign in to add a comment