(CANCELED) [LayoutNG] Use ChildrenInline() to determine if children are inline

[LayoutNG] Use ChildrenInline() to determine if children are inline

LayoutObject::ChildrenInline() is set by examining more than just the
first child is inline. When NG relies on LayoutObject for tree building,
using ChildrenInline() helps to match to the LayoutObject tree.

BUG=712665

[kojii, 2017-04-24 08:56:02]

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

[commit-bot: I haz the power, 2017-04-24 08:56:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2017-04-24 09:00:00]

Description was changed from

==========
ngchildreninline

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

to

==========
[LayoutNG] Use ChildrenInline() to determine if children are inline

LayoutObject::ChildrenInline() is set by examining more than just the
first child is inline. When NG relies on LayoutObject for tree building,
using ChildrenInline() helps to match to the LayoutObject tree.

BUG=712665
==========

[commit-bot: I haz the power, 2017-04-24 10:31:34]

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

[commit-bot: I haz the power, 2017-04-24 10:31:34]

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

[kojii, 2017-04-25 02:04:37]

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

[commit-bot: I haz the power, 2017-04-25 02:05:03]

Dry run: 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-04-25 02:08:50]

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

[commit-bot: I haz the power, 2017-04-25 02:08:51]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kojii, 2017-04-25 02:33:51]

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

[commit-bot: I haz the power, 2017-04-25 02:34:22]

Dry run: 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-04-25 04:20:48]

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

[commit-bot: I haz the power, 2017-04-25 04:20:49]

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

[kojii, 2017-04-25 05:59:01]

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

[commit-bot: I haz the power, 2017-04-25 05:59:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kojii, 2017-04-25 05:59:50]

kojii@chromium.org changed reviewers:
+ eae@chromium.org, glebl@chromium.org, ikilpatrick@chromium.org

[kojii, 2017-04-25 05:59:51]

PTAL.

[eae, 2017-04-25 06:01:45]

I'd like Gleb to review this.

[commit-bot: I haz the power, 2017-04-25 07:44:41]

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

[commit-bot: I haz the power, 2017-04-25 07:44:42]

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

[atotic, 2017-04-26 07:46:44]

FYI: Original code 6 months ago used to call ChildrenInline(). I was the one who
rewrote it, because we wanted to test block layout with oof and floats.

[eae, 2017-04-26 09:01:07]

On 2017/04/26 07:46:44, atotic wrote:
> FYI: Original code 6 months ago used to call ChildrenInline(). I was the one
who
> rewrote it, because we wanted to test block layout with oof and floats.

Oh, right! Good memory. Would you mind review this then Aleks?

[Gleb Lanbin, 2017-04-26 21:52:53]

On 2017/04/26 09:01:07, eae wrote:
> On 2017/04/26 07:46:44, atotic wrote:
> > FYI: Original code 6 months ago used to call ChildrenInline(). I was the one
> who
> > rewrote it, because we wanted to test block layout with oof and floats.
> 
> Oh, right! Good memory. Would you mind review this then Aleks?

@kojii see https://codereview.chromium.org/2838673005/#msg13 I can take care of
this patch,
i.e. change the inline layout algorithm to support inline wrapper's
border/padding,
deprecate ShouldHandleByInlineContext in favor of ChildrenInline.

[Gleb Lanbin, 2017-04-27 00:18:13]

On 2017/04/26 21:52:53, Gleb Lanbin wrote:
> On 2017/04/26 09:01:07, eae wrote:
> > On 2017/04/26 07:46:44, atotic wrote:
> > > FYI: Original code 6 months ago used to call ChildrenInline(). I was the
one
> > who
> > > rewrote it, because we wanted to test block layout with oof and floats.
> > 
> > Oh, right! Good memory. Would you mind review this then Aleks?
> 
> @kojii see https://codereview.chromium.org/2838673005/#msg13 I can take care
of
> this patch,
> i.e. change the inline layout algorithm to support inline wrapper's
> border/padding,
> deprecate ShouldHandleByInlineContext in favor of ChildrenInline.

I looked why NGBlockLayoutAlgorithmTest.CollapsingMarginsCase7 started failing
and found
that with this change in this example

    <div id="zero">
      <div id="float"></div>
    </div>

#float will become an inline node. Although in this example

    <div id="zero">
      <div id="float"></div>
      <div style="height:25"></div>
    </div>

#float will be a block node. In the current version of our LayoutNG block layout
algorithm we assume that we can determine our block position before we lay out
the 1st inline node. I'm afraid if we proceed with this change we will have a
lot of checks like
child->IsInline && !child->IsFloating in our code.

I still think that in the 1st example #zero layout object should return FALSE if
ChildreInline() called. In other words I think that nodes that are not
surrounded by inline nodes should not be handled by the inline context. I think
it's ok to change how we build  LayoutObject tree for this case if LayoutNG flag
is enabled (we need to change LayoutBlockFlow::MakeChildrenInlineIfPossible for
this).

What do you think?

[kojii, 2017-04-27 03:31:33]

Summarized cases in a doc:
https://docs.google.com/a/chromium.org/document/d/1kAJLEYHl5eE6ean3NSTnJOuGbJ...
Case 3 is your 1st example and case 4 is your 2nd.

The last change was:
1. To avoid having mixed children
2. NGInlineNode.NextSibling() should always be nullptr
So the case 3 (floats/oof are the only children) can go either way to me.

I thought using inline context can eliminate float/oof code from block layout,
but you're right, we have the case in your 2nd example. (case 4 in my
doc...curious, happen to know if the legacy have two code paths to handle this
case?)

Are we aware any other differences between ChildrenInline() and
ShouldHandleByInlineContext()? If this is the only difference, and Ian is good,
I'm good, but maybe Alex or Christian know better?

[kojii, 2017-05-07 04:45:08]

Cancelling this.