Bug 70664 – positionAvoidingFirstPositionInTable seems unnecessary

• Component: HTML Editing
• Status: RESOLVED INVALID

[Ryosuke Niwa]

It appears that positionAvoidingFirstPositionInTable doesn't do anything. All editing tests pass without calling this function, and the bug it claimed to fix (http://trac.webkit.org/changeset/25586) doesn't reproduce without calling this function.

We've probably fixed the same problem elsewhere, not realizing this has become a dead code.

[Ryosuke Niwa]

Created attachment 112058 [details]
Removes the function

[Antonio Gomes]

Comment on attachment 112058 [details]
Removes the function

View in context: https://bugs.webkit.org/attachment.cgi?id=112058&action=review

> Source/WebCore/editing/visible_units.cpp:-326
> -    // return table offset 0 instead of the first VisiblePosition inside the table
> -    VisiblePosition previous = c.previous();
> -    if (isLastPositionBeforeTable(previous) && isEditablePosition(previous.deepEquivalent()))
> -        return previous;

rniwa, could you explain that this block is dead code in the changelog?

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 112058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112058&action=review
> 
> > Source/WebCore/editing/visible_units.cpp:-326
> > -    // return table offset 0 instead of the first VisiblePosition inside the table
> > -    VisiblePosition previous = c.previous();
> > -    if (isLastPositionBeforeTable(previous) && isEditablePosition(previous.deepEquivalent()))
> > -        return previous;
> 
> rniwa, could you explain that this block is dead code in the changelog?

Will do.

[Ryosuke Niwa]

Comment on attachment 112058 [details]
Removes the function

View in context: https://bugs.webkit.org/attachment.cgi?id=112058&action=review

>>> Source/WebCore/editing/visible_units.cpp:-326
>>> -        return previous;
>> 
>> rniwa, could you explain that this block is dead code in the changelog?
> 
> Will do.

Wait... I've already said "Remove positionAvoidingFirstPositionInTable *because it appears to be dead code*."

What do you want me to add?

[Kent Tamura]

Comment on attachment 112058 [details]
Removes the function

View in context: https://bugs.webkit.org/attachment.cgi?id=112058&action=review

>>>> Source/WebCore/editing/visible_units.cpp:-326
>>>> -        return previous;
>>> 
>>> rniwa, could you explain that this block is dead code in the changelog?
>> 
>> Will do.
> 
> Wait... I've already said "Remove positionAvoidingFirstPositionInTable *because it appears to be dead code*."
> 
> What do you want me to add?

You think this function always returns 'c', don't you?  So, you had better explain why this function doesn't return 'previous'.

[Darin Adler]

Comment on attachment 112058 [details]
Removes the function

View in context: https://bugs.webkit.org/attachment.cgi?id=112058&action=review

>>>>> Source/WebCore/editing/visible_units.cpp:-326
>>>>> -        return previous;
>>>> 
>>>> rniwa, could you explain that this block is dead code in the changelog?
>>> 
>>> Will do.
>> 
>> Wait... I've already said "Remove positionAvoidingFirstPositionInTable *because it appears to be dead code*."
>> 
>> What do you want me to add?
> 
> You think this function always returns 'c', don't you?  So, you had better explain why this function doesn't return 'previous'.

I believe Ryosuke’s argument is that something preserves the high level semantics of editing. I do not think he is saying that the functions here in visible_units.cpp still all behave the same. If we had unit tests for them, we probably would have to change those tests.

[Antonio Gomes]

(In reply to comment #6)
> (From update of attachment 112058 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112058&action=review
> 
> >>>>> Source/WebCore/editing/visible_units.cpp:-326
> >>>>> -        return previous;
> >>>> 
> >>>> rniwa, could you explain that this block is dead code in the changelog?
> >>> 
> >>> Will do.
> >> 
> >> Wait... I've already said "Remove positionAvoidingFirstPositionInTable *because it appears to be dead code*."
> >> 
> >> What do you want me to add?
> > 
> > You think this function always returns 'c', don't you?  So, you had better explain why this function doesn't return 'previous'.
> 
> I believe Ryosuke’s argument is that something preserves the high level semantics of editing. I do not think he is saying that the functions here in visible_units.cpp still all behave the same. If we had unit tests for them, we probably would have to change those tests.

This is exactly what I would say could get clarified in the changelog.

[Ryosuke Niwa]

Comment on attachment 112058 [details]
Removes the function

It turned out that this function IS needed. It's just that there are no tests and endOfLine has a bug this function fixes for startOfLine.

[Ryosuke Niwa]

It turned out that this function is not necessary, it's causing a bug. Filed https://bugs.webkit.org/show_bug.cgi?id=70757 to fix it.