[webkit-reviews] review denied: [Bug 71194] paragraphs with different directionality in textarea with unicode-bidi: plaintext are aligned the same : [Attachment 224501] Patch proposal + updated expectations for Mac-mountainlion

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Mario Sanchez Prada
<mario at webkit.org>'s request for review:
Bug 71194: paragraphs with different directionality in textarea with
unicode-bidi: plaintext are aligned the same
https://bugs.webkit.org/show_bug.cgi?id=71194

Attachment 224501: Patch proposal + updated expectations for Mac-mountainlion
https://bugs.webkit.org/attachment.cgi?id=224501&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=224501&action=review


r- just for the FIXME comment that I'd like to see.

Note this code could in theory work without needing a root line box (for the
one place that did not get patched properly). If you knew the bidi level, you
can compute everything else, since the style is the RenderBlockFlow's style.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:580
> +    TextDirection direction;
> +    if (rootInlineBox && rootInlineBox->renderer().style().unicodeBidi() ==
Plaintext)
> +	   direction = rootInlineBox->direction();
> +    else
> +	   direction = style().direction();

The renderer for a root line box is the RenderBlockFlow, so I think this would
read better as:

if (rootInlineBox && style().unicodeBidi() == Plaintext)

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:2175
> -    updateLogicalWidthForAlignment(textAlign, 0, logicalLeft,
totalLogicalWidth, availableLogicalWidth, 0);
> +    updateLogicalWidthForAlignment(textAlign, 0, 0, logicalLeft,
totalLogicalWidth, availableLogicalWidth, 0);

This needs a FIXME, since it's obviously wrong. You pass no root line box here.
All you need is the hypothetical bidi level that would be used to construct a
line here. I would think that could be determined.