Let FrameView track object paint invalidations

Let FrameView track object paint invalidations

With client-side invalidation flags, PaintController no longer need to
track invalidation of clients for painting purpose. We also don't need
to distinguish on which GraphicsLayer a client is invalidated because
when it's invalidated it will be invalid on all GraphicsLayers.

Let FrameView track object paint invalidations so that we don't need
to send object paint invalidations to backings.

BUG=617799
Committed: https://crrev.com/5cdeb1b846e2f1c8bcab391e0b0ea55faf5c5b12
Cr-Commit-Position: refs/heads/master@{#400605}

[Xianzhu, 2016-06-10 19:56:49]

Description was changed from

==========
Let FrameView track object paint invalidations

BUG=617799
==========

to

==========
Let FrameView track object paint invalidations

With client-side invalidation flags, PaintController no longer need to
track invalidation of clients for painting purpose. We also don't need
to distinguish on which GraphicsLayer a client is invalidated because
when it's invalidated it will be invalid on all GraphicsLayers.

Let FrameView track object paint invalidations so that we don't need
to send object paint invalidations to backings.

BUG=617799
==========

[Xianzhu, 2016-06-13 16:55:13]

wangxianzhu@chromium.org changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org

[chrishtr, 2016-06-13 20:18:52]

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1241: if (!frameView ||
!frameView->isTrackingPaintInvalidations()) {
Why is this conditional no longer needed?

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1309:
invalidateDisplayItemClients(*paintInvalidationContainer,
PaintInvalidationRectangle);
Why is it ok to skip invalidating the DisplayItemClients here?

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1381:
slowSetPaintingLayerNeedsRepaint();
Document why it's slow.

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (left):

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:596: // The caller
must check isTrackingOrCheckingPaintInvalidations() before calling this method
This method now gets called in more scenarios?

[Xianzhu, 2016-06-13 20:52:01]

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1241: if (!frameView ||
!frameView->isTrackingPaintInvalidations()) {
On 2016/06/13 20:18:52, chrishtr wrote:
> Why is this conditional no longer needed?

This was an optimization to avoid execution of line 1248 which is expensive when
the FrameView is not tracking paint invalidations.

displayItemClient.setDisplayItemsUncached() was always called, either here or in
LayoutBoxModelObject::invalidateDisplayItemClientOnBacking().

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:1309:
invalidateDisplayItemClients(*paintInvalidationContainer,
PaintInvalidationRectangle);
On 2016/06/13 20:18:52, chrishtr wrote:
> Why is it ok to skip invalidating the DisplayItemClients here?

The original condition was to handle unrooted subtree case. Now we don't care
whether the subtree is unrooted.

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1381:
slowSetPaintingLayerNeedsRepaint();
On 2016/06/13 20:18:52, chrishtr wrote:
> Document why it's slow.

Done.

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (left):

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:596: // The caller
must check isTrackingOrCheckingPaintInvalidations() before calling this method
On 2016/06/13 20:18:52, chrishtr wrote:
> This method now gets called in more scenarios?

In less scenarios. Now all the callers have rect ready, so checking
isTrackingOrCheckingPaintInvalidation() before calling trackPaintInvalidation()
won't save much cost.

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:725:
json->setString("name", debugName());
This will help us understand the relationship between the layers and the layout
objects in the tree dump.

[Xianzhu, 2016-06-16 19:06:14]

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (left):

https://codereview.chromium.org/2051333005/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:596: // The caller
must check isTrackingOrCheckingPaintInvalidations() before calling this method
On 2016/06/13 20:52:01, Xianzhu wrote:
> On 2016/06/13 20:18:52, chrishtr wrote:
> > This method now gets called in more scenarios?
> 
> In less scenarios.

I meant we track paint invalidations in less scenarios: now only for rect paint
invalidation here.

> Now all the callers have rect ready, so checking
> isTrackingOrCheckingPaintInvalidation() before calling
trackPaintInvalidation()
> won't save much cost.

[chrishtr, 2016-06-17 07:06:50]

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:385:
layoutObject->invalidatePaintRectangleNotInvalidatingDisplayItemClients(LayoutRect(paintInvalidationRect));
Why is it ok to change this to start invalidating display item clients?

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:326: bool
isTrackingPaintInvalidations() const { return
m_trackedObjectPaintInvalidations.get(); }
Add a note that these methods are for testing only.

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1390: void
invalidateDisplayItemClientWithPaintInvalidationState(const
PaintInvalidationState&, const DisplayItemClient&, PaintInvalidationReason)
const;
The naming here is inconsistent with
slowSetPaintingLayerNeedsRepaintAndInvalidateDisplayItemClient, because it
doesn't
mention setting needs repaint.

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1592: // This calls
paintingLayer() which walks the tree up.
"walks up the tree"

[Xianzhu, 2016-06-17 18:39:31]

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.cpp (left):

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.cpp:385:
layoutObject->invalidatePaintRectangleNotInvalidatingDisplayItemClients(LayoutRect(paintInvalidationRect));
On 2016/06/17 07:06:49, chrishtr wrote:
> Why is it ok to change this to start invalidating display item clients?

It also was OK before but was avoided because invalidating display item clients
was not super fast.

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/FrameView.h:326: bool
isTrackingPaintInvalidations() const { return
m_trackedObjectPaintInvalidations.get(); }
On 2016/06/17 07:06:49, chrishtr wrote:
> Add a note that these methods are for testing only.

Done.

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.h (right):

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1390: void
invalidateDisplayItemClientWithPaintInvalidationState(const
PaintInvalidationState&, const DisplayItemClient&, PaintInvalidationReason)
const;
On 2016/06/17 07:06:49, chrishtr wrote:
> The naming here is inconsistent with
> slowSetPaintingLayerNeedsRepaintAndInvalidateDisplayItemClient, because it
> doesn't
> mention setting needs repaint.

Renamed to setPaintingLayerNeedsRepaintAndInvalidateDisplayItemClient(), and
moved up to be beside the slow version.

https://codereview.chromium.org/2051333005/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.h:1592: // This calls
paintingLayer() which walks the tree up.
On 2016/06/17 07:06:49, chrishtr wrote:
> "walks up the tree"

Done.

[chrishtr, 2016-06-18 08:28:46]

lgtm

[Xianzhu, 2016-06-18 20:14:20]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-06-18 20:14:20]

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

[commit-bot: I haz the power, 2016-06-18 20:14:27]

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

[commit-bot: I haz the power, 2016-06-18 21:35:20]

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

[commit-bot: I haz the power, 2016-06-18 21:35:22]

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

[Xianzhu, 2016-06-18 22:00:54]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-06-18 22:00:55]

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

[commit-bot: I haz the power, 2016-06-18 22:01:26]

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

[commit-bot: I haz the power, 2016-06-18 23:19:04]

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

[commit-bot: I haz the power, 2016-06-18 23:19:05]

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

[Xianzhu, 2016-06-18 23:44:16]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-06-18 23:44:16]

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

