Bug 27154

Summary: useless null-check statement in visible_units.cpp@logicalStartOfLine
Product: WebKit Reporter: Antonio Gomes <tonikitoo>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: bfulgham, eric, jparent, justin.garcia, ojan, rniwa, xji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Description Flags
simple fix: statement removed darin: review+

Description Antonio Gomes 2009-07-10 12:44:57 PDT
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
Comment 1 Ryosuke Niwa 2009-07-13 18:44:33 PDT
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.
Comment 2 Justin Garcia 2009-07-13 18:47:26 PDT
>    if (visPos.isNull())
>        return c.honorEditableBoundaryAtOrAfter(visPos);
>    return c.honorEditableBoundaryAtOrAfter(visPos);

Hmm strange.  Does logicalEndOfLine have this problem as well?

Comment 3 Antonio Gomes 2009-07-13 18:54:22 PDT
Created attachment 32693 [details]
simple fix: statement removed
Comment 4 Ryosuke Niwa 2009-07-13 19:31:45 PDT
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?
Comment 5 Ryosuke Niwa 2009-07-13 19:35:25 PDT
(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.
Comment 6 Ryosuke Niwa 2009-07-13 19:42:05 PDT
(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.
Comment 7 Xiaomei Ji 2009-07-14 10:26:03 PDT
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.
Comment 8 Brent Fulgham 2009-07-15 11:07:46 PDT
Landed in http://trac.webkit.org/changeset/45926.