Fix compositingContainer for stacked inlines.

Fix compositingContainer for stacked inlines.

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

Review-Url: https://codereview.chromium.org/2640053004
Cr-Commit-Position: refs/heads/master@{#445164}
Committed: https://chromium.googlesource.com/chromium/src/+/2d6a760cbb0204f306e9312bc39036b75f80c5f2

[chrishtr, 2017-01-19 20:49:29]

Description was changed from

==========
none


none

BUG=
==========

to

==========
none


none

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

[chrishtr, 2017-01-19 20:50:57]

Description was changed from

==========
none


none

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

to

==========
Fix compositingContainer for stacked inlines.

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

[chrishtr, 2017-01-19 21:20:58]

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

[Xianzhu, 2017-01-19 21:50:02]

In the test case, should the inline object be the container (in the manner of
LayoutObject::container) of the floating object?

https://codereview.chromium.org/2640053004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/2640053004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:923:
!stackingNode()->isStacked() &&
Nit: How about combining the two stackingNode()->isStacked() conditions at line
923 and line 927?

https://codereview.chromium.org/2640053004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayerTest.cpp (right):

https://codereview.chromium.org/2640053004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayerTest.cpp:537: "      <iframe
srcdoc=foo' id='target' style='float: right'></iframe>"
Nit: s/foo'/'foo'/

[chrishtr, 2017-01-19 21:53:16]

On 2017/01/19 at 21:50:02, wangxianzhu wrote:
> In the test case, should the inline object be the container (in the manner of
LayoutObject::container) of the floating object?

Which testcase are you referring to?

> 
>
https://codereview.chromium.org/2640053004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):
> 
>
https://codereview.chromium.org/2640053004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/PaintLayer.cpp:923:
!stackingNode()->isStacked() &&
> Nit: How about combining the two stackingNode()->isStacked() conditions at
line 923 and line 927?
> 
>
https://codereview.chromium.org/2640053004/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/paint/PaintLayerTest.cpp (right):
> 
>
https://codereview.chromium.org/2640053004/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/paint/PaintLayerTest.cpp:537: "      <iframe
srcdoc=foo' id='target' style='float: right'></iframe>"
> Nit: s/foo'/'foo'/

[Xianzhu, 2017-01-19 21:55:44]

On 2017/01/19 21:53:16, chrishtr wrote:
> On 2017/01/19 at 21:50:02, wangxianzhu wrote:
> > In the test case, should the inline object be the container (in the manner
of
> LayoutObject::container) of the floating object?
> 
> Which testcase are you referring to?
> 

The layout test, containing a stacked floating object under a stacking-context
inline.

[chrishtr, 2017-01-19 22:03:20]

On 2017/01/19 at 21:55:44, wangxianzhu wrote:
> On 2017/01/19 21:53:16, chrishtr wrote:
> > On 2017/01/19 at 21:50:02, wangxianzhu wrote:
> > > In the test case, should the inline object be the container (in the manner
of
> > LayoutObject::container) of the floating object?
> > 
> > Which testcase are you referring to?
> > 
> 
> The layout test, containing a stacked floating object under a stacking-context
inline.

It's the unittest that has a stacking context (the clip-path one). In the layout
test, the compositing container that has a CLM (i.e. the one computed in
GraphicsLayerUpdater) is the docment/HTML element. The display: inline and
overflow: hidden / pos: relative elements don't create stacking contexts.

In the layout tests, the container for the float is the overflow: hidden / pos:
relative element.

[chrishtr, 2017-01-19 22:03:42]

On 2017/01/19 at 22:03:20, chrishtr wrote:
> On 2017/01/19 at 21:55:44, wangxianzhu wrote:
> > On 2017/01/19 21:53:16, chrishtr wrote:
> > > On 2017/01/19 at 21:50:02, wangxianzhu wrote:
> > > > In the test case, should the inline object be the container (in the
manner of
> > > LayoutObject::container) of the floating object?
> > > 
> > > Which testcase are you referring to?
> > > 
> > 
> > The layout test, containing a stacked floating object under a
stacking-context inline.
> 
> It's the unittest that has a stacking context (the clip-path one). In the
layout test, the compositing container that has a CLM (i.e. the one computed in
GraphicsLayerUpdater) is the docment/HTML element. The display: inline and
overflow: hidden / pos: relative elements don't create stacking contexts.
> 
> In the layout tests, the container for the float is the overflow: hidden /
pos: relative element.

(what I'm saying here is that this is what LayoutObject::container() returns,
nothing else. :)

[chrishtr, 2017-01-19 22:03:51]

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

[commit-bot: I haz the power, 2017-01-19 22:04:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2017-01-19 22:46:26]

On 2017/01/19 22:03:20, chrishtr wrote:
> On 2017/01/19 at 21:55:44, wangxianzhu wrote:
> > On 2017/01/19 21:53:16, chrishtr wrote:
> > > On 2017/01/19 at 21:50:02, wangxianzhu wrote:
> > > > In the test case, should the inline object be the container (in the
manner
> of
> > > LayoutObject::container) of the floating object?
> > > 
> > > Which testcase are you referring to?
> > > 
> > 
> > The layout test, containing a stacked floating object under a
stacking-context
> inline.
> 
> It's the unittest that has a stacking context (the clip-path one). In the
layout
> test, the compositing container that has a CLM (i.e. the one computed in
> GraphicsLayerUpdater) is the docment/HTML element. The display: inline and
> overflow: hidden / pos: relative elements don't create stacking contexts.
> 
> In the layout tests, the container for the float is the overflow: hidden /
pos:
> relative element.

Sorry I misread the layout test.

LayoutObject::container() (changed in
https://codereview.chromium.org/2575423003/) seems problematic for a floating
object under a relative or stacking-context inline (test case:
jsbin.com/hoyiwegiki).

This CL LGTM.

[commit-bot: I haz the power, 2017-01-20 00:16:44]

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

[commit-bot: I haz the power, 2017-01-20 00:16:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Xianzhu, 2017-01-20 00:52:14]

I'm working on a CL to add isStacked check for other floating special cases.

It's core is this new LayoutObject method:

  // A block can contain floating objects. A stacked inline can contain stacked
  // floating objects. A floating object's container is the nearest ancestor
  // that can contain the object.
  bool canContainFloatingObject(const LayoutObject& floating) const {
    DCHECK(floating.isFloating);
    return isLayoutBlock() ||
           (styleRef().isStacked() && floating.styleRef().isStacked());
  }

https://codereview.chromium.org/2640053004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/paint/inline/floating-inline.html (right):

https://codereview.chromium.org/2640053004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/inline/floating-inline.html:12: </div>
There are failures on win and mac bots.

Can you convert this test into a ref test?

[chrishtr, 2017-01-20 17:43:02]

https://codereview.chromium.org/2640053004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/paint/inline/floating-inline.html (right):

https://codereview.chromium.org/2640053004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/inline/floating-inline.html:12: </div>
On 2017/01/20 at 00:52:14, Xianzhu wrote:
> There are failures on win and mac bots.
> 
> Can you convert this test into a ref test?

I think this will be hard to do because of the floating element.

[Xianzhu, 2017-01-20 17:44:46]

https://codereview.chromium.org/2640053004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/paint/inline/floating-inline.html (right):

https://codereview.chromium.org/2640053004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/inline/floating-inline.html:12: </div>
On 2017/01/20 17:43:02, chrishtr wrote:
> On 2017/01/20 at 00:52:14, Xianzhu wrote:
> > There are failures on win and mac bots.
> > 
> > Can you convert this test into a ref test?
> 
> I think this will be hard to do because of the floating element.

Can it be like
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/paint/inv...
and
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/paint/inv...

[chrishtr, 2017-01-20 18:00:17]

https://codereview.chromium.org/2640053004/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/paint/inline/floating-inline.html (right):

https://codereview.chromium.org/2640053004/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/inline/floating-inline.html:12: </div>
On 2017/01/20 at 17:44:46, Xianzhu wrote:
> On 2017/01/20 17:43:02, chrishtr wrote:
> > On 2017/01/20 at 00:52:14, Xianzhu wrote:
> > > There are failures on win and mac bots.
> > > 
> > > Can you convert this test into a ref test?
> > 
> > I think this will be hard to do because of the floating element.
> 
> Can it be like
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/paint/inv...
and
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/paint/inv...

Ah yes, good point. I forgot that a float under an inline paints like the inline
wasn't there. Done.

[chrishtr, 2017-01-20 18:00:24]

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

[commit-bot: I haz the power, 2017-01-20 18:00:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Xianzhu, 2017-01-20 18:22:23]

https://codereview.chromium.org/2640053004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/paint/inline/floating-inline-expected.html
(right):

https://codereview.chromium.org/2640053004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/inline/floating-inline-expected.html:10:
</div>
Nit: can the expectation be further minimized? e.g. like:
  <div style="width: 100%; background: light blue">
    test
  </div>
plus anything needed?

[chrishtr, 2017-01-20 18:44:54]

https://codereview.chromium.org/2640053004/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/paint/inline/floating-inline-expected.html
(right):

https://codereview.chromium.org/2640053004/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/paint/inline/floating-inline-expected.html:10:
</div>
On 2017/01/20 at 18:22:23, Xianzhu wrote:
> Nit: can the expectation be further minimized? e.g. like:
>   <div style="width: 100%; background: light blue">
>     test
>   </div>
> plus anything needed?

Done.

[Xianzhu, 2017-01-20 18:45:41]

The CQ bit was checked by wangxianzhu@chromium.org

[Xianzhu, 2017-01-20 18:45:41]

lgtm

[commit-bot: I haz the power, 2017-01-20 18:45:54]

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-20 21:15:11]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484937941188270,
"parent_rev": "6c72e9c753adc139ac086deb62ce48ab4dd46957", "commit_rev":
"2d6a760cbb0204f306e9312bc39036b75f80c5f2"}

[commit-bot: I haz the power, 2017-01-20 21:15:44]

Description was changed from

==========
Fix compositingContainer for stacked inlines.

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

to

==========
Fix compositingContainer for stacked inlines.

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

Review-Url: https://codereview.chromium.org/2640053004
Cr-Commit-Position: refs/heads/master@{#445164}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d6a760cbb0204f306e9312bc390...
==========

[commit-bot: I haz the power, 2017-01-20 21:15:45]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/2d6a760cbb0204f306e9312bc390...