[webkit-reviews] review denied: [Bug 9272] Left/Right borders/padding/margins are not always added correctly when rendering multiline inline boxes with bidi elements : [Attachment 87274] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Yael <yael.aharon at nokia.com>'s request
for review:
Bug 9272: Left/Right borders/padding/margins are not always added correctly
when rendering multiline inline boxes with bidi elements
https://bugs.webkit.org/show_bug.cgi?id=9272

Attachment 87274: Patch
https://bugs.webkit.org/attachment.cgi?id=87274&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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! :)