[webkit-reviews] review denied: [Bug 33266] WebCore::InlineFlowBox::determineSpacingForFlowBoxes ReadAV at NULL (43c64e8abbda6766e5f5edbd254c2d57) : [Attachment 46665] patch - cleaned up version of previous patch, even more layout tests

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Roland Steiner <rolandsteiner at chromium.org>'s
request for review:
Bug 33266: WebCore::InlineFlowBox::determineSpacingForFlowBoxes ReadAV at NULL
(43c64e8abbda6766e5f5edbd254c2d57)
https://bugs.webkit.org/show_bug.cgi?id=33266

Attachment 46665: patch - cleaned up version of previous patch, even more
layout tests
https://bugs.webkit.org/attachment.cgi?id=46665&action=review

------- Additional Comments from mitz at webkit.org
This looks okay.

> +	   Ruby did not handle mal-formed cases correctly when the ruby base
was in

Typo. Should be “malformed”.

> +void RenderBlock::moveAllChildrenTo(RenderObject* to, RenderObjectChildList*
toChildList)
> +{
> +    for (RenderObject* child = children()->firstChild(); child; child =
children()->firstChild())

Existing loops that iterate over children while removing them store the pointer
to the current child’s next sibling before moving it, then make the stored
sibling the current child. I wonder if that’s more efficient that calling
children()->firstChild().

> +    // RenderRubyBase needs to do some complex block manipulation.
> +    friend class RenderRubyBase;

I wish this could be avoided. And the comment isn’t helpful.

> +    if (childrenInline())
> +	   moveChildrenFromInlineFlow(toBase, beforeChild);
> +    else
> +	   moveChildrenFromBlockFlow(toBase, beforeChild);

Following the naming scheme used in RenderBlock, I would name these methods
moveInlineChildren() and moveBlockChildren(). This is always a block flow.

> +	   // If toBase has a suitable block, we re-use it, otherwise create a
new one.
> +	   RenderBlock* block;
> +	   RenderObject* lastChild = toBase->lastChild();
> +	   if (lastChild && lastChild->isAnonymousBlock() &&
lastChild->childrenInline()) {
> +	       block = toRenderBlock(lastChild);
> +	   } else {

Braces around a single-line clause.

Should you check that !lastChild->parent()->createsAnonymousWrapper()? Can you
assert that?

> +	   for (RenderObject* child = firstChild(); child != beforeChild; child
= firstChild())
> +	       moveChildTo(block, block->children(), child);

Perhaps you can unify this loop with the similar loop at the end of the “if”
clause.

> +    if (beforeChild != firstChild()) {

You can rewrite this with an early return.

> +	   if (firstChildHere && firstChildHere->isAnonymousBlock() &&
firstChildHere->childrenInline() && 
> +	       lastChildThere && lastChildThere->isAnonymousBlock() &&
lastChildThere->childrenInline() ) {

Please move the && to the beginning of the second line, and remove the space
before the last parenthesis.

> +	       for (RenderObject* child = anonBlockHere->firstChild(); child;
child = anonBlockHere->firstChild())
> +		   anonBlockHere->moveChildTo(anonBlockThere,
anonBlockThere->children(), child);

This looks like moveAllChildrenTo().

r- because of the createsAnonymousWrapper() question, function naming (using
“block flow” and “inline flow” instead of “block children” and “inline
children”) and the style issues.