Merge patch for stacked floats with shape-outside from WebKit

Merge patch for stacked floats with shape0-outside from WebKit

From the webkit changelog:
Stacked floats get positioned based on the bounding box of the shape,
not on the shape contours itself. This patch causes that to happen.

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

[Julien - ping for review, 2013-04-10 21:33:59]

https://codereview.chromium.org/13909006/diff/1/LayoutTests/fast/exclusions/s...
File
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked.html
(right):

https://codereview.chromium.org/13909006/diff/1/LayoutTests/fast/exclusions/s...
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked.html:20:
.triangle-left:before {
Do we really need such a complicated test case (involving 2 :before + content +
absolute position) to test the code?

https://codereview.chromium.org/13909006/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/13909006/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderBlockLineLayout.cpp:194: for (--it; it != begin;
--it) {
Won't this loop ignore the first float if it is your "last float"? (I don't
really like the name as it's imprecise and fairly confusing).

You will also never got into the for loop if you have only one float.

[Bem Jones-Bey (gmail), 2013-04-11 00:00:55]

On 2013/04/10 21:33:59, jchaffraix wrote:
>
https://codereview.chromium.org/13909006/diff/1/LayoutTests/fast/exclusions/s...
> File
>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked.html
> (right):
> 
>
https://codereview.chromium.org/13909006/diff/1/LayoutTests/fast/exclusions/s...
>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked.html:20:
> .triangle-left:before {
> Do we really need such a complicated test case (involving 2 :before + content
+
> absolute position) to test the code?

Strictly speaking, no. It's just there to add a display of the shape to make it
easier for someone looking at the test to understand what's going on (and make
it easier to write an explanation of what the user should see). I could remove
it if you think it makes things harder to understand. (Or I could replace those
with SVGs instead if that's clearer)

> 
>
https://codereview.chromium.org/13909006/diff/1/Source/WebCore/rendering/Rend...
> File Source/WebCore/rendering/RenderBlockLineLayout.cpp (right):
> 
>
https://codereview.chromium.org/13909006/diff/1/Source/WebCore/rendering/Rend...
> Source/WebCore/rendering/RenderBlockLineLayout.cpp:194: for (--it; it !=
begin;
> --it) {
> Won't this loop ignore the first float if it is your "last float"? (I don't
> really like the name as it's imprecise and fairly confusing).
> 
> You will also never got into the for loop if you have only one float.

You are correct that it will ignore the first float. This isn't a problem with
the current way this method is used, as it is always called after the new float
has already been added to the floating objects list. However, I will change it
so that it doesn't skip the first element in the list, just as a defensive
practice in case the circumstances in which this method are called ever change.

As for the naming, does "previousFloat" make more sense to you? Or
previousFloatOnLine?

And I just saw another issue, I should be checking to make sure that the float I
find is actually on the current line before selecting it.

[Bem Jones-Bey (gmail), 2013-04-11 22:49:45]

I've fixed the issue with the loop and renamed the lastFloat variable to
previousFloat.

The issue I thought I had discovered about checking for the line turns out to
not be an issue, so I haven't made any changes related to that.

I didn't change the test, but I can do that if using SVGs instead of psuedo
elements would be better. (or if I should remove the visualization of the shapes
entirely from the test, but that makes it much harder to describe in words what
someone looking at the test should see.)

[Bem Jones-Bey (gmail), 2013-04-15 16:28:30]

Hey Julien, do you have time to look at my updates?

[Julien - ping for review, 2013-04-17 21:20:22]

https://codereview.chromium.org/13909006/diff/13001/LayoutTests/fast/exclusio...
File
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked-expected.html
(right):

https://codereview.chromium.org/13909006/diff/13001/LayoutTests/fast/exclusio...
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked-expected.html:15:
.triangle-left:before {
As I said, I would better have a more focused test case. As a rule tests should
be minimal and serve as a documentation for our behavior.

Here all the craziness you add only obscure what the test is doing.

https://codereview.chromium.org/13909006/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/13909006/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.h:174: LayoutUnit
logicalRightOffsetForLine(LayoutUnit position, bool shouldIndentText,
RenderRegion* region, LayoutUnit offsetFromLogicalTopOfFirstPage, LayoutUnit
logicalHeight = 0, ShapeOutsideFloatOffsetMode offsetMode =
ShapeOutsideFloatShapeOffset) const
Adding more parameters is making this function confusing as we now have 6
parameters (some of which have implicit values).

Couldn't we split the 2 code paths instead of piling up more arguments?

https://codereview.chromium.org/13909006/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlockLineLayout.cpp (right):

https://codereview.chromium.org/13909006/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlockLineLayout.cpp:183: #if ENABLE(CSS_EXCLUSIONS)
We should probably remove these.

[Bem Jones-Bey (gmail), 2013-04-17 21:47:10]

On 2013/04/17 21:20:22, Julien Chaffraix wrote:
>
https://codereview.chromium.org/13909006/diff/13001/LayoutTests/fast/exclusio...
> File
>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked-expected.html
> (right):
> 
>
https://codereview.chromium.org/13909006/diff/13001/LayoutTests/fast/exclusio...
>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked-expected.html:15:
> .triangle-left:before {
> As I said, I would better have a more focused test case. As a rule tests
should
> be minimal and serve as a documentation for our behavior.
> 
> Here all the craziness you add only obscure what the test is doing.

Ok, I'll come up with a test that doesn't need to display the shapes.

> 
>
https://codereview.chromium.org/13909006/diff/13001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderBlock.h (right):
> 
>
https://codereview.chromium.org/13909006/diff/13001/Source/core/rendering/Ren...
> Source/core/rendering/RenderBlock.h:174: LayoutUnit
> logicalRightOffsetForLine(LayoutUnit position, bool shouldIndentText,
> RenderRegion* region, LayoutUnit offsetFromLogicalTopOfFirstPage, LayoutUnit
> logicalHeight = 0, ShapeOutsideFloatOffsetMode offsetMode =
> ShapeOutsideFloatShapeOffset) const
> Adding more parameters is making this function confusing as we now have 6
> parameters (some of which have implicit values).
> 
> Couldn't we split the 2 code paths instead of piling up more arguments?

I'll try and come up with a split that isn't more confusing. (Many of the
functions in WebKit have way more arguments than I think are reasonable, so it's
hard for me to know exactly how many arguments tip the scale.)

> 
>
https://codereview.chromium.org/13909006/diff/13001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderBlockLineLayout.cpp (right):
> 
>
https://codereview.chromium.org/13909006/diff/13001/Source/core/rendering/Ren...
> Source/core/rendering/RenderBlockLineLayout.cpp:183: #if
ENABLE(CSS_EXCLUSIONS)
> We should probably remove these.

Yeah, I'll remove them from this patch. Bear is putting together a patch to
remove them from the rest of the code.

[Bem Jones-Bey (gmail), 2013-05-20 20:49:06]

Per review comments, I've updated the test to be much simpler. I have also
refactored the code so that I am no longer adding another argument to
logical(Left|Right)OffsetForLine.

[Julien - ping for review, 2013-05-29 17:14:49]

Thanks, the test case is a lot better. Mostly cosmetic comments and some
questions about the test case but we are nearly good to go.

https://codereview.chromium.org/13909006/diff/20001/LayoutTests/fast/exclusio...
File
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked.html
(right):

https://codereview.chromium.org/13909006/diff/20001/LayoutTests/fast/exclusio...
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked.html:6:
window.internals.settings.setCSSExclusionsEnabled(true);
Do we need this? CSS runtime features are enabled by default AFAICT.

https://codereview.chromium.org/13909006/diff/20001/LayoutTests/fast/exclusio...
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked.html:20:
height: 100px;
I would have expected these 2 properties to not be required for the test to pass
as the 'shape-outside' should define the float area. Am I missing something or
is this a bug in our implementation?

https://codereview.chromium.org/13909006/diff/20001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/13909006/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:4036: floatLogicalLeft =
logicalLeftOffsetForLineUsingFloatBoundingBox(logicalTopOffset,
logicalLeftOffset, false, &heightRemainingLeft, 0);
The last argument (0) is optional, it's probably better to avoid it altogether
as it doesn't add much. I won't repeat it for the other calls to
logicalRightOffsetForLineUsingFloatBoundingBox.

https://codereview.chromium.org/13909006/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:4360: LayoutUnit
RenderBlock::logicalLeftOffsetForLineWithoutFloats(LayoutUnit fixedOffset, bool
applyTextIndent) const
Ideally we should ASSERT that there is no floats to prevent misuse.

https://codereview.chromium.org/13909006/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:4422: LayoutUnit
RenderBlock::logicalRightOffsetForLineWithoutFloats(LayoutUnit fixedOffset, bool
applyTextIndent) const
Ditto.

[Bem Jones-Bey (gmail), 2013-05-29 19:11:19]

On 2013/05/29 17:14:49, Julien Chaffraix wrote:
> Thanks, the test case is a lot better. Mostly cosmetic comments and some
> questions about the test case but we are nearly good to go.
> 
>
https://codereview.chromium.org/13909006/diff/20001/LayoutTests/fast/exclusio...
> File
>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked.html
> (right):
> 
>
https://codereview.chromium.org/13909006/diff/20001/LayoutTests/fast/exclusio...
>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked.html:6:
> window.internals.settings.setCSSExclusionsEnabled(true);
> Do we need this? CSS runtime features are enabled by default AFAICT.

Looks like we don't. I've removed it.

>
https://codereview.chromium.org/13909006/diff/20001/LayoutTests/fast/exclusio...
>
LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-stacked.html:20:
> height: 100px;
> I would have expected these 2 properties to not be required for the test to
pass
> as the 'shape-outside' should define the float area. Am I missing something or
> is this a bug in our implementation?

shape-outside does define the float area, but it doesn't change the box model of
the float, so if I don't give a width and height, it would not be possible to
see the background color of the float (since the float's content box would have
a 0 width and height). Does that make sense?

> 
>
https://codereview.chromium.org/13909006/diff/20001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderBlock.cpp (right):
> 
>
https://codereview.chromium.org/13909006/diff/20001/Source/core/rendering/Ren...
> Source/core/rendering/RenderBlock.cpp:4036: floatLogicalLeft =
> logicalLeftOffsetForLineUsingFloatBoundingBox(logicalTopOffset,
> logicalLeftOffset, false, &heightRemainingLeft, 0);
> The last argument (0) is optional, it's probably better to avoid it altogether
> as it doesn't add much. I won't repeat it for the other calls to
> logicalRightOffsetForLineUsingFloatBoundingBox.

Removed.

> 
>
https://codereview.chromium.org/13909006/diff/20001/Source/core/rendering/Ren...
> Source/core/rendering/RenderBlock.cpp:4360: LayoutUnit
> RenderBlock::logicalLeftOffsetForLineWithoutFloats(LayoutUnit fixedOffset,
bool
> applyTextIndent) const
> Ideally we should ASSERT that there is no floats to prevent misuse.
> 
>
https://codereview.chromium.org/13909006/diff/20001/Source/core/rendering/Ren...
> Source/core/rendering/RenderBlock.cpp:4422: LayoutUnit
> RenderBlock::logicalRightOffsetForLineWithoutFloats(LayoutUnit fixedOffset,
bool
> applyTextIndent) const
> Ditto.

Perhaps it needs a better name. I've renamed the 'fixedOffset' parameter to
'offsetFromFloats', to hopefully make things clearer. It gets called when there
are floats, it just does the part of the computation that is independent of
floats, and expects that the offset passed in will be the offset including
floats. Technically, it's an implementation detail of
logical(Right|Left)OffsetForLine and
logical(Right|Left)OffsetForLineUsingFloatBoundingBox, and really shouldn't be
used anywhere else, which is why I made them private.

[Bem Jones-Bey (gmail), 2013-06-03 16:54:03]

*bump*

[Julien - ping for review, 2013-06-06 20:49:07]

https://codereview.chromium.org/13909006/diff/28004/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/13909006/diff/28004/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:4386: LayoutUnit
RenderBlock::logicalRightOffsetForLineWithoutFloats(LayoutUnit offsetFromFloats,
bool applyTextIndent) const
I am unsure about what this function intents to do. "without floats" for me
means that there shouldn't be any floats or else the results would be wrong. BUT
you do pass an offset from float to something that ignore floats so I am left
confused.

https://codereview.chromium.org/13909006/diff/28004/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.h (right):

https://codereview.chromium.org/13909006/diff/28004/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.h:69: enum ShapeOutsideFloatOffsetMode {
ShapeOutsideFloatShapeOffset, ShapeOutsideFloatBoundingBoxOffset };
We don't need this anymore.

[Bem Jones-Bey (gmail), 2013-06-24 18:46:15]

On 2013/06/06 20:49:07, Julien Chaffraix wrote:
>
https://codereview.chromium.org/13909006/diff/28004/Source/core/rendering/Ren...
> File Source/core/rendering/RenderBlock.cpp (right):
> 
>
https://codereview.chromium.org/13909006/diff/28004/Source/core/rendering/Ren...
> Source/core/rendering/RenderBlock.cpp:4386: LayoutUnit
> RenderBlock::logicalRightOffsetForLineWithoutFloats(LayoutUnit
offsetFromFloats,
> bool applyTextIndent) const
> I am unsure about what this function intents to do. "without floats" for me
> means that there shouldn't be any floats or else the results would be wrong.
BUT
> you do pass an offset from float to something that ignore floats so I am left
> confused.

I've renamed it to 'adjustLogicalRightOffsetForLine'. Does that make more sense?
If not, some guidance would be appreciated.

> 
>
https://codereview.chromium.org/13909006/diff/28004/Source/core/rendering/Ren...
> File Source/core/rendering/RenderBlock.h (right):
> 
>
https://codereview.chromium.org/13909006/diff/28004/Source/core/rendering/Ren...
> Source/core/rendering/RenderBlock.h:69: enum ShapeOutsideFloatOffsetMode {
> ShapeOutsideFloatShapeOffset, ShapeOutsideFloatBoundingBoxOffset };
> We don't need this anymore.

I like still using it because it is more readable than using a boolean. I've
moved it to be private to the class, however.

[Julien - ping for review, 2013-07-09 20:05:48]

I am not happy about the extra complexity added by this change (we now have 8
functions to do what 2 did before the change) but can't think of way to avoid it
as we need to get new offsets.

LGTM...

https://codereview.chromium.org/13909006/diff/35001/Source/core/rendering/Ren...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/13909006/diff/35001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:4322:
m_floatingObjects->placedFloatsTree().allOverlapsWithAdapter(adapter);
We should avoid placing the floats if we don't need them below for the
offsetMode != ShapeOutsideFloatShapeOffset case.

https://codereview.chromium.org/13909006/diff/35001/Source/core/rendering/Ren...
Source/core/rendering/RenderBlock.cpp:4382:
FloatIntervalSearchAdapter<FloatingObject::FloatRight> adapter(this,
roundToInt(logicalTop), roundToInt(logicalTop + logicalHeight),
rightFloatOffset, heightRemaining);
Same comment.

[Bem Jones-Bey (gmail), 2013-07-09 23:56:50]

On 2013/07/09 20:05:48, Julien Chaffraix wrote:
> I am not happy about the extra complexity added by this change (we now have 8
> functions to do what 2 did before the change) but can't think of way to avoid
it
> as we need to get new offsets.
> 
> LGTM...
> 
>
https://codereview.chromium.org/13909006/diff/35001/Source/core/rendering/Ren...
> File Source/core/rendering/RenderBlock.cpp (right):
> 
>
https://codereview.chromium.org/13909006/diff/35001/Source/core/rendering/Ren...
> Source/core/rendering/RenderBlock.cpp:4322:
> m_floatingObjects->placedFloatsTree().allOverlapsWithAdapter(adapter);
> We should avoid placing the floats if we don't need them below for the
> offsetMode != ShapeOutsideFloatShapeOffset case.
> 
>
https://codereview.chromium.org/13909006/diff/35001/Source/core/rendering/Ren...
> Source/core/rendering/RenderBlock.cpp:4382:
> FloatIntervalSearchAdapter<FloatingObject::FloatRight> adapter(this,
> roundToInt(logicalTop), roundToInt(logicalTop + logicalHeight),
> rightFloatOffset, heightRemaining);
> Same comment.

That's the original code to update the offset from floats. It updates both left
(or right) and heightRemaining. The only behavior I added to it was to set
lastFloat so that I could adjust the offset it computes if we are determining
the shape offset (as opposed to the bounding box offset).

(Yes, it would be better if it was more explicit about the fact that it does
meaningful work, but that would be a different patch entirely to redesign how
that works.)

[commit-bot: I haz the power, 2013-07-10 15:34:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bemajaniman@gmail.com/13909006/42001

[commit-bot: I haz the power, 2013-07-10 17:05:07]

Change committed as 153908