Fix first paint tracking for SPv2

Fix first paint tracking for SPv2

Now move first paint notification code from GraphicsLayer
CompositedLayerMapping into FrameView.

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

Review-Url: https://codereview.chromium.org/2623043002
Cr-Commit-Position: refs/heads/master@{#443503}
Committed: https://chromium.googlesource.com/chromium/src/+/0562259b2ac8d61de08dfbed6b13b9050bda9c0a

[Xianzhu, 2017-01-10 23:21:40]

Description was changed from

==========
Fix first paint tracking for SPv2

Now move first paint notification code from GraphicsLayer
CompositedLayerMapping into FrameView.

BUG=657825
==========

to

==========
Fix first paint tracking for SPv2

Now move first paint notification code from GraphicsLayer
CompositedLayerMapping into FrameView.

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

[Xianzhu, 2017-01-10 23:29:01]

wangxianzhu@chromium.org changed reviewers:
+ panicker@chromium.org, pdr@chromium.org, sunjian@chromium.org

[Xianzhu, 2017-01-10 23:29:02]

https://codereview.chromium.org/2623043002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/ViewPainter.cpp (left):

https://codereview.chromium.org/2623043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/ViewPainter.cpp:173:
TRACE_EVENT_SCOPE_GLOBAL);
@panicker: I'm not sure if the above user_timing event is used and how it's
used. If we do need it, I can add back
PaintController::(setN|n)onDefaultBackgroundColorPainted(). Or do we just need
the event of first-painted?

[panicker, 2017-01-11 00:12:08]

https://codereview.chromium.org/2623043002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/ViewPainter.cpp (left):

https://codereview.chromium.org/2623043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/ViewPainter.cpp:173:
TRACE_EVENT_SCOPE_GLOBAL);
On 2017/01/10 23:29:01, Xianzhu wrote:
> @panicker: I'm not sure if the above user_timing event is used and how it's
> used. If we do need it, I can add back
> PaintController::(setN|n)onDefaultBackgroundColorPainted(). Or do we just need
> the event of first-painted?

It's fine to remove that trace event (was primarily used by me).

[panicker, 2017-01-11 00:17:25]

https://codereview.chromium.org/2623043002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/ViewPainter.cpp (left):

https://codereview.chromium.org/2623043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/ViewPainter.cpp:173:
TRACE_EVENT_SCOPE_GLOBAL);
On 2017/01/11 00:12:07, panicker wrote:
> On 2017/01/10 23:29:01, Xianzhu wrote:
> > @panicker: I'm not sure if the above user_timing event is used and how it's
> > used. If we do need it, I can add back
> > PaintController::(setN|n)onDefaultBackgroundColorPainted(). Or do we just
need
> > the event of first-painted?
> 
> It's fine to remove that trace event (was primarily used by me).

Sorry I misunderstood your question I think, we definitely want to keep the
logic that first-paint should only fire non-default background has painted (for
intance it should not fire on paint of white / default browser background
color).
So yes please keep the check for 
!context.getPaintController().nonDefaultBackgroundColorPainted()

[Xianzhu, 2017-01-11 00:23:41]

https://codereview.chromium.org/2623043002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/ViewPainter.cpp (left):

https://codereview.chromium.org/2623043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/ViewPainter.cpp:173:
TRACE_EVENT_SCOPE_GLOBAL);
On 2017/01/11 00:17:25, panicker wrote:
> On 2017/01/11 00:12:07, panicker wrote:
> > On 2017/01/10 23:29:01, Xianzhu wrote:
> > > @panicker: I'm not sure if the above user_timing event is used and how
it's
> > > used. If we do need it, I can add back
> > > PaintController::(setN|n)onDefaultBackgroundColorPainted(). Or do we just
> need
> > > the event of first-painted?
> > 
> > It's fine to remove that trace event (was primarily used by me).
> 
> Sorry I misunderstood your question I think, we definitely want to keep the
> logic that first-paint should only fire non-default background has painted
(for
> intance it should not fire on paint of white / default browser background
> color).
> So yes please keep the check for 
> !context.getPaintController().nonDefaultBackgroundColorPainted()

I think this change doesn't break that. The new code here will set firstPainted
to true when a non-default background is painted.
PaintController::processNewItem() will also set firstPainted to true when a
non-viewport-background is painted. They combined replace the original logic
here and the logic in GraphicsLayer checking the first paint.

[panicker, 2017-01-11 21:13:34]

Looks good overall, and great clean up!
Would be great to check couple of web pages with & without your CL to sanity
check that FP / FCP remain the same.
Thanks!

https://codereview.chromium.org/2623043002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/ViewPainter.cpp (left):

