[Webkit-unassigned] [Bug 9272] Left/Right borders/padding/margins are not always added correctly when rendering multiline inline boxes with bidi elements

[bugzilla-daemon at webkit.org]

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


Eric Seidel <eric at webkit.org> changed:

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




--- Comment #17 from Eric Seidel <eric at webkit.org>  2011-03-29 01:31:13 PST ---
(From update of attachment 87274)
View in context: https://bugs.webkit.org/attachment.cgi?id=87274&action=review

It's hard to tell if this is right because the code isn't self-documenting yet.  I think we need another round of cleanup before we can land this.  See possible suggestinos below.

> Source/WebCore/rendering/InlineFlowBox.cpp:237
>          if (!lineBoxList->lastLineBox()->isConstructed()) {

I think once we land this patch we should land a cleanup patch which turns this whole big if block into a helper function which returns includeRightEdge and includeLeftEdge as reference variables.  Then it's easy to early return instead of making these nested ifs.

> Source/WebCore/rendering/InlineFlowBox.cpp:245
> +            RenderObject* object = logicallyLastRunRenderer->parent();
> +            while (object && !object->isRenderBlock()) {
> +                if (object == renderer())
> +                    break;
> +                object = object->parent();
> +            }

I guess this is "isAncestorWithoutInterveningBlock"? maybe?  There is an isAncestorOf() function already.  I guess you don't really need the isRenderBlock() check, except for speed.  I think we should still consider breaking this out into a well named fucntion since it's not clear what it does.  We could explainit in a comment, but it's betetr to make nicely named functions than add comments.

> Source/WebCore/rendering/InlineFlowBox.cpp:248
> +                bool shouldIncludeEdge = lastLine || object != renderer();

Yeah, this is just confusing.  You should put the object != render check in a local bool, or better yet, just put this all in a nicely named helper.  Also we don't need to do the walk unless we get into this if anyway.

Should includeEdge is not the right name for this bool either.  The whole decision you're making is about including eht edge.  This specific bool is only part of that check.  Or?

If we convert this to be a helper fucntion with early return, than it's easy to check if (!shouldIncludeEdge) and return early! :)

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