[webkit-reviews] review denied: [Bug 24168] RTL: Home/End key does not behave correctly in mixed bidi text in RTL document : [Attachment 29402] patch w/ Layout test

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied 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 29402: patch w/ Layout test
https://bugs.webkit.org/attachment.cgi?id=29402&action=review

------- Additional Comments from mitz at webkit.org
I think you are right: you should change the backward/forward, not the
left/right variant, and make the home/end keys map to back/forward.

Please use "logical" instead of the abbreviation "logic".

A typical line contains very few leaf boxes (typically 1, perhaps 2 for
contenteditable text with line-break: after-white-space, but hardly ever more
than 10), so reconstructing the logical order every time for an operation like
home/end is not a problem. Adding two pointer-sized members to InlineBox is not
necessary, and has a severe impact on memory use.

I think it would actually be simpler to use a Vector<InlineBox*>, fill it up
with the boxes in visual order (i.e. in nextLeafChild() order), then do the
reordering in-place in the vector.

You could have one

static void getLeafBoxesInLogicalOrder(RootInlineBox*, Vector<InlineBox*>&)

that did that. You wouldn't even need a helper function for reversing because
you could use std::reverse().

+    unsigned char levelLow = 128;
+    unsigned char levelHigh = 0;

I know that the names levelLow and levelHigh are used in BidiResolver, but
those are old and rather unusual names. minLevel and maxLevel would be better.

+    InlineBox* firstBox = NULL;
+    InlineBox* lastBox = NULL;

This will go away if you use a Vector-based getLeafBoxesInLogicalOrder(), but I
just wanted to point out that WebKit style is to use 0, not NULL, for the null
pointer, and that since you follow with a call to reconstructLogicOrder(),
which initializes firstBox and lastBox, you do not need to initialize them when
defining them anyway.

+    while (1) {
+	 if (!startBox)
+	     return;

Since there is nothing after the while loop, this can be written simply as
+    while (startBox)

+	 RenderObject *startRenderer = startBox->renderer();

The star should go next to RenderObject.

+	 if (!startRenderer) {
+	     startNode = NULL;
+	     return;
+	 }

Can this ever happen? I know that there is other old code checking for this,
but I don't think this case can happen. Maybe the code should assert that it
doesn't happen. Again, if you leave this code in, please use 0 instead of NULL.


+    RootInlineBox *rootBox = rootBoxForLine(c);

The start should go next to the type name.

+    // Generated content (e.g. list markers and CSS :before and :after
+    // pseudoelements) have no corresponding DOM element, and so cannot be
+    // represented by a VisiblePosition.  Use whatever follows instead.

I don't understand this comment. Where is the part where you "use whatever
follows"? I think it is actually part of getLogicStartBoxAndNode() now!

+    InlineBox *logicStartBox = NULL;
+    Node *logicStartNode = NULL;

I think the rule should be that getLogicStartBoxAndNode() initializes those.
Also, please correct the placement of the stars.

+    if (logicStartBox == NULL || logicStartNode == NULL) {
+	 return VisiblePosition();
+    }

The WebKit style of writing this is
+    if (!logicStartBox || !logicStartNode)
+	 return VisiblePosition();

without NULL or 0 and without braces around the single-line clause. Also,
shouldn't it be enough to only null-test the node?

+    int startOffset = 0;
+    if (logicStartBox->isInlineTextBox()) {
+	 InlineTextBox *startTextBox = static_cast<InlineTextBox
*>(logicStartBox);
+	 startOffset = startTextBox->start();
+    }

I think you can use InlineBox::caretMinOffset() here instead of special-casing
InlineTextBox.

+VisiblePosition logicStartOfLine(const VisiblePosition& c)
+{
+    VisiblePosition visPos = logicStartPositionForLine(c);
+    
+    if (visPos.isNotNull()) {

You can change this to an early return.

+    int endOffset = 1;
+    if (logicEndNode->hasTagName(brTag)) {
+	 endOffset = 0;
+    } else if (logicEndBox->isInlineTextBox()) {
+	 InlineTextBox *endTextBox = static_cast<InlineTextBox *>(logicEndBox);

+	 endOffset = endTextBox->start();
+	 if (!endTextBox->isLineBreak())
+	     endOffset += endTextBox->len();
+    }

And here you can use InlineBox::caretMaxOffset() in the else clause, instead of
special-casing InlineTextBox. Can you use caretMaxOffset() to get the desired
result for a <br> too?

+bool inSameLogicLine(const VisiblePosition &a, const VisiblePosition &b)

Can inSameLogicLine(a, b) and inSameLine(a, b) ever return a different result?

Finally, I want to ask about thse two cases:

+	 // 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,
logicStartPositionForLine 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())) {

and

+    // Make sure the end of line is at the same line as the given input
position.  Else use the previous position to 
+    // obtain end 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,
logicEndPositionForLine would incorrectly hand back a position
+    // in the next line instead. 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. 
+    if (!inSameLogicLine(c, visPos)) {

I don't fully understand the former and I am not sure the latter can happen.
Are they covered by your test? Do we need them?

+//	       positions.push({ node: sel.anchorNode, offset:
sel.anchorOffset});

+ //	       positions.push({ node: sel.anchorNode, offset:
sel.anchorOffset});

Please remove commented-out code from the test.

Property changes on: LayoutTests/editing/selection/home-end.html
___________________________________________________________________
Name: svn:executable
   + *

This is unnecessary.

Looks like you are in the right direction, but you can do the same with simpler
code!