Bug 27154 – useless null-check statement in visible_units.cpp@logicalStartOfLine

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Antonio Gomes]

quoted code below and bug summary are self explanatory. 


(...)
VisiblePosition logicalStartOfLine(const VisiblePosition& c)
{
   VisiblePosition visPos = logicalStartPositionForLine(c);

   if (visPos.isNull())
       return c.honorEditableBoundaryAtOrAfter(visPos);

   return c.honorEditableBoundaryAtOrAfter(visPos);
}
(...)


honorEditableBoundaryAtOrAfter is called regardless the null check and possibly (wrongly) twice

[Ryosuke Niwa]

It seems like the only difference between logicalStartOfLine and logicalStartPositionForLine is that we call honorEditableBoundaryAtOrAfter for logicalStartOfLine.  But because there is no mentioning of editable boundaries in the function, it's utterly confusing.  We should fix that.

[Justin Garcia]

>    if (visPos.isNull())
>        return c.honorEditableBoundaryAtOrAfter(visPos);
> 
>    return c.honorEditableBoundaryAtOrAfter(visPos);

Hmm strange.  Does logicalEndOfLine have this problem as well?

Justin

[Antonio Gomes]

Created attachment 32693 [details]
simple fix: statement removed

[Ryosuke Niwa]

logicalStartPositionForLine is only used in logicalStartOfLine.  I don't understand what was the reason for making two functions, one of which is only used in the other function.  It seems like this function is added in https://trac.webkit.org/changeset/43032.  Maybe that the person intended to remove logicalStartOfLine later?

[Ryosuke Niwa]

(In reply to comment #2)
> Hmm strange.  Does logicalEndOfLine have this problem as well?
> 
> Justin

logicalEndOfLine is slightly better:

    VisiblePosition visPos = logicalEndPositionForLine(c);
   
    // Make sure the end of line is at the same line as the given input position. For a wrapping line, the logical end
    // position for the not-last-2-lines might incorrectly hand back the logical beginning of the next line. 
    // For example, <div contenteditable dir="rtl" style="line-break:before-white-space">abcdefg abcdefg abcdefg
    // a abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg abcdefg </div>
    // In this case, use the previous position of the computed logical end position.
    if (!inSameLogicalLine(c, visPos))
        visPos = visPos.previous();
    
    return c.honorEditableBoundaryAtOrBefore(visPos);

But logicalEndPositionForLine isn't used anywhere else either.

[Ryosuke Niwa]

(In reply to comment #3)
> Created an attachment (id=32693) [details]
> simple fix: statement removed

Thank you for posting a patch, but I'd like to investigate more about these functions.  It seems like we don't even need logicalStartPositionForLine (we can put everything inside logicalStartOfLine).  I have added the author of the changeset 43032.  Hopefully he can give us some feedback on this.

[Xiaomei Ji]

Thanks for identifying and working on this bug!

logicalStartPositionForLine/logicalStartOfLine and 
logicalEndPositionForLine/logicalEndOfLine are introduced when fixing issue 24168
(RTL: Home/End key does not behave correctly in mixed bidi text in RTL document ).

Since Home/End key are logical keys, those logical operations are introduced, while the original startPositionForLine/startOfLine/endPositionForLine/endOfLine are preserved for visual operations.

I think it is my bug having the following code in logicalStartOfLine (although honorEditableBoundaryAtOrAfter wont be called twice).
if (visPos.isNull())
       return c.honorEditableBoundaryAtOrAfter(visPos);
return c.honorEditableBoundaryAtOrAfter(visPos);

Antonio Gomes's patch should fix it.

The structure of logicalStartOfLine was similar to that of startOfLine() which have "if (visPos.isNotNull())". But during the testing, I did not find any cases where the above condition is true, so, I removed it (see the last 4th review feedback in comments #20 of issue 24168). I guess the above mistake was introduced when I removed it (should remove the if block completely).

The structure of logicalEndOfLine is slightly different because I did find cases where the returning position from logicalEndPositionForLine() need to be adjusted (see comments in the code).

I am leaving the logicalStartPositionForLine and logicalStartOfLine (same for end of line) as separate functions to follow the pattern of startPositionForLine and startOfLine. I prefer to leave it as is (although logicalStartOfLine() is pretty simple right now), but I do not have strong opinion about it. Maybe mitz has better suggestions.

I did not replace any of the startPositionForLine/startOfLine/endPositionForLine/endofLine simply because they are visual operations and are used in several other places. I do not have enough knowledge of whether those functionalities are correct or not and whether their usages are correct or not.

[Brent Fulgham]

Landed in http://trac.webkit.org/changeset/45926.