Notify paint for each frame

Notify paint for each frame

Currently main frame reports paint timing and subframes report paint
timing via main frame. The desired behavior is that each frame reports
its own paint timing.

BUG=705315
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2872793002
Cr-Commit-Position: refs/heads/master@{#471922}
Committed: https://chromium.googlesource.com/chromium/src/+/5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a

[Zhen Wang, 2017-05-08 21:57:04]

Description was changed from

==========
Notify paint for each frame

BUG=705315
==========

to

==========
Notify paint for each frame

BUG=705315
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Zhen Wang, 2017-05-08 22:00:12]

Description was changed from

==========
Notify paint for each frame

BUG=705315
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Notify paint for each frame

Currently main frame reports paint timing and subframes report paint
timing via main frame. The desired behavior is that each frame reports
its own paint timing.

BUG=705315
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[zhenw, 2017-05-08 22:07:52]

zhenw@google.com changed reviewers:
+ panicker@chromium.org, wangxianzhu@chromium.org, zhenw@google.com

[zhenw, 2017-05-08 22:07:54]

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:3265:
FramePainter(graphics_context, *this).NotifyPaint();
Paint() on line 3263 will eventually call NotifyPaint(). But
CommitNewDispalyItems() on line 3264 will update first paint. So I do another
explicit NotifyPaint() here.

This seems like a hack. Any suggestion on a better way?

[Zhen Wang, 2017-05-08 22:29:35]

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

[commit-bot: I haz the power, 2017-05-08 22:36:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2017-05-08 22:44:46]

Please also add unit tests.

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:3265:
FramePainter(graphics_context, *this).NotifyPaint();
On 2017/05/08 22:07:54, zhenw wrote:
> Paint() on line 3263 will eventually call NotifyPaint(). But
> CommitNewDispalyItems() on line 3264 will update first paint. So I do another
> explicit NotifyPaint() here.
> 
> This seems like a hack. Any suggestion on a better way?

