[LayoutNG] More precise checks for empty inline and add test

[LayoutNG] More precise checks for empty inline and add test

Whether an empty inline should create a box or not affects several
things, such as:
* If nothing else on the line, it affects whether to create a line box
  or not, which has a height of strut.
* A line box prevents margins before and after to collapse.

While the condition is not clearly defined, all 4 browsers pass the
tests in this CL.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng

Review-Url: https://codereview.chromium.org/2931363002
Cr-Commit-Position: refs/heads/master@{#481114}
Committed: https://chromium.googlesource.com/chromium/src/+/9c70225b2a1ab3982b2c6345cc46f2d96bad025b

[kojii, 2017-06-12 10:19:42]

Description was changed from

==========
ngempty

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

to

==========
ngempty

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

[kojii, 2017-06-12 10:23:48]

Description was changed from

==========
ngempty

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

to

==========
[LayoutNG] More check and test for empty inline

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

[kojii, 2017-06-12 10:25:31]

kojii@chromium.org changed reviewers:
+ ikilpatrick@chromium.org

[kojii, 2017-06-12 10:25:32]

Ian, does this match to what you need?

Need more code to really create/disregard linebox, but great if you could check
SetShouldCreateLineBox() is something you can hook.

[kojii, 2017-06-12 10:27:12]

Description was changed from

==========
[LayoutNG] More check and test for empty inline

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

to

==========
[LayoutNG] More precise checks and add test for empty inline

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

[kojii, 2017-06-12 10:32:20]

On second thoughts, this doesn't solve if floats come earlier than
SetShouldCreateLineBox(). Huh...

[ikilpatrick, 2017-06-12 15:56:31]

On 2017/06/12 10:32:20, kojii wrote:
> On second thoughts, this doesn't solve if floats come earlier than
> SetShouldCreateLineBox(). Huh...

Right, this is why in my patch I explicitly keep them around until we know if
something is going to create a line.

The tricky case is:

Have unpositioned floats from previous sibling, no BFC offset yet:
I.e.
<div id=container>
  <div id=zero><div id=float1></div></div>
  <span><div id=float2></div></div>
  <div id=zero2></div>
  <span><div id=float3></div>text<div id=float4></div>text2</div>
</div>

We can only position floats 1, 2, 3 when we hit "text", e.g. that is the when we
know we can collapse our margins and compute the BFC offset.

Up until that point the NGLineBreaker has no way of knowing what the actual
available_width is (which is why I made it a WTF::Optional). It's impossible to
tell ahead of time unless we walk the inlines list twice - which is why I think
it needs to be inside NGLineBreaker.

Additionally float4 in this example will probably change the available_width.

Basically I think that the NGLineBreaker should be constraint space aware,
instead of just trying to fit text in 1-dimension. Does that make sense?

[kojii, 2017-06-12 16:09:40]

On 2017/06/12 at 15:56:31, ikilpatrick wrote:
> On 2017/06/12 10:32:20, kojii wrote:
> > On second thoughts, this doesn't solve if floats come earlier than
> > SetShouldCreateLineBox(). Huh...
> 
> Right, this is why in my patch I explicitly keep them around until we know if
something is going to create a line.
> 
> The tricky case is:
> 
> Have unpositioned floats from previous sibling, no BFC offset yet:
> I.e.
> <div id=container>
>   <div id=zero><div id=float1></div></div>
>   <span><div id=float2></div></div>
>   <div id=zero2></div>
>   <span><div id=float3></div>text<div id=float4></div>text2</div>
> </div>
> 
> We can only position floats 1, 2, 3 when we hit "text", e.g. that is the when
we know we can collapse our margins and compute the BFC offset.
> 
> Up until that point the NGLineBreaker has no way of knowing what the actual
available_width is (which is why I made it a WTF::Optional). It's impossible to
tell ahead of time unless we walk the inlines list twice - which is why I think
it needs to be inside NGLineBreaker.
> 
> Additionally float4 in this example will probably change the available_width.

I will need to pursue this since NG is failing some of tests, but then I'm back
to 2.a and 2.b should be handled in the same code path as 2.c.

We can try to optimize in some cases, but I'm not a fan of doing it too much
when the code becomes so complex. WDYT?

> Basically I think that the NGLineBreaker should be constraint space aware,
instead of just trying to fit text in 1-dimension. Does that make sense?

ah...can you explain a bit more?

[kojii, 2017-06-16 04:35:12]

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

[commit-bot: I haz the power, 2017-06-16 04:35:35]

Dry run: CQ is trying da patch.

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

[kojii, 2017-06-16 04:40:00]

Description was changed from

==========
[LayoutNG] More precise checks and add test for empty inline

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

to

==========
[LayoutNG] More precise checks for empty inline and add test

Whether an empty inline should create a box or not affects several
things, such as:
* If nothing else on the line, it affects whether to create a line box
  or not, which has a height of strut.
* A line box prevents margins before and after to collapse.

While the condition is not clearly defined, all 4 browsers pass the
tests in this CL.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[kojii, 2017-06-16 04:44:11]

PTAL, this is ready for review now.

All 4 browsers passed the included tests.

Moved a few variables to member as SetShouldCreateLineBox() needs them and there
are some call paths to it. I don't have strong preference to member or to add
them to parameters to all call paths, but is easy for now as we might need more.

rune@ and mstensho@ pointed out there are more cases such as when collapsed
whitespace affects line box creation in http://crbug.com/733609, I saw that in
the current code base. I'll study them further.

[commit-bot: I haz the power, 2017-06-16 08:58:39]

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

[commit-bot: I haz the power, 2017-06-16 08:58:40]

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

[kojii, 2017-06-19 05:44:15]

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

[commit-bot: I haz the power, 2017-06-19 05:44:19]

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-06-19 05:46:09]

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

