Remove updatePaintingInfoForFragments() and per-fragment shouldPaintContent flag.

Remove updatePaintingInfoForFragments() and per-fragment shouldPaintContent flag.

It seems that all it did was to erroneously make us skip fragments. It
intersected with layer bounds, which don't include layer overflow. Before
we create the fragments in RenderMultiColumnSet, we carefully intersect
with everything relevant anyway. So rather than fixing the buggy code,
just remove it.

This means that there's no need for a per-fragment shouldPaintContent flag.
anymore.

BUG=359877
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184562

[mstensho (USE GERRIT), 2014-10-17 15:29:08]

mstensho@opera.com changed reviewers:
+ chrishtr@chromium.org

[chrishtr, 2014-10-17 19:05:59]

https://codereview.chromium.org/666493002/diff/1/Source/core/paint/LayerPaint...
File Source/core/paint/LayerPainter.cpp (left):

https://codereview.chromium.org/666493002/diff/1/Source/core/paint/LayerPaint...
Source/core/paint/LayerPainter.cpp:432: fragment.shouldPaintContent &=
m_renderLayer.intersectsDamageRect(fragment.layerBounds,
fragment.backgroundRect.rect(), localPaintingInfo.rootLayer,
&newOffsetFromRoot);
For the case of only one fragment (i.e. non-column code), is the early out for
intersecting the damage rect implemented elsewhere? Where?

[mstensho (USE GERRIT), 2014-10-17 20:06:50]

https://codereview.chromium.org/666493002/diff/1/Source/core/paint/LayerPaint...
File Source/core/paint/LayerPainter.cpp (left):

https://codereview.chromium.org/666493002/diff/1/Source/core/paint/LayerPaint...
Source/core/paint/LayerPainter.cpp:432: fragment.shouldPaintContent &=
m_renderLayer.intersectsDamageRect(fragment.layerBounds,
fragment.backgroundRect.rect(), localPaintingInfo.rootLayer,
&newOffsetFromRoot);
On 2014/10/17 19:05:59, chrishtr wrote:
> For the case of only one fragment (i.e. non-column code), is the early out for
> intersecting the damage rect implemented elsewhere? Where?

Oops, I was a bit quick there. I got confused by the unnecessary work done for
multicol, but you're absolutely right, of course. My change has unintended
consequences for non-multicol. No need to go all the way to the renderer,
although the renderer would return early as well if the intersection is empty.
But we would potentially waste a non-negligible abount of time before getting
there.

Anyway, the code is problematic (and generally pointless) for multicol, when a
column only contains overflow from the layer. Which is what the test
illustrates. I'll try to come up with something better.

[chrishtr, 2014-10-17 20:35:31]

Ok. An approach that works for 1 or more than 1 fragment would be great, but it
might be better to just specialize to the 1 fragment case for this optimization.
That's the critical part to keep fast. It's best to focus on correctness first
for multicolumn IMO.

[mstensho (USE GERRIT), 2014-10-17 21:03:07]

On 2014/10/17 20:35:31, chrishtr wrote:
> Ok. An approach that works for 1 or more than 1 fragment would be great, but
it
> might be better to just specialize to the 1 fragment case for this
optimization.
> That's the critical part to keep fast. It's best to focus on correctness first
> for multicolumn IMO.

We're on the same page here. :) When it comes to multicol, we need to be able to
walk before we can run. My main priority regarding performance while working on
the multicol implementation is that I don't introduce regressions for
non-multicol. The time spent in updatePaintingInfoForFragments() isn't that
critical in any way, but when what it does causes trouble, something has to be
done.

[mstensho (USE GERRIT), 2014-10-20 08:16:02]

https://codereview.chromium.org/666493002/diff/1/Source/core/paint/LayerPaint...
File Source/core/paint/LayerPainter.cpp (left):

