[Webkit-unassigned] [Bug 55440] improve layout performance by reducing the traversal time of the floating objects

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=55440


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #84547|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #10 from Darin Adler <darin at apple.com>  2011-03-03 11:23:14 PST ---
(From update of attachment 84547)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.