[commit-bot: I haz the power, 2016-06-18 23:44:21]

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

[commit-bot: I haz the power, 2016-06-19 01:44:18]

Description was changed from

==========
Let FrameView track object paint invalidations

With client-side invalidation flags, PaintController no longer need to
track invalidation of clients for painting purpose. We also don't need
to distinguish on which GraphicsLayer a client is invalidated because
when it's invalidated it will be invalid on all GraphicsLayers.

Let FrameView track object paint invalidations so that we don't need
to send object paint invalidations to backings.

BUG=617799
==========

to

==========
Let FrameView track object paint invalidations

With client-side invalidation flags, PaintController no longer need to
track invalidation of clients for painting purpose. We also don't need
to distinguish on which GraphicsLayer a client is invalidated because
when it's invalidated it will be invalid on all GraphicsLayers.

Let FrameView track object paint invalidations so that we don't need
to send object paint invalidations to backings.

BUG=617799
==========

[commit-bot: I haz the power, 2016-06-19 01:44:20]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-06-19 01:45:32]

Description was changed from

==========
Let FrameView track object paint invalidations

With client-side invalidation flags, PaintController no longer need to
track invalidation of clients for painting purpose. We also don't need
to distinguish on which GraphicsLayer a client is invalidated because
when it's invalidated it will be invalid on all GraphicsLayers.

Let FrameView track object paint invalidations so that we don't need
to send object paint invalidations to backings.

BUG=617799
==========

to

==========
Let FrameView track object paint invalidations

With client-side invalidation flags, PaintController no longer need to
track invalidation of clients for painting purpose. We also don't need
to distinguish on which GraphicsLayer a client is invalidated because
when it's invalidated it will be invalid on all GraphicsLayers.

Let FrameView track object paint invalidations so that we don't need
to send object paint invalidations to backings.

BUG=617799

Committed: https://crrev.com/5cdeb1b846e2f1c8bcab391e0b0ea55faf5c5b12
Cr-Commit-Position: refs/heads/master@{#400605}
==========

[commit-bot: I haz the power, 2016-06-19 01:45:34]

Patchset 12 (id:??) landed as
https://crrev.com/5cdeb1b846e2f1c8bcab391e0b0ea55faf5c5b12
Cr-Commit-Position: refs/heads/master@{#400605}

[vasilii, 2016-06-20 08:41:08]

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

The reason for reverting is: Broke updating-scrolling-container-and-content on
all the platforms 

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/43368.

[Ken Rockot(use gerrit already), 2016-06-20 16:20:04]

On 2016/06/20 at 08:41:08, vasilii wrote:
> A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2080593002/ by vasilii@chromium.org.
> 
> The reason for reverting is: Broke updating-scrolling-container-and-content on
all the platforms 
> 
>
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/43368.

So just to clarify, this didn't get reverted after all. Instead the test was
disabled by https://codereview.chromium.org/2085503002

wangxianzhu@ could you please look into why your CL changed the test behavior?
It looks like the test was expected to crash but it no longer does.

Note that it did still crash on the CQ run (win_chromium_rel_ng) but not on any
of the waterfall bots. I assume this has something to do with the fact that
DCHECK isn't enabled on the waterfall bots.