[webkit-reviews] review denied: [Bug 121249] Make FloatingObjects responsible for creating all FloatingObject instances : [Attachment 211461] Patch

[bugzilla-daemon at webkit.org]

Alexandru Chiculita <achicu at adobe.com> has denied Bem Jones-Bey
<bjonesbe at adobe.com>'s request for review:
Bug 121249: Make FloatingObjects responsible for creating all FloatingObject
instances
https://bugs.webkit.org/show_bug.cgi?id=121249

Attachment 211461: Patch
https://bugs.webkit.org/attachment.cgi?id=211461&action=review

------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=211461&action=review


Thanks for working on this Bem!

> Source/WebCore/rendering/FloatingObjects.cpp:170
> +FloatingObject* FloatingObjects::createFloatingObject(RenderBox* renderer)

It doesn't seem like the FloatingObjects itself has anything to do with the
creation of the FloatingObject. Why not just have a static function on the
FloatingObject to do that instead? I think we could use some sort of
OwnPtr/PassOwnPtr for that even if the m_set is not. Add could be changed to
accept a passownptr and "leak" the reference into the m_set.

> Source/WebCore/rendering/FloatingObjects.cpp:175
> +    FloatingObject* newObj = new
FloatingObject(renderer->style()->floating());
> +    newObj->setShouldPaint(!renderer->hasSelfPaintingLayer()); // If a layer
exists, the float will paint itself. Otherwise someone else will.
> +    newObj->setIsDescendant(true);
> +    newObj->setRenderer(renderer);

It appears that all your "add" functions are doing 4 basic steps. I think we
should promote those steps into actual arguments and remove the setters for
values that are immutable.
1. Create the FloatingObject
2. Set the renderer 
3. Set the should paint
4. Set IsDescendant

There are two constructors that take different C++ types for the type of the
float. I would consolidate that and move the EFloat into the caller static
function.
FloatingObject(EFloat type)
FloatingObject(Type type, const LayoutRect& frameRect)

> Source/WebCore/rendering/FloatingObjects.cpp:183
> +    FloatingObject* newFloat = new FloatingObject(overhangingFloat->type(),
LayoutRect(overhangingFloat->frameRect().location() - offset,
overhangingFloat->frameRect().size()));

Both addOverhangingFloat and intrudingFloats use
"LayoutRect(overhangingFloat->frameRect().location() - offset,
overhangingFloat->frameRect().size())". Should we extract that into a "clone"
constructor instead?

FloatingObject* newFloat =
overhangingFloat->overhangingCloneWithOffset(offset);

> Source/WebCore/rendering/FloatingObjects.cpp:190
> +    if (overhangingFloat->renderer()->enclosingFloatPaintingLayer() ==
m_renderer->enclosingFloatPaintingLayer())

I see that you used m_renderer in here. You could pass that to a static
FloatingObject::createOverhaningFloat function instead. This looks like the
first usage of the m_renderer inside the FloatingObjects class. Is there a need
for it? The same for m_horizontalWritingMode. I guess there might be many
"FloatingObjects" instances and there's no reason to keep in memory what we
already have on the stack.

> Source/WebCore/rendering/FloatingObjects.cpp:193
> +	   newFloat->setShouldPaint(false);

We always have only one FloatingObject call the "paint" of the float renderer.
We change the "shouldPaint" directly, but I would rather have a function that
will "delegate" the "paint behavior" from one FloatingObject to the other (ie.
just set false on the previous and true the new one). For example:
overhangingFloat->delegatePaintingTo(newFloat);

Also, both intruding and overhanging floats seem to be better off with
m_shouldPaint set to false by default. This way you can call delegatePaintingTo
only when needed.

> Source/WebCore/rendering/FloatingObjects.cpp:222
> +	   add(floatingObject->clone());

The clone doesn't take care of the "m_shouldPaint" member, meaning that both
the old version and the new version of the FloatingObjects would be possibly be
painting.

This method is only used in moveAllChildrenIncludingFloatsTo and that one is
called just before destroying the "float object donator" RenderBlock. I think
we could most probably figure out a way of just stealing the FloatingObjects
from the old RenderBlock, but before that we could at least name the function
differently. Otherwise, I suppose that we will end up using this method in the
future and have subtle rendering issues with duplicate floats.

> Source/WebCore/rendering/RenderBlock.cpp:3881
> +		       : LayoutSize(logicalTopOffset + (prev != parent() ?
prev->marginTop() : LayoutUnit()), logicalLeftOffset);

I think (prev != parent() ? prev->marginTop() : LayoutUnit() should go to the Y
component instead. You might want to consider going back to the if case and
maybe use something like the IntRect.move function to make it more compact.