This is not needed as long as we call NotifyPaint() just after calling
PaintController::EndFrame().

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:593: }
The above block should be moved into ProcessNewItem().
  if (item.IsDrawing() && !frame_first_paints_.back().first_painted &&
      ...
    SetFirstPainted();

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:349:
HashMap<const void*, bool> image_painted_;
HashMap is much slower than direct field to access, so this will affect
PaintController performance.

How about the following method:

  struct FrameFirstPaint {
    FrameFirstPaint(const void* frame) : frame(frame), ... {}

    const void* frame;
    bool first_painted : 1;
    bool text_painted : 1;
    bool image_painted : 1;
  };

  Vector<FrameAndFirstPaint> frame_first_paints_;

void SetFirstPainted() {
  frame_first_paints_.back().first_painted = true;
}

FrameFirstPaint EndFrame() {
  auto result = frame_first_paints_.back();
  frame_first_paints_.pop_back();
  return result;
}

[commit-bot: I haz the power, 2017-05-08 23:27:50]

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

[commit-bot: I haz the power, 2017-05-08 23:27:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[Zhen Wang, 2017-05-09 20:08:58]

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

[commit-bot: I haz the power, 2017-05-09 20:10:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhen Wang, 2017-05-09 20:13:43]

> Please also add unit tests.

Do you mean layout test?

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/FrameView.cpp:3265:
FramePainter(graphics_context, *this).NotifyPaint();
Removed

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:593: }
On 2017/05/08 22:44:46, Xianzhu wrote:
> The above block should be moved into ProcessNewItem().
>   if (item.IsDrawing() && !frame_first_paints_.back().first_painted &&
>       ...
>     SetFirstPainted();

Done.

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2872793002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:349:
HashMap<const void*, bool> image_painted_;
On 2017/05/08 22:44:46, Xianzhu wrote:
> HashMap is much slower than direct field to access, so this will affect
> PaintController performance.
> 
> How about the following method:
> 
>   struct FrameFirstPaint {
>     FrameFirstPaint(const void* frame) : frame(frame), ... {}
> 
>     const void* frame;
>     bool first_painted : 1;
>     bool text_painted : 1;
>     bool image_painted : 1;
>   };
> 
>   Vector<FrameAndFirstPaint> frame_first_paints_;
> 
> void SetFirstPainted() {
>   frame_first_paints_.back().first_painted = true;
> }
> 
> FrameFirstPaint EndFrame() {
>   auto result = frame_first_paints_.back();
>   frame_first_paints_.pop_back();
>   return result;
> }

Done.

[Xianzhu, 2017-05-09 21:07:57]

https://codereview.chromium.org/2872793002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2872793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:289: if
(!frame_first_paints_.back().first_painted && display_item.IsDrawing() &&
PaintController might paint something before BeginFrame() (e.g. when
PaintController is used to paint an SVGImage), and back() will crash in the
case.

I think we can call BeginFrame(nullptr) from PaintController::PaintController()
to avoid the problem. (This will also enable us to combine the first paint data
in this PaintController into another PaintController on which we replay the
recorded result of this PaintController, in the future).

[Xianzhu, 2017-05-09 21:10:52]

On 2017/05/09 20:13:43, Zhen Wang wrote:
> > Please also add unit tests.
> 
> Do you mean layout test?

I mean unit test in PaintControllerTest.cpp to test the behavior of BeginFrame()
and EndFrame().

Layout tests will be also useful for testing per-frame paint timing at the web
api level.

[commit-bot: I haz the power, 2017-05-09 21:23:45]

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

[commit-bot: I haz the power, 2017-05-09 21:23:46]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Zhen Wang, 2017-05-09 22:59:40]

Also added unit test.

https://codereview.chromium.org/2872793002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):

https://codereview.chromium.org/2872793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:289: if
(!frame_first_paints_.back().first_painted && display_item.IsDrawing() &&
On 2017/05/09 21:07:56, Xianzhu wrote:
> PaintController might paint something before BeginFrame() (e.g. when
> PaintController is used to paint an SVGImage), and back() will crash in the
> case.
> 
> I think we can call BeginFrame(nullptr) from
PaintController::PaintController()
> to avoid the problem. (This will also enable us to combine the first paint
data
> in this PaintController into another PaintController on which we replay the
> recorded result of this PaintController, in the future). 

Done.

[Xianzhu, 2017-05-09 23:13:09]

lgtm.

+chrishtr@ as an owner of platform/.

https://codereview.chromium.org/2872793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2872793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:249:
frame_first_paints_.push_back(FrameFirstPaint(nullptr));
Please add a comment to explain the reason.

https://codereview.chromium.org/2872793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp
(right):

https://codereview.chromium.org/2872793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:2063:
FrameFirstPaint result = GetPaintController().EndFrame(nullptr);
Add a SetXXXPainted() before this line to ensure it won't crash before the first
BeginFrame().

[Zhen Wang, 2017-05-09 23:26:23]

zhenw@chromium.org changed reviewers:
+ chrishtr@chromium.org

[Zhen Wang, 2017-05-09 23:26:24]

+chrishtr for platform/

Shubhie, which layout test should we use to test this?

https://codereview.chromium.org/2872793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
(right):

https://codereview.chromium.org/2872793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.h:249:
frame_first_paints_.push_back(FrameFirstPaint(nullptr));
On 2017/05/09 23:13:08, Xianzhu wrote:
> Please add a comment to explain the reason.

Done.

https://codereview.chromium.org/2872793002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp
(right):

https://codereview.chromium.org/2872793002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:2063:
FrameFirstPaint result = GetPaintController().EndFrame(nullptr);
On 2017/05/09 23:13:08, Xianzhu wrote:
> Add a SetXXXPainted() before this line to ensure it won't crash before the
first
> BeginFrame().

Done.

[Zhen Wang, 2017-05-09 23:27:11]

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

[commit-bot: I haz the power, 2017-05-09 23:28:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 23:53:56]

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

[commit-bot: I haz the power, 2017-05-09 23:53:56]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[panicker, 2017-05-10 00:23:28]

On 2017/05/09 23:26:24, Zhen Wang wrote:
> +chrishtr for platform/
> 
> Shubhie, which layout test should we use to test this?
I haven't checked in the layout test as it was broken for child frames
I'd suggest just adding a simplelayout test that wraps this html in an iframe:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
Something like:
<!DOCTYPE html>
<head>
<title>Performance Paint Timing Iframe Test</title>
</head>
<body>
  <iframe src="./observable.html"></iframe>
</body>
</html>

> 
>
https://codereview.chromium.org/2872793002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/paint/PaintController.h
> (right):
> 
>
https://codereview.chromium.org/2872793002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/paint/PaintController.h:249:
> frame_first_paints_.push_back(FrameFirstPaint(nullptr));
> On 2017/05/09 23:13:08, Xianzhu wrote:
> > Please add a comment to explain the reason.
> 
> Done.
> 
>
https://codereview.chromium.org/2872793002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp
> (right):
> 
>
https://codereview.chromium.org/2872793002/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/platform/graphics/paint/PaintControllerTest.cpp:2063:
> FrameFirstPaint result = GetPaintController().EndFrame(nullptr);
> On 2017/05/09 23:13:08, Xianzhu wrote:
> > Add a SetXXXPainted() before this line to ensure it won't crash before the
> first
> > BeginFrame().
> 
> Done.

[Zhen Wang, 2017-05-10 17:43:25]

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

[commit-bot: I haz the power, 2017-05-10 17:45:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhen Wang, 2017-05-10 18:22:22]

> > Shubhie, which layout test should we use to test this?
> I haven't checked in the layout test as it was broken for child frames
> I'd suggest just adding a simplelayout test that wraps this html in an iframe:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/http/test...
> Something like:
> <!DOCTYPE html>
> <head>
> <title>Performance Paint Timing Iframe Test</title>
> </head>
> <body>
>   <iframe src="./observable.html"></iframe>
> </body>
> </html>

I tried your testiframe.html and child.html, and it worked fine. But it fails
http/tests/performance-timing/paint-timing/observable.html (timeout). Maybe we
can look at it together.

[commit-bot: I haz the power, 2017-05-10 19:27:53]

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

[commit-bot: I haz the power, 2017-05-10 19:27:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Zhen Wang, 2017-05-10 22:16:17]

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

[commit-bot: I haz the power, 2017-05-10 22:17:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhen Wang, 2017-05-10 22:46:07]

Updated CL to fix layout tests. Xianzhu, can you take another look?


Shubhie, I tested the CL with testiframe.html locally. From some debug logging I
added to PaintTiming::NotifyPaint(), I know that both frames has first paint
set. But the console is only printing the child ones. The main frame one will
only be printed out if the child frame is removed. This seems to be some problem
when notifying the paint timing to the JS layer.

https://codereview.chromium.org/2872793002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FramePainter.cpp (right):

https://codereview.chromium.org/2872793002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FramePainter.cpp:36:
frame_paint_timing_(context, &frame_view.GetFrame()) {}
I think we still need to do it in the constructor because there are places
calling Paint() and PaintContents().

[Xianzhu, 2017-05-10 22:55:33]

https://codereview.chromium.org/2872793002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FramePainter.cpp (right):

https://codereview.chromium.org/2872793002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FramePainter.cpp:16: #include
"core/paint/PaintTiming.h"
Remove.

https://codereview.chromium.org/2872793002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FramePainter.cpp:23: #include
"platform/graphics/paint/PaintController.h"
Remove.

https://codereview.chromium.org/2872793002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FramePainter.cpp:36:
frame_paint_timing_(context, &frame_view.GetFrame()) {}
On 2017/05/10 22:46:07, Zhen Wang wrote:
> I think we still need to do it in the constructor because there are places
> calling Paint() and PaintContents().

Paint() just calls PaintScrollbars() and PaintContents() to paint scrollbars and
contents, respectively. As we decided to skip frame scrollbars for first paint
notifications, we should notify from PaintContents().

[Zhen Wang, 2017-05-10 23:12:38]

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

[Zhen Wang, 2017-05-10 23:13:16]

https://codereview.chromium.org/2872793002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FramePainter.cpp (right):

https://codereview.chromium.org/2872793002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FramePainter.cpp:16: #include
"core/paint/PaintTiming.h"
On 2017/05/10 22:55:32, Xianzhu wrote:
> Remove.

Done.

https://codereview.chromium.org/2872793002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FramePainter.cpp:23: #include
"platform/graphics/paint/PaintController.h"
On 2017/05/10 22:55:32, Xianzhu wrote:
> Remove.

Done.

https://codereview.chromium.org/2872793002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FramePainter.cpp:36:
frame_paint_timing_(context, &frame_view.GetFrame()) {}
On 2017/05/10 22:55:32, Xianzhu wrote:
> On 2017/05/10 22:46:07, Zhen Wang wrote:
> > I think we still need to do it in the constructor because there are places
> > calling Paint() and PaintContents().
> 
> Paint() just calls PaintScrollbars() and PaintContents() to paint scrollbars
and
> contents, respectively. As we decided to skip frame scrollbars for first paint
> notifications, we should notify from PaintContents().

PaintContents() is also called by FrameView.cpp. So I add frame_paint_timing to
PaintContents() now.

[Xianzhu, 2017-05-10 23:22:47]

lgtm.

https://codereview.chromium.org/2872793002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FramePaintTiming.h (right):

https://codereview.chromium.org/2872793002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FramePaintTiming.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
Please list this file in Source/core/paint/BUILD.gn.
(See crbug.com/661744 for the reason.)

https://codereview.chromium.org/2872793002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FramePainter.h (left):

https://codereview.chromium.org/2872793002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FramePainter.h:24: explicit
FramePainter(const FrameView& frame_view)
Please revert these changes as we no longer require context in the constructor.

[Zhen Wang, 2017-05-11 20:25:46]

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

[commit-bot: I haz the power, 2017-05-11 20:26:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhen Wang, 2017-05-11 20:27:08]

https://codereview.chromium.org/2872793002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FramePaintTiming.h (right):

https://codereview.chromium.org/2872793002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FramePaintTiming.h:1: // Copyright 2016 The
Chromium Authors. All rights reserved.
On 2017/05/10 23:22:46, Xianzhu wrote:
> Please list this file in Source/core/paint/BUILD.gn.
> (See crbug.com/661744 for the reason.)

Done.

https://codereview.chromium.org/2872793002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/FramePainter.h (left):

https://codereview.chromium.org/2872793002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/FramePainter.h:24: explicit
FramePainter(const FrameView& frame_view)
On 2017/05/10 23:22:46, Xianzhu wrote:
> Please revert these changes as we no longer require context in the
constructor.

Done.

[commit-bot: I haz the power, 2017-05-11 23:05:26]

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

[commit-bot: I haz the power, 2017-05-11 23:05:28]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Zhen Wang, 2017-05-11 23:48:10]

Shubhie, this CL works. The problem I mentioned earlier is because the main
frame's perf observer is registered after FCP. Therefore, only child frame's FCP
is captured. If I force the script to run first (so to register observer first),
main frame FCP can also be captured.

[Zhen Wang, 2017-05-15 17:08:21]

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

[commit-bot: I haz the power, 2017-05-15 17:09:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[zhenw, 2017-05-15 17:12:57]

zhenw@google.com changed reviewers:
+ bmcquade@chromium.org

[zhenw, 2017-05-15 17:12:58]

+bmcquade for chrome/browser/page_load_metrics/

[Bryan McQuade, 2017-05-15 17:22:55]

thanks! just one request to remove a comment.

https://codereview.chromium.org/2872793002/diff/200001/chrome/test/data/page_...
File chrome/test/data/page_load_metrics/iframes.html (right):

https://codereview.chromium.org/2872793002/diff/200001/chrome/test/data/page_...
chrome/test/data/page_load_metrics/iframes.html:4: <!-- TODO(crbug/705315):
remove '/cross-site/?.com' when fixed. -->
can remove this line

[Zhen Wang, 2017-05-15 17:26:49]

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

[Zhen Wang, 2017-05-15 17:27:13]

https://codereview.chromium.org/2872793002/diff/200001/chrome/test/data/page_...
File chrome/test/data/page_load_metrics/iframes.html (right):

https://codereview.chromium.org/2872793002/diff/200001/chrome/test/data/page_...
chrome/test/data/page_load_metrics/iframes.html:4: <!-- TODO(crbug/705315):
remove '/cross-site/?.com' when fixed. -->
On 2017/05/15 17:22:55, Bryan McQuade wrote:
> can remove this line

Done.

[commit-bot: I haz the power, 2017-05-15 17:27:31]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[panicker, 2017-05-15 17:28:17]

LGTM

[Bryan McQuade, 2017-05-15 17:35:10]

lgtm for page_load_metrics_browsertest and related files, thanks!

[commit-bot: I haz the power, 2017-05-15 18:21:46]

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

[commit-bot: I haz the power, 2017-05-15 18:21:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Zhen Wang, 2017-05-15 19:38:47]

Chris, can you take a look at the platform part? Thanks!

[chrishtr, 2017-05-15 20:13:39]

lgtm

[Zhen Wang, 2017-05-15 20:17:46]

The CQ bit was checked by zhenw@chromium.org

[Zhen Wang, 2017-05-15 20:17:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/2872793002/#ps220001
(title: "remove one comment")

[commit-bot: I haz the power, 2017-05-15 20:18:48]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 22:19:53]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1494879466110790,
"parent_rev": "6031ec2baa3cb70e8105ff2d513a2635839b37ed", "commit_rev":
"5c0563522bdf7cb4c56420cd7d39c0139b5f8f1a"}

[commit-bot: I haz the power, 2017-05-15 22:20:13]

Description was changed from

==========
Notify paint for each frame

Currently main frame reports paint timing and subframes report paint
timing via main frame. The desired behavior is that each frame reports
its own paint timing.

BUG=705315
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Notify paint for each frame

Currently main frame reports paint timing and subframes report paint
timing via main frame. The desired behavior is that each frame reports
its own paint timing.

BUG=705315
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2872793002
Cr-Commit-Position: refs/heads/master@{#471922}
Committed:
https://chromium.googlesource.com/chromium/src/+/5c0563522bdf7cb4c56420cd7d39...
==========

[commit-bot: I haz the power, 2017-05-15 22:20:14]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/5c0563522bdf7cb4c56420cd7d39...

[kolos1, 2017-05-16 12:38:54]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2885773002/ by kolos@chromium.org.

The reason for reverting is: This CL might be a culprit of test flakiness.
https://bugs.chromium.org/p/chromium/issues/detail?id=722703#c2.

[Bryan McQuade, 2017-05-16 14:09:38]

On 2017/05/16 at 12:38:54, kolos wrote:
> A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2885773002/ by kolos@chromium.org.
> 
> The reason for reverting is: This CL might be a culprit of test flakiness.
https://bugs.chromium.org/p/chromium/issues/detail?id=722703#c2.

The revert should fix this issue.

There is some downstream page load metrics code that makes some assumptions that
no longer hold after this patch landed.

I'm going to land a change to fix those assumptions now.

Once that's landed, we should be able to re-land this patch.