[Webkit-unassigned] [Bug 24168] RTL: Home/End key does not behave correctly in mixed bidi text in RTL document

[bugzilla-daemon at webkit.org]

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


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #29756|review?                     |review+
               Flag|                            |




------- Comment #20 from mitz at webkit.org  2009-04-29 10:58 PDT -------
(From update of attachment 29756)
> +    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.


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