[commit-bot: I haz the power, 2017-06-19 05:46:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kojii, 2017-06-19 06:27:42]

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

[commit-bot: I haz the power, 2017-06-19 06:27:54]

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-06-19 07:58:00]

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

[commit-bot: I haz the power, 2017-06-19 07:58:02]

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

[kojii, 2017-06-20 07:29:07]

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

[commit-bot: I haz the power, 2017-06-20 07:29: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-06-20 07:37:02]

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

[kojii, 2017-06-20 07:37:11]

Patchset #10 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-06-20 07:37:20]

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-06-20 08:20:36]

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

[commit-bot: I haz the power, 2017-06-20 08:20:37]

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

[ikilpatrick, 2017-06-20 23:48:16]

lgtm

https://codereview.chromium.org/2931363002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc (left):

https://codereview.chromium.org/2931363002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:103:
WTF::Optional<LayoutUnit> available_width;
I should explain why I did this in the first place :)

Basically my personal preference is to stack allocate things which only have
meaning for the duration of a function call (which available_width,
content_offset, position, should_create_line_box, have in this API)

I'm always fearful that somebody in the future will rely on the internal state
of the class and misuse it at a later date, e.g.

NGLineBreaker breaker(...)
breaker.BreakLine();
breaker.AvailableWidth(); // shouldn't be public but very easy to expose since
this is a member of the class.

E.g. in the block layout algorithm there are a few structs which are used for
this purpose:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/ng...

They basically ensure that the lifetime and access are well defined.

This is very much a personal preference, so this isn't blocking, just thought
i'd explain my thought process for this.

https://codereview.chromium.org/2931363002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc (right):

https://codereview.chromium.org/2931363002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:166: //
Returns the inline size of the first layout opportunity from the given
doesn't return

https://codereview.chromium.org/2931363002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:168: void
NGLineBreaker::ComputeAvailableWidth() {
maybe UpdateAvailableWidth now?

https://codereview.chromium.org/2931363002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/ng/inline/ng_line_breaker.cc:557:
ResolveBFCOffset();
consider inlining this function here (up to you).

[kojii, 2017-06-21 03:22:37]

Understood the point, thank you for taking time to explain, this is very
helpful!

I think then, as you pointed out, there are more states that are tied to
"current line", we should have a context object that represents the current line
as we do in block. That will suffice your point and also my desire not to pass
several arguments to all functions.

I'll consider the refactoring, thank you.

Others are all done, thank you again.

[kojii, 2017-06-21 03:26:01]

The CQ bit was checked by kojii@chromium.org

[kojii, 2017-06-21 03:26:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
ikilpatrick@chromium.org
Link to the patchset: https://codereview.chromium.org/2931363002/#ps220001
(title: "ikilpatrick review")

[commit-bot: I haz the power, 2017-06-21 03:26:10]

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-06-21 05:12:05]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1498015561068000,
"parent_rev": "2cc12536a8b000ea3e2e4e289b2a9b1989757336", "commit_rev":
"9c70225b2a1ab3982b2c6345cc46f2d96bad025b"}

[commit-bot: I haz the power, 2017-06-21 05:12:17]

Description was changed from

==========
[LayoutNG] More precise checks for empty inline and add test

Whether an empty inline should create a box or not affects several
things, such as:
* If nothing else on the line, it affects whether to create a line box
  or not, which has a height of strut.
* A line box prevents margins before and after to collapse.

While the condition is not clearly defined, all 4 browsers pass the
tests in this CL.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
[LayoutNG] More precise checks for empty inline and add test

Whether an empty inline should create a box or not affects several
things, such as:
* If nothing else on the line, it affects whether to create a line box
  or not, which has a height of strut.
* A line box prevents margins before and after to collapse.

While the condition is not clearly defined, all 4 browsers pass the
tests in this CL.

BUG=636993
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng

Review-Url: https://codereview.chromium.org/2931363002
Cr-Commit-Position: refs/heads/master@{#481114}
Committed:
https://chromium.googlesource.com/chromium/src/+/9c70225b2a1ab3982b2c6345cc46...
==========

[commit-bot: I haz the power, 2017-06-21 05:12:18]

Committed patchset #11 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/9c70225b2a1ab3982b2c6345cc46...