Bug 138346

Summary: Simple line layout: Abstract out content iteration and text handling in general.
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, koivisto, kondapallykalyan, mmaxfield
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description zalan 2014-11-03 19:55:43 PST
This is in preparation to support multiple render elements in simple line layout.
Comment 1 zalan 2014-11-03 20:10:11 PST
Created attachment 240904 [details]
Patch
Comment 2 zalan 2014-11-03 20:14:57 PST
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.
Comment 3 Antti Koivisto 2014-11-04 06:03:59 PST
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.
Comment 4 Antti Koivisto 2014-11-04 06:07:02 PST
LazyLineBreakIterator itself is poorly named. It looks more like a sort of iterator cache.
Comment 5 zalan 2014-11-04 15:22:50 PST
Created attachment 240960 [details]
Patch
Comment 6 Antti Koivisto 2014-11-04 15:41:47 PST
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
Comment 7 Antti Koivisto 2014-11-04 15:44:32 PST
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)
Comment 8 zalan 2014-11-04 16:03:52 PST
Created attachment 240974 [details]
Patch
Comment 9 WebKit Commit Bot 2014-11-04 19:47:40 PST
Comment on attachment 240974 [details]
Patch

Clearing flags on attachment: 240974

Committed r175601: <http://trac.webkit.org/changeset/175601>
Comment 10 WebKit Commit Bot 2014-11-04 19:47:44 PST
All reviewed patches have been landed.  Closing bug.