[webkit-reviews] review granted: [Bug 188148] [LFC][Floating] Add basic left/right floating positioning. : [Attachment 346035] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted zalan <zalan at apple.com>'s request
for review:
Bug 188148: [LFC][Floating] Add basic left/right floating positioning.
https://bugs.webkit.org/show_bug.cgi?id=188148

Attachment 346035: Patch

https://bugs.webkit.org/attachment.cgi?id=346035&action=review




--- Comment #2 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 346035
  --> https://bugs.webkit.org/attachment.cgi?id=346035
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346035&action=review

> Source/WebCore/layout/FloatingContext.cpp:92
> +class Iterator {
> +public:
> +    Iterator(const LayoutContext&, const FloatingState&);
> +
> +    void set(LayoutUnit verticalPosition);
> +    const FloatingPair& current() const { return m_current; }
> +    LayoutUnit verticalPosition() const { return m_verticalPosition; }
> +    Iterator& operator++();
> +
> +private:
> +    const LayoutContext& m_layoutContext;
> +    const FloatingState& m_floatingState;
> +    FloatingPair m_current;
> +    LayoutUnit m_verticalPosition;
> +};

It would be slightly nicer to give this the full iterator interface (basically
add operator== and operator*) so it can be used in range-for loops etc.

> Source/WebCore/layout/FloatingContext.cpp:94
> +
> +

Extra empty line.

> Source/WebCore/layout/FloatingContext.cpp:121
> +    Iterator iterator(layoutContext(), m_floatingState);
> +    // Start with the inner-most floating pair for the initial vertical
position.
> +    iterator.set(initialVerticalPosition);

These could be factored to a begin() style function that returns the iterator.