Update should paint flag when a float's composited scrolling status changes

Update should paint flag when a float's composited scrolling status changes

Normally floating objects' should paint flags are updated during layout.
However, it also depends on self-painting status which can be changed
during compositing update (after layout) when composited scrolling
status changes.

Update should paint flag when floating object's composited scrolling
status changes.

No need to handle change of non-floating objects' composited scrolling
status affecting should paint flags of contained floating objects,
because should paint flags don't cross overflow clipping boundaries.

Also no need to handle the cases that have been handled by layout.

BUG=420072
TEST=fast/block/float/float-change-composited-scrolling.html

Committed: https://crrev.com/871b186bc143ef3082dfb80f1437c2250cca06f3
Cr-Commit-Position: refs/heads/master@{#373198}

[Xianzhu, 2016-02-02 18:03:52]

Description was changed from

==========
Update should paint flag when a float's composited scrolling status changes

Normally floating objects' should paint flags are updated during layout.
However, it also depends on self-painting status which can be changed
during compositing update (after layout) when composited scrolling
status changes.

Update should paint flag when floating object's composited scrolling
status changes.

No need to handle change of non-floating objects' composited scrolling
status affecting should paint flags of contained floating objects,
because should paint flags don't cross overflow clipping boundaries.

BUG=420072
TEST=fast/block/float/float-change-composited-scrolling.html
==========

to

==========
Update should paint flag when a float's composited scrolling status changes

Normally floating objects' should paint flags are updated during layout.
However, it also depends on self-painting status which can be changed
during compositing update (after layout) when composited scrolling
status changes.

Update should paint flag when floating object's composited scrolling
status changes.

No need to handle change of non-floating objects' composited scrolling
status affecting should paint flags of contained floating objects,
because should paint flags don't cross overflow clipping boundaries.

Also no need to handle the cases that have been handled by layout.

BUG=420072
TEST=fast/block/float/float-change-composited-scrolling.html
==========

[Xianzhu, 2016-02-02 18:04:09]

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

[chrishtr, 2016-02-02 19:32:05]

https://codereview.chromium.org/1661523002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1661523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:2646: if
(ancestorBlock->hasSelfPaintingLayer() ||
!ancestorBlock->isOverhangingFloat(floatingObject) ||
ancestorBlock->createsNewFormattingContext()) {
Why is this the right set of conditions? Does this occur elsewhere? (Share
code?)

[Xianzhu, 2016-02-02 20:19:08]

https://codereview.chromium.org/1661523002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1661523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:2646: if
(ancestorBlock->hasSelfPaintingLayer() ||
!ancestorBlock->isOverhangingFloat(floatingObject) ||
ancestorBlock->createsNewFormattingContext()) {
On 2016/02/02 19:32:04, chrishtr wrote:
> Why is this the right set of conditions? Does this occur elsewhere? (Share
> code?)

The conditions are scattered in this file, mainly in layoutBlockFlow(),
addLowestFloatFromChildren(), addOverhaningFloats(), etc. Different places use
different combinations of the conditions because of their different contexts in
which some conditions are already checked or not applicable.

Basically the rules of float painting are:
- Let the nearest enclosing block in which the float doesn't overhang paint the
float;
- Or even if the float overhangs, if the block has self-painting layer, the
block paints the float.

Checked the conditions again and found that createNewFormattingContext() is not
necessary because a float never overhangs a block creating new formatting
context (the block's height has been adjusted to include full height of the
float).

trchen@ discussed with me about float painting. His idea is to allocate
self-painting layers for floats, so that they always paint themselves and we can
avoid the complex logics dealing with shouldPaint flags. His method can also
avoid wrong paint-ordering of overlapping floats because of negative margins.
This method seems to also apply to all other pseudo stacking contexts. I think
it is worth trying the method in the future.

[chrishtr, 2016-02-02 20:46:32]

On 2016/02/02 at 20:19:08, wangxianzhu wrote:
>
https://codereview.chromium.org/1661523002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):
> 
>
https://codereview.chromium.org/1661523002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:2646: if
(ancestorBlock->hasSelfPaintingLayer() ||
!ancestorBlock->isOverhangingFloat(floatingObject) ||
ancestorBlock->createsNewFormattingContext()) {
> On 2016/02/02 19:32:04, chrishtr wrote:
> > Why is this the right set of conditions? Does this occur elsewhere? (Share
> > code?)
> 
> The conditions are scattered in this file, mainly in layoutBlockFlow(),
addLowestFloatFromChildren(), addOverhaningFloats(), etc. Different places use
different combinations of the conditions because of their different contexts in
which some conditions are already checked or not applicable.
> 
> Basically the rules of float painting are:
> - Let the nearest enclosing block in which the float doesn't overhang paint
the float;
> - Or even if the float overhangs, if the block has self-painting layer, the
block paints the float.
> 
> Checked the conditions again and found that createNewFormattingContext() is
not necessary because a float never overhangs a block creating new formatting
context (the block's height has been adjusted to include full height of the
float).
> 
> trchen@ discussed with me about float painting. His idea is to allocate
self-painting layers for floats, so that they always paint themselves and we can
avoid the complex logics dealing with shouldPaint flags. His method can also
avoid wrong paint-ordering of overlapping floats because of negative margins.
This method seems to also apply to all other pseudo stacking contexts. I think
it is worth trying the method in the future.

Ok file a bug for that?

[Xianzhu, 2016-02-02 20:49:09]

On 2016/02/02 20:46:32, chrishtr wrote:
> On 2016/02/02 at 20:19:08, wangxianzhu wrote:
> > trchen@ discussed with me about float painting. His idea is to allocate
> self-painting layers for floats, so that they always paint themselves and we
can
> avoid the complex logics dealing with shouldPaint flags. His method can also
> avoid wrong paint-ordering of overlapping floats because of negative margins.
> This method seems to also apply to all other pseudo stacking contexts. I think
> it is worth trying the method in the future.
> 
> Ok file a bug for that?

http://crbug.com/583428.

[chrishtr, 2016-02-02 20:49:40]

https://codereview.chromium.org/1661523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1661523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:2647: if
(ancestorBlock->hasSelfPaintingLayer() ||
!ancestorBlock->isOverhangingFloat(floatingObject)) {
Please paste your explanatory comment here:

The rules of float painting are:
- Let the nearest enclosing block in which the float doesn't overhang paint the
float;
- Or even if the float overhangs, if the block has self-painting layer, the
block paints the float.

[wkorman, 2016-02-02 22:02:34]

wkorman@chromium.org changed reviewers:
+ wkorman@chromium.org

[wkorman, 2016-02-02 22:02:35]

https://codereview.chromium.org/1661523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/block/float/float-change-composited-scrolling.html
(right):

https://codereview.chromium.org/1661523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/block/float/float-change-composited-scrolling.html:17:
if (window.testRunner)
I think you can just pass true as a second param to this call to
runAfterLayoutAndPaint() and omit this and next two lines.

https://codereview.chromium.org/1661523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/FloatingObjects.h (right):

https://codereview.chromium.org/1661523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/FloatingObjects.h:154: FloatingObjectSet&
set() { return m_set; }
Perhaps name this mutableSet() to more explicitly differentiate from the other?

[Xianzhu, 2016-02-02 22:47:12]

https://codereview.chromium.org/1661523002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/block/float/float-change-composited-scrolling.html
(right):

https://codereview.chromium.org/1661523002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/block/float/float-change-composited-scrolling.html:17:
if (window.testRunner)
On 2016/02/02 22:02:35, wkorman wrote:
> I think you can just pass true as a second param to this call to
> runAfterLayoutAndPaint() and omit this and next two lines.

Done.

Actually there was a syntax error here. Added logic to ensure the test finishes.

https://codereview.chromium.org/1661523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/FloatingObjects.h (right):

https://codereview.chromium.org/1661523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/FloatingObjects.h:154: FloatingObjectSet&
set() { return m_set; }
On 2016/02/02 22:02:35, wkorman wrote:
> Perhaps name this mutableSet() to more explicitly differentiate from the
other?

Good point. Done.

https://codereview.chromium.org/1661523002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp (right):

https://codereview.chromium.org/1661523002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:2647: if
(ancestorBlock->hasSelfPaintingLayer() ||
!ancestorBlock->isOverhangingFloat(floatingObject)) {
On 2016/02/02 20:49:40, chrishtr wrote:
> Please paste your explanatory comment here:
> 
> The rules of float painting are:
> - Let the nearest enclosing block in which the float doesn't overhang paint
the
> float;
> - Or even if the float overhangs, if the block has self-painting layer, the
> block paints the float.

Done.

[chrishtr, 2016-02-02 22:52:02]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-02-02 22:52:02]

lgtm

[commit-bot: I haz the power, 2016-02-02 22:56:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1661523002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1661523002/60001

[commit-bot: I haz the power, 2016-02-03 00:01:49]

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

[commit-bot: I haz the power, 2016-02-03 00:01:51]

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-02-03 06:40:27]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2016-02-03 06:40:28]

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/1661523002/#ps80001
(title: "Fix windows build")

[commit-bot: I haz the power, 2016-02-03 06:40:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1661523002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1661523002/80001

[commit-bot: I haz the power, 2016-02-03 08:22:42]

Description was changed from

==========
Update should paint flag when a float's composited scrolling status changes

Normally floating objects' should paint flags are updated during layout.
However, it also depends on self-painting status which can be changed
during compositing update (after layout) when composited scrolling
status changes.

Update should paint flag when floating object's composited scrolling
status changes.

No need to handle change of non-floating objects' composited scrolling
status affecting should paint flags of contained floating objects,
because should paint flags don't cross overflow clipping boundaries.

Also no need to handle the cases that have been handled by layout.

BUG=420072
TEST=fast/block/float/float-change-composited-scrolling.html
==========

to

==========
Update should paint flag when a float's composited scrolling status changes

Normally floating objects' should paint flags are updated during layout.
However, it also depends on self-painting status which can be changed
during compositing update (after layout) when composited scrolling
status changes.

Update should paint flag when floating object's composited scrolling
status changes.

No need to handle change of non-floating objects' composited scrolling
status affecting should paint flags of contained floating objects,
because should paint flags don't cross overflow clipping boundaries.

Also no need to handle the cases that have been handled by layout.

BUG=420072
TEST=fast/block/float/float-change-composited-scrolling.html
==========

[commit-bot: I haz the power, 2016-02-03 08:22:43]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-02-03 08:24:55]

Description was changed from

==========
Update should paint flag when a float's composited scrolling status changes

Normally floating objects' should paint flags are updated during layout.
However, it also depends on self-painting status which can be changed
during compositing update (after layout) when composited scrolling
status changes.

Update should paint flag when floating object's composited scrolling
status changes.

No need to handle change of non-floating objects' composited scrolling
status affecting should paint flags of contained floating objects,
because should paint flags don't cross overflow clipping boundaries.

Also no need to handle the cases that have been handled by layout.

BUG=420072
TEST=fast/block/float/float-change-composited-scrolling.html
==========

to

==========
Update should paint flag when a float's composited scrolling status changes

Normally floating objects' should paint flags are updated during layout.
However, it also depends on self-painting status which can be changed
during compositing update (after layout) when composited scrolling
status changes.

Update should paint flag when floating object's composited scrolling
status changes.

No need to handle change of non-floating objects' composited scrolling
status affecting should paint flags of contained floating objects,
because should paint flags don't cross overflow clipping boundaries.

Also no need to handle the cases that have been handled by layout.

BUG=420072
TEST=fast/block/float/float-change-composited-scrolling.html

Committed: https://crrev.com/871b186bc143ef3082dfb80f1437c2250cca06f3
Cr-Commit-Position: refs/heads/master@{#373198}
==========

[commit-bot: I haz the power, 2016-02-03 08:24:56]

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