Detect floats to avoid or clear due to negative margin top

Detect floats to avoid or clear due to negative margin top

When a negative margin top pushes a block back up into its previous siblings
we need to check for any floats in those siblings it now needs to avoid or clear.

Previously we were just looking at its neighbour, we need to keep looking until
we reach a sibling that we don't overlap.

BUG=666487
Committed: https://crrev.com/3c8d298acf826fc5337c526b1016a03b37c2656a
Cr-Commit-Position: refs/heads/master@{#435497}

[rhogan, 2016-11-26 12:46:11]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-26 12:46:15]

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, 2016-11-26 13:47:16]

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

[commit-bot: I haz the power, 2016-11-26 13:47:17]

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_...)

[rhogan, 2016-11-27 11:18:49]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-27 11:19:00]

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

[rhogan, 2016-11-27 11:29:51]

Description was changed from

==========
Detect floats to avoid or clear due to negative margin top

BUG=666487
==========

to

==========
Detect floats to avoid or clear due to negative margin top

When a negative margin top pushes a block back up into its previous siblings
we need to check for any floats in those siblings it now needs to avoid or
clear.

Previously we were just looking at its neighbour, we need to keep looking until
we reach a sibling that we don't overlap.

BUG=666487
==========

[rhogan, 2016-11-27 11:29:52]

robhogan@gmail.com changed reviewers:
+ eae@chromium.org

[rhogan, 2016-11-27 11:31:50]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-27 11:31:59]

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, 2016-11-27 12:29:38]

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

[commit-bot: I haz the power, 2016-11-27 12:29:39]

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_...)

[rhogan, 2016-11-27 13:42:50]

The CQ bit was checked by robhogan@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-27 13:42:59]

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, 2016-11-27 15:48:41]

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

[commit-bot: I haz the power, 2016-11-27 15:48:42]

Dry run: This issue passed the CQ dry run.

[rhogan, 2016-11-29 20:09:56]

robhogan@gmail.com changed reviewers:
+ kojii@chromium.org, pdr@chromium.org

[rhogan, 2016-11-29 20:10:15]

On 2016/11/27 at 11:29:57, rhogan wrote:
> 

pdr/kojii - can either of you take a look in eae's absence?

[kojii, 2016-11-30 04:24:47]

lgtm, great fix for interop, thank you for working on this.

[kojii, 2016-11-30 05:50:23]

(pdr@, I'm not an owner)

[rhogan, 2016-11-30 08:08:37]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2016-11-30 08:09:06]

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, 2016-11-30 08:21:27]

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

[commit-bot: I haz the power, 2016-11-30 08:21:31]

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...)

[pdr., 2016-11-30 21:45:33]

Two minor suggestions, LGTM otherwise.

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

https://codereview.chromium.org/2531953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1724: prev &&
prev->isLayoutBlockFlow() ? toLayoutBlockFlow(prev) : 0;
nit: nullptr instead of 0?

https://codereview.chromium.org/2531953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlockFlow.cpp:1824: while
(previousBlockFlow &&
This is pretty hard to read. Do you think we could refactor it?

Something like:
while (previousBlockFlow) {
  auto previousNeedsBetterName = previousBlockFlow->logicalTop() +
    previousBlockFlow->lowestFloatLogicalBottom();
  if (previousNeedsBetterName > logicalTop)
    addOverhangingFloats(previousBlockFlow, false);
  auto* previousSibling = previousBlockFlow->previousSibling();
  if (previousSibling && previousSibling->isLayoutBlockFlow())
    previousBlockFlow = toLayoutBlockFlow(previousSibling);
  else
    break;
}

[pdr., 2016-11-30 21:47:04]

On 2016/11/30 at 21:45:33, pdr. wrote:
>   else
>     break;
> }

err... else previousBlockFlow = nullptr;

[rhogan, 2016-11-30 22:57:15]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2016-11-30 22:57:16]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
kojii@chromium.org
Link to the patchset: https://codereview.chromium.org/2531953002/#ps80001
(title: "bug 666487")

[commit-bot: I haz the power, 2016-11-30 22:57:37]

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, 2016-12-01 00:38:05]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480546635794070,
"parent_rev": "59cfb9b67b37ffa3b2a923bdf24f281f978b70fe", "commit_rev":
"53122f4907a7ef081fe38f1ba817eec65205f431"}

[commit-bot: I haz the power, 2016-12-01 00:38:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-01 00:42:14]

Description was changed from

==========
Detect floats to avoid or clear due to negative margin top

When a negative margin top pushes a block back up into its previous siblings
we need to check for any floats in those siblings it now needs to avoid or
clear.

Previously we were just looking at its neighbour, we need to keep looking until
we reach a sibling that we don't overlap.

BUG=666487
==========

to

==========
Detect floats to avoid or clear due to negative margin top

When a negative margin top pushes a block back up into its previous siblings
we need to check for any floats in those siblings it now needs to avoid or
clear.

Previously we were just looking at its neighbour, we need to keep looking until
we reach a sibling that we don't overlap.

BUG=666487

Committed: https://crrev.com/3c8d298acf826fc5337c526b1016a03b37c2656a
Cr-Commit-Position: refs/heads/master@{#435497}
==========

[commit-bot: I haz the power, 2016-12-01 00:42:15]

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