Add ClearanceOffset to LayoutNG Constraint space.

Add ClearanceOffset to LayoutNG Constraint space.

It's used for the case when fragment can position itself but needs to
know the current clearance line.

In particular this change fixes the use case that has blocks with
clearances, margins and intruding floats
https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4847

BUG=635619
TEST=PositionBlocksWithClearanceAndIntrudingFloats

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

[Gleb Lanbin, 2017-02-23 23:30:03]

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

[ikilpatrick, 2017-02-24 17:36:57]

this looks really good, just one question about the margin strut.

https://codereview.chromium.org/2711803007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2711803007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:799:
curr_bfc_offset_.block_offset += curr_margin_strut_.Sum();
Are we able to do this here? I.e. does it handle something like:
(a clear: left child which has a zero height div which effects the
margin-strut?)

https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E...

https://codereview.chromium.org/2711803007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h
(right):

https://codereview.chromium.org/2711803007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:43: //
Updates the fragment's BFC offset if it's not already set.
I might go through later and reorder everything so it roughly matches the cc
file. We are a little all over the place at the moment.

https://codereview.chromium.org/2711803007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(right):

https://codereview.chromium.org/2711803007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:173:
WTF::Optional<LayoutUnit> clearance_offset =
worth adding more comments here for the is_new_fc_ cases?

[Gleb Lanbin, 2017-02-24 18:37:00]

thanks for the review.

https://codereview.chromium.org/2711803007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc
(right):

https://codereview.chromium.org/2711803007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.cc:799:
curr_bfc_offset_.block_offset += curr_margin_strut_.Sum();
On 2017/02/24 17:36:57, ikilpatrick wrote:
> Are we able to do this here? I.e. does it handle something like:
> (a clear: left child which has a zero height div which effects the
> margin-strut?)
> 
>
https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E...

thanks for the test case. It works as expected, updated the documentation
section a bit.

https://codereview.chromium.org/2711803007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h
(right):

https://codereview.chromium.org/2711803007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_block_layout_algorithm.h:43: //
Updates the fragment's BFC offset if it's not already set.
On 2017/02/24 17:36:57, ikilpatrick wrote:
> I might go through later and reorder everything so it roughly matches the cc
> file. We are a little all over the place at the moment.

agree, I tried to organize things here (see "Read-only Getters" section for
example) in the past but we need all agree how we want to group methods. I like
Oracle java guideline
https://web.archive.org/web/20130627193836/http://www.oracle.com/technetwork/...
We can try to follow it here. For now I just moved this method closed to the end
before "getters" section.

https://codereview.chromium.org/2711803007/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc
(right):

https://codereview.chromium.org/2711803007/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_constraint_space_builder.cc:173:
WTF::Optional<LayoutUnit> clearance_offset =
On 2017/02/24 17:36:57, ikilpatrick wrote:
> worth adding more comments here for the is_new_fc_ cases?

Done.

[Gleb Lanbin, 2017-02-24 18:37:23]

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

[commit-bot: I haz the power, 2017-02-24 18:37:42]

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

[ikilpatrick, 2017-02-24 18:39:17]

lgtm, thanks for the additional comments. :)

[commit-bot: I haz the power, 2017-02-24 18:40:40]

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

[commit-bot: I haz the power, 2017-02-24 18:40:41]

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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Gleb Lanbin, 2017-02-24 18:53:58]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2017-02-24 18:53:59]

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/2711803007/#ps40001
(title: "git rebase-update")

[commit-bot: I haz the power, 2017-02-24 18:54:43]

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-02-24 19:10:21]

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

[commit-bot: I haz the power, 2017-02-24 19:10:23]

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

[Gleb Lanbin, 2017-02-24 22:07:22]

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

[commit-bot: I haz the power, 2017-02-24 22:08:29]

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-02-25 01:06:38]

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

[commit-bot: I haz the power, 2017-02-25 01:06:43]

Dry run: This issue passed the CQ dry run.

[Gleb Lanbin, 2017-02-25 01:12:39]

The CQ bit was checked by glebl@chromium.org

[Gleb Lanbin, 2017-02-25 01:12:39]

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/2711803007/#ps50009
(title: "update TestExpectations")

[commit-bot: I haz the power, 2017-02-25 01:12:51]

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-02-25 01:19:45]

CQ is committing da patch.

Bot data: {"patchset_id": 50009, "attempt_start_ts": 1487985159458830,
"parent_rev": "b8ab8bd804ea8206e8f11ed10c6ea93a9b0dffcf", "commit_rev":
"2d05eecafb761285205faf28e05f3c73f75efeb7"}

[commit-bot: I haz the power, 2017-02-25 01:21:16]

Description was changed from

==========
Add ClearanceOffset to LayoutNG Constraint space.

It's used for the case when fragment can position itself but needs to
know the current clearance line.

In particular this change fixes the use case that has blocks with
clearances, margins and intruding floats
https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4847

BUG=635619
TEST=PositionBlocksWithClearanceAndIntrudingFloats
==========

to

==========
Add ClearanceOffset to LayoutNG Constraint space.

It's used for the case when fragment can position itself but needs to
know the current clearance line.

In particular this change fixes the use case that has blocks with
clearances, margins and intruding floats
https://software.hixie.ch/utilities/js/live-dom-viewer/saved/4847

BUG=635619
TEST=PositionBlocksWithClearanceAndIntrudingFloats

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

[commit-bot: I haz the power, 2017-02-25 01:21:17]

Committed patchset #4 (id:50009) as
https://chromium.googlesource.com/chromium/src/+/2d05eecafb761285205faf28e05f...