[webkit-reviews] review granted: [Bug 24168] RTL: Home/End key does not behave correctly in mixed bidi text in RTL document : [Attachment 29756] patch w/ Layout test (version 6)

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted Xiaomei Ji <xji at chromium.org>'s request for review:
Bug 24168: RTL: Home/End key does not behave correctly in mixed bidi text in
RTL document
https://bugs.webkit.org/show_bug.cgi?id=24168

Attachment 29756: patch w/ Layout test (version 6)
https://bugs.webkit.org/attachment.cgi?id=29756&action=review

------- Additional Comments from mitz at webkit.org
> +    if (!rootBox->renderer()->style()->visuallyOrdered()) {

It would be nicer to test for the opposite condition and bail out early if it's
true.

> +	   // Reverse of reordering of the line (L2 according to Bidi spec):
> +	   // L2. From the highest level found in the text to the lowest odd
level on each line,
> +	   // reverse any contiguous sequence of characters that are at that
level or higher.
> +
> +	   // reversing of L2 is only done up to the lowest odd level

That last comment should also be capitalized and end with a period. I don't
understand what "of L2" means in that sentence.

> +	       Vector<InlineBox*>::iterator iter =
leafBoxesInLogicalOrder.begin();

A Vector<InlineBox*>::iterator is just an InlineBox**, so usually we just write
InlineBox**.

> +	       Vector<InlineBox*>::iterator end =
leafBoxesInLogicalOrder.end();

This can go outside the outer while() loop, because it never changes.

> +    size_t index = len - 1;
> +    while (1) {
> +	   endBox = leafBoxesInLogicalOrder[index];
> +	   endNode = endBox->renderer()->node();
> +	   if (endNode || !index)
> +	       return;
> +	   --index;
> +    }
> +}

Please use while (true) instead of while (1). Also, I think it would be nicer
to make this a for() loop, to be more like getLogicalStartBoxAndNode(). You can
do something like

+     for (i = leafBoxesInLogicalOrder.size(); i > 0; --i) {
+	  endBox = leafBoxesInLogicalOrder[i - 1];
	  ...

> +    // Make sure the start of line is not greater than the given input
position.  Else use the previous position to 
> +    // obtain start of line.  This condition happens when the input position
is before the space character at the end 
> +    // of a soft-wrapped non-editable line. In this scenario,
logicalStartPositionForLine would incorrectly hand back a position
> +    // greater than the input position.  This fix is to account for the
discrepancy between lines with webkit-line-break:after-white-space 
> +    // style versus lines without that style, which would break before a
space by default. 
> +    Position p = visPos.deepEquivalent();
> +    if (p.m_offset > c.deepEquivalent().m_offset &&
p.node()->isSameNode(c.deepEquivalent().node())) {
> +	   visPos = c.previous();
> +	   if (visPos.isNull())
> +	       return VisiblePosition();
> +	   visPos = logicalStartPositionForLine(visPos);
> +    }

Since we think this cannot happen, please remove the above from
logicalStartOfLine().

> +	   if (p.node()->renderer() && p.node()->renderer()->isRenderBlock() &&
p.m_offset == 0)

Please write !p.m_offset instead of p.m_offset == 0 (there are two instances of
this).

> +    if (logicalEndNode->hasTagName(brTag)) {
> +	   endOffset = 0;
> +    } else if (logicalEndBox->isInlineTextBox()) {

Please remove the braces around the 1-line if clause.

> +bool inSameLogicalLine(const VisiblePosition &a, const VisiblePosition &b)

The ampersands should go next to the type.

r=me but please make the style changes and consider making the other changes
too before landing.