Bug 188148

Created attachment 346035 [details]
Patch

Comment on attachment 346035 [details]
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.

Committed r234423: <https://trac.webkit.org/changeset/234423>

<rdar://problem/42772230>

(In reply to Antti Koivisto from comment #2)
> Comment on attachment 346035 [details]
> 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.
Good point.
This might get a bit more involved when the iterator gets persistent (to take advantage of the fact that (most of the time)we can just resume it as new floatings are coming in..but for now the simple iterator interface works.

<rdar://problem/42772336>