Make FloatingObjects responsible for creating all FloatingObject instances
Created attachment 211461 [details] Patch
Comment on attachment 211461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211461&action=review > Source/WebCore/rendering/FloatingObjects.h:125 > + FloatingObject(EFloat type) Can't we just put the definition into the cpp? It would make the header file more clear. > Source/WebCore/rendering/FloatingObjects.h:143 > + FloatingObject(Type type, const LayoutRect& frameRect) Same. > Source/WebCore/rendering/FloatingObjects.h:158 > + FloatingObject* clone() const Same?
Comment on attachment 211461 [details] Patch 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.
(In reply to comment #2) > (From update of attachment 211461 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211461&action=review > > > Source/WebCore/rendering/FloatingObjects.h:125 > > + FloatingObject(EFloat type) > > Can't we just put the definition into the cpp? It would make the header file more clear. > > > Source/WebCore/rendering/FloatingObjects.h:143 > > + FloatingObject(Type type, const LayoutRect& frameRect) > > Same. > > > Source/WebCore/rendering/FloatingObjects.h:158 > > + FloatingObject* clone() const > > Same? Thanks, will do!
Comment on attachment 211461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211461&action=review Thanks for the feedback! >> 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. Because I would like to get to the point where a FloatingObject knows if it's container has horizontalWritingMode or not, so we don't have to pass them into the logical dimension methods. While I could just have a member of FloatingObject that holds a copy of this information, I would have to walk the entire floating objects set every single time it changes in the parent in order to update that flag. I figured that since FloatingObject is generally not used detached from the FloatingObjects instance, I could have FloatingObjects manage them and then also give them a pointer to their containing FloatingObjects so that each FloatingObject could just get the horizontalWritingMode flag from the FloatingObjects container. (and that flag is already properly updated when the block containing the RenderBlock is updated). Does that make sense? >> Source/WebCore/rendering/FloatingObjects.cpp:175 >> + 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) Can I do this in a future patch? I would like to do the minimal amount of changes to move it out, make sure that works, and then refactor more. >> 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); Yes, we probably can. Can I do this in a future patch as well? >> 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. Yes, I could create a static function to do this, and pass in the renderer (the horizontalWritingMode could be acquired from the passed in renderer as well. However, m_renderer is used in computePlacedFloatsTree(), logicalLeftOffset, logicalRightOffset, and clearLineBoxTreePointers. I have not looked to see where the renderer() method of FloatingObjects is used. I could remove use of m_horizontalWritingMode and replace it will calls to m_renderer->isHorizontalWritingMode(), but I'd also like to do that in a future patch if the memory savings is actually worth it, since that would be a memory/perf tradeoff. Neither of these are new members of FloatingObjects, and were there before it was moved into it's own file. >> 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. I like this, but again would like to add it in a future patch. >> 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. I'll fix that bug. As for stealing the FloatingObjects, when writing moveAllChildrenIncludingFloatsTo, I couldn't come up with a reasonable way to do that (neither could my reviewer at the time). Cloning seemed much less accident prone, so we decided to stick with that. Unless you have a simple way to do the stealing, I'd be skeptical that it's worth it. >> 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. You're right, I got that wrong. I'll also take a look at IntRect.move.
Based on Alex's feedback, I change my approach completely. The new patch is Bug 121323.