[webkit-reviews] review denied: [Bug 55440] improve layout performance by reducing the traversal time of the floating objects : [Attachment 84547] updated patch addressing comment #8

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Yuqiang Xian
<yuqiang.xian at intel.com>'s request for review:
Bug 55440: improve layout performance by reducing the traversal time of the
floating objects
https://bugs.webkit.org/show_bug.cgi?id=55440

Attachment 84547: updated patch addressing comment #8
https://bugs.webkit.org/attachment.cgi?id=84547&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=84547&action=review

Seems like a good approach, but needs some style fixes.

> Source/WebCore/rendering/RenderBlock.h:716
> -    OwnPtr<FloatingObjectSet> m_floatingObjects;
> +    struct FloatingObjects {
> +	   FloatingObjects()

I don’t think the functions in this class should all be defined inside the
class definition. That just makes the class harder to read. I suggest putting
inline function bodies elsewhere. Since they are only used in the one .cpp
file, they can be inside the .cpp file and not in the header at all.

> Source/WebCore/rendering/RenderBlock.h:723
> +	   virtual ~FloatingObjects() {ASSERT(m_floatingObjectSet); delete
m_floatingObjectSet;}

It is incorrect to mark this virtual. Makes the entire object bigger and makes
calling the destructor slower for no reason.

And you won’t need this destructor if you take my advice below about
m_floatingObjectSet.

Formatting without spaces is wrong for our project.

> Source/WebCore/rendering/RenderBlock.h:729
> +	       m_leftFloatsCount = 0;

I don’t think the same class should call floating objects both “floating
objects” and “floats”. We should choose one name or the other.

> Source/WebCore/rendering/RenderBlock.h:741
> +	   void increaseFloatsCount(FloatingObject::Type type)
> +	   {
> +	       type == FloatingObject::FloatLeft ? m_leftFloatsCount++ :
m_rightFloatsCount++;
> +	   }
> +	   void decreaseFloatsCount(FloatingObject::Type type)
> +	   {
> +	       type == FloatingObject::FloatLeft ? m_leftFloatsCount-- :
m_rightFloatsCount--;
> +	       ASSERT(m_leftFloatsCount >= 0 && m_rightFloatsCount >= 0);
> +	   }

I think these would read much better with if statements or switch statements.
Using a ? : expression for its side effects is not a conventional readable
coding style.

> Source/WebCore/rendering/RenderBlock.h:743
> +	   bool hasLeftFloats() {return m_leftFloatsCount > 0;}

WebKit style puts spaces after these.

> Source/WebCore/rendering/RenderBlock.h:746
> +	   FloatingObjectSet* m_floatingObjectSet;

This should be a set, not a pointer to a set. If it was a pointer it should be
OwnPtr.

This should probably be called just m_set. It’s already in a class named
FloatingObjects.

Data members of a struct should not have the m_ prefix. We use those only for
private data members in classes, not public data members like these. Maybe this
should be a class. I’m not sure clients need direct access to the floating
object counts.

> Source/WebCore/rendering/RenderBlock.h:748
> +	   int m_leftFloatsCount;
> +	   int m_rightFloatsCount;

These should be unsigned integers, not signed. And maybe private as I mentioned
above.

> Source/WebCore/rendering/RenderBlock.h:751
> +    FloatingObjectSet* getFloatingObjectSet() const {return
m_floatingObjects ? m_floatingObjects->m_floatingObjectSet : 0;}

In the WebKit project we don’t use get in the function names of this type of
function.

Further, the callers of this function all already have explicit checks to see
if m_floatingObjects is 0, so you are adding a double check for null
everywhere. I think you should probably dump this function and just use:

    m_floatingObjects->set()

everwherer. Or m_floatingObjects->set if you keep the set a public data member.