When performing a fast-path scroll, issue repaints for viewport-constrained layers

When performing a fast-path scroll, issue repaints for viewport-constrained layers
relative to their repaint container, not their enclosing composited ancestor.

The latter is wrong for squashing.
BUG=370664
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175534

[Ian Vollick, 2014-06-04 20:02:06]

On 2014/06/04 19:57:29, chrishtr wrote:

That totally makes sense. lgtm.

[ojan, 2014-06-04 20:07:28]

lgtm as well.

https://codereview.chromium.org/318793002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/318793002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1423: if (repaintContainer &&
!repaintContainer->isRenderView()) {
Can we really get a null repaintContainer here? It looks to me that
containerForRepaint will only return null if the renderer isn't rooted. But if
it's not rooted I would expect we can't get to this code.

I'm not 100% sure. But, I'd be happier with an ASSERT(!repaintContainer) if it
passes the tests.

[chrishtr, 2014-06-04 20:11:41]

https://codereview.chromium.org/318793002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/318793002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1423: if (repaintContainer &&
!repaintContainer->isRenderView()) {
On 2014/06/04 20:07:29, ojan wrote:
> Can we really get a null repaintContainer here? It looks to me that
> containerForRepaint will only return null if the renderer isn't rooted. But if
> it's not rooted I would expect we can't get to this code.
> 
> I'm not 100% sure. But, I'd be happier with an ASSERT(!repaintContainer) if it
> passes the tests.

Yes it can happen. crbug.com/380147

[chrishtr, 2014-06-04 20:11:52]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-06-04 20:12:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishtr@chromium.org/318793002/40001

[leviw_travelin_and_unemployed, 2014-06-04 20:37:24]

https://codereview.chromium.org/318793002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/318793002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1423: if (repaintContainer &&
!repaintContainer->isRenderView()) {
On 2014/06/04 20:11:41, chrishtr wrote:
> On 2014/06/04 20:07:29, ojan wrote:
> > Can we really get a null repaintContainer here? It looks to me that
> > containerForRepaint will only return null if the renderer isn't rooted. But
if
> > it's not rooted I would expect we can't get to this code.
> > 
> > I'm not 100% sure. But, I'd be happier with an ASSERT(!repaintContainer) if
it
> > passes the tests.
> 
> Yes it can happen. crbug.com/380147

Do you have a test case? We shouldn't be getting a null repaint value here...

[commit-bot: I haz the power, 2014-06-04 21:07:47]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6949)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[chrishtr, 2014-06-04 21:18:56]

https://codereview.chromium.org/318793002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/318793002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1423: if (repaintContainer &&
!repaintContainer->isRenderView()) {
On 2014/06/04 20:37:25, leviw wrote:
> On 2014/06/04 20:11:41, chrishtr wrote:
> > On 2014/06/04 20:07:29, ojan wrote:
> > > Can we really get a null repaintContainer here? It looks to me that
> > > containerForRepaint will only return null if the renderer isn't rooted.
But
> if
> > > it's not rooted I would expect we can't get to this code.
> > > 
> > > I'm not 100% sure. But, I'd be happier with an ASSERT(!repaintContainer)
if
> it
> > > passes the tests.
> > 
> > Yes it can happen. crbug.com/380147
> 
> Do you have a test case? We shouldn't be getting a null repaint value here...

compositing/reflections/nested-reflection-transformed.html

It happens while building the DOM tree:

<null>
 0x7fafae82f77e WebCore::RenderObject::boundsRectForRepaint()
#4 0x7fafae7f99e3 WebCore::RenderLayerRepainter::computeRepaintRects()
#5 0x7fafae7f97a2 WebCore::RenderLayerRepainter::repaintAfterLayout()
#6 0x7fafae7d6f0b WebCore::RenderLayer::updateLayerPositions()
#7 0x7fafae7f8258 WebCore::RenderLayerModelObject::styleDidChange()
#8 0x7fafae73fddc WebCore::RenderBox::styleDidChange()
#9 0x7fafae832f99 WebCore::RenderObject::setStyle()
#10 0x7fafae7f9445 WebCore::RenderLayerReflectionInfo::updateAfterStyleChange()
#11 0x7fafae7dc712 WebCore::RenderLayer::updateReflectionInfo()
#12 0x7fafae7e75be WebCore::RenderLayer::styleChanged()
#13 0x7fafae7f8378 WebCore::RenderLayerModelObject::styleDidChange()
#14 0x7fafae73fddc WebCore::RenderBox::styleDidChange()
#15 0x7fafae6da956 WebCore::RenderBlock::styleDidChange()
#16 0x7fafae71ca16 WebCore::RenderBlockFlow::styleDidChange()
#17 0x7fafae832f99 WebCore::RenderObject::setStyle()
#18 0x7fafadcb04c1
WebCore::RenderTreeBuilder::createRendererForElementIfNeeded()
#19 0x7fafadc36a4f WebCore::Element::attach()
#20 0x7fafadbb90d3 WebCore::ContainerNode::attachChildren()
#21 0x7fafadbb531f WebCore::ContainerNode::attach()
#22 0x7fafadc36ab7 WebCore::Element::attach()

[commit-bot: I haz the power, 2014-06-04 21:31:00]

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

[commit-bot: I haz the power, 2014-06-04 21:31:00]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6953)

