[webkit-reviews] review granted: [Bug 122969] Move m_lineBoxes from RenderBlock to RenderBlockFlow : [Attachment 214641] Part 2

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted Sam Weinig <sam at webkit.org>'s
request for review:
Bug 122969: Move m_lineBoxes from RenderBlock to RenderBlockFlow
https://bugs.webkit.org/show_bug.cgi?id=122969

Attachment 214641: Part 2
https://bugs.webkit.org/attachment.cgi?id=214641&action=review

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=214641&action=review


> Source/WebCore/rendering/RenderBlockFlow.cpp:2416
> +// Helper methods for obtaining the last line, computing line counts and
heights for line counts (crawling into blocks).
> +static bool shouldCheckLines(RenderObject* obj)
> +{

The comment seems to be about the next function.

This one could take reference instead of pointer.

> Source/WebCore/rendering/RenderBlockFlow.cpp:2425
> +	   return 0;

nullptr

> Source/WebCore/rendering/RenderBlockFlow.cpp:2431
> +    if (childrenInline()) {
> +	   for (auto box = firstRootBox(); box; box = box->nextRootBox())
> +	       if (!i--)
> +		   return box;
> +    } else {

This could use early return.

> Source/WebCore/rendering/RenderBlockFlow.cpp:2440
> +    return 0;

nullptr

> Source/WebCore/rendering/RenderBlockFlow.cpp:2446
> +	   return 0;

nullptr

> Source/WebCore/rendering/RenderBlockFlow.cpp:2459
> +    if (childrenInline()) {
> +	   for (RootInlineBox* box = firstRootBox(); box; box =
box->nextRootBox()) {
> +	       count++;
> +	       if (box == stopRootInlineBox) {
> +		   if (found)
> +		       *found = true;
> +		   break;
> +	       }
> +	   }
> +    } else {

Early return

>> Source/WebCore/rendering/RenderBlockFlow.cpp:2476
>> +static int getHeightForLineCount(RenderBlockFlow* block, int l, bool
includeBottom, int& count)
> 
> l is incorrectly named. Don't use the single letter 'l' as an identifier
name.  [readability/naming] [4]

const reference

> Source/WebCore/rendering/RenderBlockFlow.cpp:2486
> +    if (block->childrenInline()) {
> +	   for (auto box = block->firstRootBox(); box; box =
box->nextRootBox()) {
> +	       if (++count == l)
> +		   return box->lineBottom() + (includeBottom ?
(block->borderBottom() + block->paddingBottom()) : LayoutUnit());
> +	   }
> +    } else {

Early return

> Source/WebCore/rendering/RenderBlockFlow.cpp:2518
> +    if (childrenInline() && hasMarkupTruncation()) {
> +	   setHasMarkupTruncation(false);
> +	   for (RootInlineBox* box = firstRootBox(); box; box =
box->nextRootBox())
> +	       box->clearTruncation();
> +    } else {

Early return

> Source/WebCore/rendering/RenderBlockFlow.h:301
> +    int lineCount(const RootInlineBox* = 0, bool* = 0) const;

nullptrs