https://codereview.chromium.org/2623043002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/ViewPainter.cpp:173:
TRACE_EVENT_SCOPE_GLOBAL);
On 2017/01/11 00:23:41, Xianzhu wrote:
> On 2017/01/11 00:17:25, panicker wrote:
> > On 2017/01/11 00:12:07, panicker wrote:
> > > On 2017/01/10 23:29:01, Xianzhu wrote:
> > > > @panicker: I'm not sure if the above user_timing event is used and how
> it's
> > > > used. If we do need it, I can add back
> > > > PaintController::(setN|n)onDefaultBackgroundColorPainted(). Or do we
just
> > need
> > > > the event of first-painted?
> > > 
> > > It's fine to remove that trace event (was primarily used by me).
> > 
> > Sorry I misunderstood your question I think, we definitely want to keep the
> > logic that first-paint should only fire non-default background has painted
> (for
> > intance it should not fire on paint of white / default browser background
> > color).
> > So yes please keep the check for 
> > !context.getPaintController().nonDefaultBackgroundColorPainted()
> 
> I think this change doesn't break that. The new code here will set
firstPainted
> to true when a non-default background is painted.
> PaintController::processNewItem() will also set firstPainted to true when a
> non-viewport-background is painted. They combined replace the original logic
> here and the logic in GraphicsLayer checking the first paint.

Ok makes sense, thanks.

[panicker, 2017-01-11 21:15:51]

On 2017/01/11 21:13:34, panicker wrote:
> Looks good overall, and great clean up!
> Would be great to check couple of web pages with & without your CL to sanity
> check that FP / FCP remain the same.
This is straightforward to do with taking a trace, select the category
"blink.user_timing"
> Thanks!
> 
>
https://codereview.chromium.org/2623043002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/ViewPainter.cpp (left):
> 
>
https://codereview.chromium.org/2623043002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/ViewPainter.cpp:173:
> TRACE_EVENT_SCOPE_GLOBAL);
> On 2017/01/11 00:23:41, Xianzhu wrote:
> > On 2017/01/11 00:17:25, panicker wrote:
> > > On 2017/01/11 00:12:07, panicker wrote:
> > > > On 2017/01/10 23:29:01, Xianzhu wrote:
> > > > > @panicker: I'm not sure if the above user_timing event is used and how
> > it's
> > > > > used. If we do need it, I can add back
> > > > > PaintController::(setN|n)onDefaultBackgroundColorPainted(). Or do we
> just
> > > need
> > > > > the event of first-painted?
> > > > 
> > > > It's fine to remove that trace event (was primarily used by me).
> > > 
> > > Sorry I misunderstood your question I think, we definitely want to keep
the
> > > logic that first-paint should only fire non-default background has painted
> > (for
> > > intance it should not fire on paint of white / default browser background
> > > color).
> > > So yes please keep the check for 
> > > !context.getPaintController().nonDefaultBackgroundColorPainted()
> > 
> > I think this change doesn't break that. The new code here will set
> firstPainted
> > to true when a non-default background is painted.
> > PaintController::processNewItem() will also set firstPainted to true when a
> > non-viewport-background is painted. They combined replace the original logic
> > here and the logic in GraphicsLayer checking the first paint.
> 
> Ok makes sense, thanks.

[Xianzhu, 2017-01-11 22:19:05]

On 2017/01/11 21:15:51, panicker wrote:
> On 2017/01/11 21:13:34, panicker wrote:
> > Looks good overall, and great clean up!
> > Would be great to check couple of web pages with & without your CL to sanity
> > check that FP / FCP remain the same.
> This is straightforward to do with taking a trace, select the category
> "blink.user_timing"

Done. The results looked good.

Also tried https://codereview.chromium.org/2528513003/ with this patch, and all
fast/performance tests passed with --enable-slimming-paint-v2.

[Xianzhu, 2017-01-12 04:19:57]

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

[commit-bot: I haz the power, 2017-01-12 04:20:17]

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-01-12 07:02:27]

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

[commit-bot: I haz the power, 2017-01-12 07:02:28]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2017-01-12 21:43:07]

pdr@ can you review this CL as an owner?

[pdr., 2017-01-12 21:56:45]

