Bug 138346 – Simple line layout: Abstract out content iteration and text handling in general.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

This is in preparation to support multiple render elements in simple line layout.

[zalan]

Created attachment 240904 [details]
Patch

[zalan]

I am not too happy about the RenderFlowContentIterator name. The class does a lot more than just iterating.
Alternatively we could move some of the (non-iterating)class functions back to static inline and pass the RenderFlowContentIterator object around.

[Antti Koivisto]

Comment on attachment 240904 [details]
Patch

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

Looks like right direction.

> Source/WebCore/rendering/SimpleLineLayout.cpp:216
> +template <typename CharacterType> class RenderFlowContentIterator {

I think we usually put template arguments to a line of their own for classes.

ContentIterator? FlowIterator? 

Actually this class doesn't really feel like an iterator at all. If I understand correctly it doesn't hold the current position and doesn't change state during traversal. Maybe it should? Or maybe it should just be named differently (FlowContents or similar perhaps)?

> Source/WebCore/rendering/SimpleLineLayout.cpp:225
> +    unsigned findNextBreakablePosition(unsigned position)

I think this is logically const too though LazyLineBreakIterator interface currently requires non-const call.

[Antti Koivisto]

LazyLineBreakIterator itself is poorly named. It looks more like a sort of iterator cache.

[zalan]

Created attachment 240960 [details]
Patch

[Antti Koivisto]

Comment on attachment 240960 [details]
Patch

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

> Source/WebCore/rendering/SimpleLineLayout.cpp:444
> +    const Style& style = contentIterator.style();

Could use auto&

> Source/WebCore/rendering/SimpleLineLayout.cpp:551
> +    const Style style = contentIterator.style();

This can be reference: auto&

> Source/WebCore/rendering/SimpleLineLayout.cpp:678
> +    FlowContentIterator<CharacterType> contentIterator = FlowContentIterator<CharacterType>(flow);

This could use auto

[Antti Koivisto]

Comment on attachment 240960 [details]
Patch

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

>> Source/WebCore/rendering/SimpleLineLayout.cpp:678
>> +    FlowContentIterator<CharacterType> contentIterator = FlowContentIterator<CharacterType>(flow);
> 
> This could use auto

Actually this could just be FlowContentIterator<CharacterType> contentIterator(flow)

[zalan]

Created attachment 240974 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 240974 [details]
Patch

Clearing flags on attachment: 240974

Committed r175601: <http://trac.webkit.org/changeset/175601>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.