[Webkit-unassigned] [Bug 33266] WebCore::InlineFlowBox::determineSpacingForFlowBoxes ReadAV at NULL (43c64e8abbda6766e5f5edbd254c2d57)

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=33266


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #46665|review?                     |review-
               Flag|                            |




--- Comment #7 from mitz at webkit.org  2010-01-18 23:04:05 PST ---
(From update of attachment 46665)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.