https://codereview.chromium.org/2623043002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2623043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3008: if (Document* document
= m_frame->document()) {
Would it be valid to add DCHECK(m_frame->document()) above this check?

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

https://codereview.chromium.org/2623043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:176: if
(!m_firstPainted && displayItem.isDrawing() &&
processNewItem is pretty hot so I'm worried about adding a branch here. Could we
put this check in commitNewDisplayItems instead, which is called less
frequently? Doing this would require iterating through the first few items to
find a painted one.

[Xianzhu, 2017-01-12 22:13:39]

https://codereview.chromium.org/2623043002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/2623043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.cpp:3008: if (Document* document
= m_frame->document()) {
On 2017/01/12 21:56:45, pdr. wrote:
> Would it be valid to add DCHECK(m_frame->document()) above this check?

Yes. We should have document if we have painted.

Done.

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

https://codereview.chromium.org/2623043002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:176: if
(!m_firstPainted && displayItem.isDrawing() &&
On 2017/01/12 21:56:45, pdr. wrote:
> processNewItem is pretty hot so I'm worried about adding a branch here. Could
we
> put this check in commitNewDisplayItems instead, which is called less
> frequently? Doing this would require iterating through the first few items to
> find a painted one.

Good idea. Done.

[pdr., 2017-01-12 22:21:55]

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

https://codereview.chromium.org/2623043002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:529: if
(UNLIKELY(!m_firstPainted) && item.isDrawing() &&
This will still be called for all new and cached items. WDYT about pulling this
out of the loop? Something like this on line 505:
if (!m_firstPainted) {
  for (const auto& item : m_newDisplayItemList) {
    if (item.isDrawing() && ...) {
      m_firstPainted = true;
      break;
    }
  }
}

[Xianzhu, 2017-01-12 22:32:22]

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

https://codereview.chromium.org/2623043002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:529: if
(UNLIKELY(!m_firstPainted) && item.isDrawing() &&
On 2017/01/12 22:21:55, pdr. wrote:
> This will still be called for all new and cached items. WDYT about pulling
this
> out of the loop? Something like this on line 505:
> if (!m_firstPainted) {
>   for (const auto& item : m_newDisplayItemList) {
>     if (item.isDrawing() && ...) {
>       m_firstPainted = true;
>       break;
>     }
>   }
> }

Done.

Considered this but thought the cost of 'if (UNLIKELY(!m_firstPainted)...)' in
the loop was comparable to that of an iteration of a separate loop. Overlooked
the fact that we won't execute the loop at all if m_firstPaint is true.

[pdr., 2017-01-12 22:35:03]

On 2017/01/12 at 22:32:22, wangxianzhu wrote:
>
https://codereview.chromium.org/2623043002/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
(right):
> 
>
https://codereview.chromium.org/2623043002/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:529: if
(UNLIKELY(!m_firstPainted) && item.isDrawing() &&
> On 2017/01/12 22:21:55, pdr. wrote:
> > This will still be called for all new and cached items. WDYT about pulling
this
> > out of the loop? Something like this on line 505:
> > if (!m_firstPainted) {
> >   for (const auto& item : m_newDisplayItemList) {
> >     if (item.isDrawing() && ...) {
> >       m_firstPainted = true;
> >       break;
> >     }
> >   }
> > }
> 
> Done.
> 
> Considered this but thought the cost of 'if (UNLIKELY(!m_firstPainted)...)' in
the loop was comparable to that of an iteration of a separate loop. Overlooked
the fact that we won't execute the loop at all if m_firstPaint is true.

LGTM!

[Xianzhu, 2017-01-12 22:37:20]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-01-12 22:37:53]

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-01-13 04:44:11]

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

[commit-bot: I haz the power, 2017-01-13 04:44:13]

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_...)

[Xianzhu, 2017-01-13 04:45:04]

The CQ bit was checked by wangxianzhu@chromium.org

[commit-bot: I haz the power, 2017-01-13 04:45:33]

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-01-13 06:08:26]

CQ is committing da patch.

Bot data: {"patchset_id": 40002, "attempt_start_ts": 1484282704486220,
"parent_rev": "5739add10c75198926cea1a66772ab04df47ad51", "commit_rev":
"0562259b2ac8d61de08dfbed6b13b9050bda9c0a"}

[commit-bot: I haz the power, 2017-01-13 06:09:55]

Description was changed from

==========
Fix first paint tracking for SPv2

Now move first paint notification code from GraphicsLayer
CompositedLayerMapping into FrameView.

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

to

==========
Fix first paint tracking for SPv2

Now move first paint notification code from GraphicsLayer
CompositedLayerMapping into FrameView.

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

Review-Url: https://codereview.chromium.org/2623043002
Cr-Commit-Position: refs/heads/master@{#443503}
Committed:
https://chromium.googlesource.com/chromium/src/+/0562259b2ac8d61de08dfbed6b13...
==========

[commit-bot: I haz the power, 2017-01-13 06:09:57]

Committed patchset #4 (id:40002) as
https://chromium.googlesource.com/chromium/src/+/0562259b2ac8d61de08dfbed6b13...