Remove out-of-date FIXME on collapsing margins

Remove out-of-date FIXME on collapsing margins

CSS2.1 is now clear about what to do if the collapsed margins are
greater than the offset required to clear a float.

"Computing the clearance of an element on which 'clear' is set is done
by first determining the hypothetical position of the element's top
border edge. This position is where the actual top border edge would
have been if the element's 'clear' property had been 'none'. If this
hypothetical position of the element's top border edge is not past
the relevant floats, then clearance [..] is set to the greater of:
The amount necessary to place the border edge of the block even with
the bottom outer edge of the lowest float that is to be cleared. [or]
The amount necessary to place the top border edge of the block at
its hypothetical position."

http://www.w3.org/TR/CSS2/visuren.html#clearance


BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169798

[Julien - ping for review, 2014-03-19 00:56:24]

lgtm

https://codereview.chromium.org/195743002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlockFlow.cpp (left):

https://codereview.chromium.org/195743002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlockFlow.cpp:1303: // margins involved.
Removing this FIXME is OK but are we matching CSS 2.1's behavior? Is this
covered by any cases?

By removing this FIXME, we may be losing the fact that this code needs to be
updated. I am fine with either fixing (along with appropriate testing) or just
adding an up-to-date FIXME.

[rhogan, 2014-03-20 19:48:01]

On 2014/03/19 00:56:24, Julien Chaffraix - PST wrote:
> lgtm
> 
>
https://codereview.chromium.org/195743002/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderBlockFlow.cpp (left):
> 
>
https://codereview.chromium.org/195743002/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderBlockFlow.cpp:1303: // margins involved.
> Removing this FIXME is OK but are we matching CSS 2.1's behavior? Is this
> covered by any cases?

Yes. But I'm not sure if we have a test case that covers it. Such a case would
be:

data:text/html,<div><div style="height: 20px; width:50px; background-color:
green; margin-bottom: 5px;"></div><div style="height: 10px; width:50px;
background-color: blue; float:left;"></div><div style="height: 20px; width:50px;
background-color: green; margin-top: 20px; clear:both"></div></div>

The second green box sits 20px below the other green box - i.e. it chooses the
greater of its hypothetical position without clearance and its position after
clearance (which would be directly below the float instead of an extra 5px below
it).

> 
> By removing this FIXME, we may be losing the fact that this code needs to be
> updated. I am fine with either fixing (along with appropriate testing) or just
> adding an up-to-date FIXME.

I'll land it with the above test.

[rhogan, 2014-03-22 12:49:08]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2014-03-22 12:49:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robhogan@gmail.com/195743002/20001

[commit-bot: I haz the power, 2014-03-22 13:50:55]

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

[commit-bot: I haz the power, 2014-03-22 13:50:55]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[eseidel, 2014-03-22 18:34:29]

The CQ bit was checked by eseidel@chromium.org

[commit-bot: I haz the power, 2014-03-22 18:34:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robhogan@gmail.com/195743002/20001

[commit-bot: I haz the power, 2014-03-22 20:56:38]

Change committed as 169798