https://codereview.chromium.org/666493002/diff/1/Source/core/paint/LayerPaint...
Source/core/paint/LayerPainter.cpp:432: fragment.shouldPaintContent &=
m_renderLayer.intersectsDamageRect(fragment.layerBounds,
fragment.backgroundRect.rect(), localPaintingInfo.rootLayer,
&newOffsetFromRoot);
On 2014/10/17 20:06:50, mstensho wrote:
> On 2014/10/17 19:05:59, chrishtr wrote:
> > For the case of only one fragment (i.e. non-column code), is the early out
for
> > intersecting the damage rect implemented elsewhere? Where?
> 
> Oops, I was a bit quick there. I got confused by the unnecessary work done for
> multicol, but you're absolutely right, of course. My change has unintended
> consequences for non-multicol. No need to go all the way to the renderer,
> although the renderer would return early as well if the intersection is empty.
> But we would potentially waste a non-negligible abount of time before getting
> there.
> 
> Anyway, the code is problematic (and generally pointless) for multicol, when a
> column only contains overflow from the layer. Which is what the test
> illustrates. I'll try to come up with something better.

Done. I had a hard time doing something pretty here, but let me know what you
think about what I did. Still beats all the "fragments.shouldPaintContent"
checks, I think?

[mstensho (USE GERRIT), 2014-10-22 16:36:46]

ping

[chrishtr, 2014-10-28 18:47:42]

https://codereview.chromium.org/666493002/diff/60001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (left):