[chrishtr, 2014-06-04 21:34:16]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-06-04 21:35:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishtr@chromium.org/318793002/40001

[commit-bot: I haz the power, 2014-06-04 22:08:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6955)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[commit-bot: I haz the power, 2014-06-04 22:11:19]

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

[commit-bot: I haz the power, 2014-06-04 22:11:20]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6956)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[chrishtr, 2014-06-04 23:05:19]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-06-04 23:05:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishtr@chromium.org/318793002/40001

[commit-bot: I haz the power, 2014-06-04 23:32:08]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6958)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[commit-bot: I haz the power, 2014-06-04 23:35:10]

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

[commit-bot: I haz the power, 2014-06-04 23:35:11]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6960)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[chrishtr, 2014-06-05 03:02:00]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-06-05 03:02:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishtr@chromium.org/318793002/40001

[commit-bot: I haz the power, 2014-06-05 03:15:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6971)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[commit-bot: I haz the power, 2014-06-05 03:17:56]

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

[commit-bot: I haz the power, 2014-06-05 03:17:57]

Try jobs failed on following builders:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[chrishtr, 2014-06-05 03:58:50]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-06-05 03:59:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishtr@chromium.org/318793002/40001

[commit-bot: I haz the power, 2014-06-05 04:01:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6980)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[commit-bot: I haz the power, 2014-06-05 04:03:26]

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

[commit-bot: I haz the power, 2014-06-05 04:03:27]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6981)

[chrishtr, 2014-06-05 04:12:12]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-06-05 04:12:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishtr@chromium.org/318793002/50001

[commit-bot: I haz the power, 2014-06-05 05:16:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/6984)

[commit-bot: I haz the power, 2014-06-05 05:29:39]

Change committed as 175534

[ojan, 2014-06-07 17:35:23]

https://codereview.chromium.org/318793002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/318793002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1423: if (repaintContainer &&
!repaintContainer->isRenderView()) {
I don't see scrollContentsFastPath on that stack. I know that we can get null
repaintContainers in general (i.e. for renderers that are not rooted), but what
I'm asking is if we can get them in scrollContentsFastPath.

[chrishtr, 2014-06-09 16:40:17]

https://codereview.chromium.org/318793002/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/318793002/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:1423: if (repaintContainer &&
!repaintContainer->isRenderView()) {
On 2014/06/07 17:35:23, ojan wrote:
> I don't see scrollContentsFastPath on that stack. I know that we can get null
> repaintContainers in general (i.e. for renderers that are not rooted), but
what
> I'm asking is if we can get them in scrollContentsFastPath.

Ok I see, I misinterpreted your question. It does look impossible. I'll add the
assert.