https://codereview.chromium.org/666493002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:428: for (size_t i = 0; i < fragments.size();
++i) {
How about just a method:

bool LayerPainter::atLeastOneFragmentIsVisible(LayerFragments& fragments) {
  for (fragment: fragments) {
      if (&m_renderLayer == localPaintingInfo.rootLayer && !(localPaintFlags &
PaintLayerPaintingOverflowContents))
          return true;

      if (m_renderLayer.intersectsDamageRect(fragment.layerBounds,
fragment.backgroundRect.rect(), localPaintingInfo.rootLayer, &newOffsetFromRoot)
          return true;
  }
  return false;
}

Then plumb this bool into the various methods and replace
fragment.shouldPaintContent with the overall bool.

https://codereview.chromium.org/666493002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/666493002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1526: if (fragments.isEmpty()) {
When would there ever be 0 fragments?

https://codereview.chromium.org/666493002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1526: if (fragments.isEmpty()) {
When would there ever be 0 fragments? Will this part of the method only be
reachable for columns?

[chrishtr, 2014-10-28 18:47:50]

FYI sorry for the delay, was on vacation.

[mstensho (USE GERRIT), 2014-10-28 19:14:31]

On 2014/10/28 18:47:50, chrishtr wrote:
> FYI sorry for the delay, was on vacation.

No worries! I eventually realized that you might be on vacation, since you were
extremely quiet in the reviews last week. :)

Okay, let me get on with addressing your issues, then.

[mstensho (USE GERRIT), 2014-10-28 20:08:21]

https://codereview.chromium.org/666493002/diff/60001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (left):

https://codereview.chromium.org/666493002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:428: for (size_t i = 0; i < fragments.size();
++i) {
On 2014/10/28 18:47:42, chrishtr wrote:
> How about just a method:
> 
> bool LayerPainter::atLeastOneFragmentIsVisible(LayerFragments& fragments) {
>   for (fragment: fragments) {
>       if (&m_renderLayer == localPaintingInfo.rootLayer && !(localPaintFlags &
> PaintLayerPaintingOverflowContents))
>           return true;
> 
>       if (m_renderLayer.intersectsDamageRect(fragment.layerBounds,
> fragment.backgroundRect.rect(), localPaintingInfo.rootLayer,
&newOffsetFromRoot)
>           return true;
>   }
>   return false;
> }
> 
> Then plumb this bool into the various methods and replace
> fragment.shouldPaintContent with the overall bool.

You mean that you don't want the extra code (and parameter) that I added to
collectFragments()? The reason why I wanted to get rid of
updatePaintingInfoForFragments() was that fragment.layerBounds doesn't contain
overflow. It doesn't contain overflow because
RenderLayerClipper::calculateRects() (called from collectFragments()) sets
layerBounds to the border box, and doesn't care about overflow. While that does
look somewhat weird, since it's called "layerBounds" and all, it seems to be the
right thing to do. There are a lot of instances where we do
layerBounds.location() to find the location of the layer (e.g.
hitTestContentsForFragments() in RenderLayer and paintBackgroundForFragments()
and a bunch of other methods in LayerPainter), so including overflow there would
be wrong.

So we either need to add another member to LayerFragment, which expresses
overflow, or refrain from intersecting "layerBounds" with the damage rect.

https://codereview.chromium.org/666493002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/666493002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1526: if (fragments.isEmpty()) {
On 2014/10/28 18:47:42, chrishtr wrote:
> When would there ever be 0 fragments? Will this part of the method only be
> reachable for columns?

Yes, only reachable by columns. There'll be no fragments if none of the columns
intersect with the damage rect, for instance.

[chrishtr, 2014-10-28 20:18:25]

https://codereview.chromium.org/666493002/diff/60001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (left):

https://codereview.chromium.org/666493002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:428: for (size_t i = 0; i < fragments.size();
++i) {
On 2014/10/28 at 20:08:21, mstensho wrote:
> On 2014/10/28 18:47:42, chrishtr wrote:
> > How about just a method:
> > 
> > bool LayerPainter::atLeastOneFragmentIsVisible(LayerFragments& fragments) {
> >   for (fragment: fragments) {
> >       if (&m_renderLayer == localPaintingInfo.rootLayer && !(localPaintFlags
&
> > PaintLayerPaintingOverflowContents))
> >           return true;
> > 
> >       if (m_renderLayer.intersectsDamageRect(fragment.layerBounds,
> > fragment.backgroundRect.rect(), localPaintingInfo.rootLayer,
&newOffsetFromRoot)
> >           return true;
> >   }
> >   return false;
> > }
> > 
> > Then plumb this bool into the various methods and replace
> > fragment.shouldPaintContent with the overall bool.
> 
> You mean that you don't want the extra code (and parameter) that I added to
collectFragments()? The reason why I wanted to get rid of
updatePaintingInfoForFragments() was that fragment.layerBounds doesn't contain
overflow. It doesn't contain overflow because
RenderLayerClipper::calculateRects() (called from collectFragments()) sets
layerBounds to the border box, and doesn't care about overflow. While that does
look somewhat weird, since it's called "layerBounds" and all, it seems to be the
right thing to do. There are a lot of instances where we do
layerBounds.location() to find the location of the layer (e.g.
hitTestContentsForFragments() in RenderLayer and paintBackgroundForFragments()
and a bunch of other methods in LayerPainter), so including overflow there would
be wrong.
> 
> So we either need to add another member to LayerFragment, which expresses
overflow, or refrain from intersecting "layerBounds" with the damage rect.

Is this method hard to implement?


I was suggesting a way to reduce the size of this patch and simplify the code...
With the proposed method I think you can reduce this patch to calling
atLeastOneFragmentIsVisible instead of calling updatePaintingInfoForFragments,
plus plumbing the return value into the places were we need to consult it

I had forgotten about the overflow complication. However: it appears we can
modify the proposed LayerPainter::atLeastOneFragmentIsVisible to handle this, as
follows:

bool LayerPainter::atLeastOneFragmentIsVisible(LayerFragments& fragments) {
   // FIXME: implement correctly for columns
   if (there are columns)
     return true;

   for (fragment: fragments) {
       if (&m_renderLayer == localPaintingInfo.rootLayer && !(localPaintFlags &
PaintLayerPaintingOverflowContents))
           return true;
 
       if (m_renderLayer.intersectsDamageRect(fragment.layerBounds,
 fragment.backgroundRect.rect(), localPaintingInfo.rootLayer,
&newOffsetFromRoot)
           return true;
   }
   return false;
 }

[mstensho (USE GERRIT), 2014-10-28 21:33:20]

https://codereview.chromium.org/666493002/diff/60001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (left):

https://codereview.chromium.org/666493002/diff/60001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:428: for (size_t i = 0; i < fragments.size();
++i) {
On 2014/10/28 20:18:25, chrishtr wrote:
> On 2014/10/28 at 20:08:21, mstensho wrote:
> > On 2014/10/28 18:47:42, chrishtr wrote:
> > > How about just a method:
> > > 
> > > bool LayerPainter::atLeastOneFragmentIsVisible(LayerFragments& fragments)
{
> > >   for (fragment: fragments) {
> > >       if (&m_renderLayer == localPaintingInfo.rootLayer &&
!(localPaintFlags
> &
> > > PaintLayerPaintingOverflowContents))
> > >           return true;
> > > 
> > >       if (m_renderLayer.intersectsDamageRect(fragment.layerBounds,
> > > fragment.backgroundRect.rect(), localPaintingInfo.rootLayer,
> &newOffsetFromRoot)
> > >           return true;
> > >   }
> > >   return false;
> > > }
> > > 
> > > Then plumb this bool into the various methods and replace
> > > fragment.shouldPaintContent with the overall bool.
> > 
> > You mean that you don't want the extra code (and parameter) that I added to
> collectFragments()? The reason why I wanted to get rid of
> updatePaintingInfoForFragments() was that fragment.layerBounds doesn't contain
> overflow. It doesn't contain overflow because
> RenderLayerClipper::calculateRects() (called from collectFragments()) sets
> layerBounds to the border box, and doesn't care about overflow. While that
does
> look somewhat weird, since it's called "layerBounds" and all, it seems to be
the
> right thing to do. There are a lot of instances where we do
> layerBounds.location() to find the location of the layer (e.g.
> hitTestContentsForFragments() in RenderLayer and paintBackgroundForFragments()
> and a bunch of other methods in LayerPainter), so including overflow there
would
> be wrong.
> > 
> > So we either need to add another member to LayerFragment, which expresses
> overflow, or refrain from intersecting "layerBounds" with the damage rect.
> 
> Is this method hard to implement?
> 
> 
> I was suggesting a way to reduce the size of this patch and simplify the
code...
> With the proposed method I think you can reduce this patch to calling
> atLeastOneFragmentIsVisible instead of calling updatePaintingInfoForFragments,
> plus plumbing the return value into the places were we need to consult it
> 
> I had forgotten about the overflow complication. However: it appears we can
> modify the proposed LayerPainter::atLeastOneFragmentIsVisible to handle this,
as
> follows:
> 
> bool LayerPainter::atLeastOneFragmentIsVisible(LayerFragments& fragments) {
>    // FIXME: implement correctly for columns
>    if (there are columns)
>      return true;
> 
>    for (fragment: fragments) {
>        if (&m_renderLayer == localPaintingInfo.rootLayer && !(localPaintFlags
&
> PaintLayerPaintingOverflowContents))
>            return true;
>  
>        if (m_renderLayer.intersectsDamageRect(fragment.layerBounds,
>  fragment.backgroundRect.rect(), localPaintingInfo.rootLayer,
> &newOffsetFromRoot)
>            return true;
>    }
>    return false;
>  }
> 
> 
> 

Done. It's kind of meaningless to loop through the fragments for non-multicol
only, since there'll always only be one fragment then. But this is a messy
neighborhood, and there's really no clean solution that doesn't involve a lot of
refactoring.

[chrishtr, 2014-10-28 21:44:46]

https://codereview.chromium.org/666493002/diff/80001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/666493002/diff/80001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:427: return true; // The fragments created
have already been found to intersect with the damage rect.
Is this comment accurate? Isn't it more like we are bailing in that scenario?

https://codereview.chromium.org/666493002/diff/80001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:434: if
(m_renderLayer.intersectsDamageRect(fragment.layerBounds,
fragment.backgroundRect.rect(), localPaintingInfo.rootLayer,
&newOffsetFromRoot))
Add a note that this works only for the main fragment due to the issue you
encountered.

[mstensho (USE GERRIT), 2014-10-28 22:56:49]

https://codereview.chromium.org/666493002/diff/80001/Source/core/paint/LayerP...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/666493002/diff/80001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:427: return true; // The fragments created
have already been found to intersect with the damage rect.
On 2014/10/28 21:44:46, chrishtr wrote:
> Is this comment accurate? Isn't it more like we are bailing in that scenario?

Yes, it should be accurate. RenderMultiColumnSet::collectLayerFragments()
intersects with everything interesting, and it doesn't bother to create a
fragment for a column that doesn't intersect.

https://codereview.chromium.org/666493002/diff/80001/Source/core/paint/LayerP...
Source/core/paint/LayerPainter.cpp:434: if
(m_renderLayer.intersectsDamageRect(fragment.layerBounds,
fragment.backgroundRect.rect(), localPaintingInfo.rootLayer,
&newOffsetFromRoot))
On 2014/10/28 21:44:46, chrishtr wrote:
> Add a note that this works only for the main fragment due to the issue you
> encountered.
> 

Done.

[chrishtr, 2014-10-28 23:47:21]

lgtm

[mstensho (USE GERRIT), 2014-10-29 05:39:51]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-10-29 05:40:10]

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

[commit-bot: I haz the power, 2014-10-29 05:43:45]

Committed patchset #6 (id:100001) as 184562