Bug 86649 – isStartOfDocument and isEndOfDocument are poorly named

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

bool isStartOfDocument(const VisiblePosition &p)
{
    return p.isNotNull() && p.previous().isNull();
}

bool isEndOfDocument(const VisiblePosition &p)
{
    return p.isNotNull() && p.next().isNull();
}

These functions are extremely poorly named. They don't check whether we're at the end of a document at all. They check whether we're at an editing boundary or not.

[Ryosuke Niwa]

I'm fixing isStartOfDocument and isEndOfDocument to cross editing boundary and adding isEndOfEditableOrNonEditableContent which does what isEndOfDocument used to do.

After this change, selectFrameElementInParentIfFullySelected is the only client of isStartOfDocument and isEndOfDocument. The problem is that this function was added in http://trac.webkit.org/changeset/8942 without any tests without any tests. As far as I know, this function had been broken for ages (it only checked if the selection started and ended at the beginning and the end of the current editable region).

So I'm very inclined to just delete it altogether. If anything, we should probably support control selection on iframes per https://bugs.webkit.org/show_bug.cgi?id=12250.

Darin, what is your opinion on this?

[Ryosuke Niwa]

Created attachment 142833 [details]
Renames isEndOfDocument and adds fixed versions functions

[Ryosuke Niwa]

Created attachment 142836 [details]
Removed erroneous change in FrameSelection

[Early Warning System Bot]

Comment on attachment 142836 [details]
Removed erroneous change in FrameSelection

Attachment 142836 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12726614

[WebKit Review Bot]

Comment on attachment 142836 [details]
Removed erroneous change in FrameSelection

Attachment 142836 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12731052

[Gyuyoung Kim]

Comment on attachment 142836 [details]
Removed erroneous change in FrameSelection

Attachment 142836 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12732062

[Ryosuke Niwa]

Comment on attachment 142836 [details]
Removed erroneous change in FrameSelection

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

> Source/WebCore/editing/visible_units.cpp:-1379
> -bool isStartOfDocument(const VisiblePosition &p)
> -{
> -    return p.isNotNull() && p.previous().isNull();
> -}
> -
> -bool isEndOfDocument(const VisiblePosition &p)
> -{
> -    return p.isNotNull() && p.next().isNull();
> -}
> -

How odd. I shouldn't be removing these functions. Will fix.

[Early Warning System Bot]

Comment on attachment 142836 [details]
Removed erroneous change in FrameSelection

Attachment 142836 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12730070

[Build Bot]

Comment on attachment 142836 [details]
Removed erroneous change in FrameSelection

Attachment 142836 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12729275

[Ryosuke Niwa]

Created attachment 142847 [details]
Fixed builds

[Ryosuke Niwa]

For now, I kept selectFrameElementInParentIfFullySelected intact (in fact I'm probably fixing a bug in it).

[Mikhail Naganov]

Comment on attachment 142847 [details]
Fixed builds

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

> Source/WebCore/ChangeLog:10
> +        Also added new versions of isStartOfDocument and isEndOfDocument that correctly cross editing boundaries

Is this true? I don't see the new versions.

[Ryosuke Niwa]

(In reply to comment #12)
> (From update of attachment 142847 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142847&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Also added new versions of isStartOfDocument and isEndOfDocument that correctly cross editing boundaries
> 
> Is this true? I don't see the new versions.

Ugh... my change in isStartOfDocument and isEndOfDocument have been lost.

[Ryosuke Niwa]

Created attachment 143050 [details]
Put back the missing changes

[Levi Weintraub]

Comment on attachment 143050 [details]
Put back the missing changes

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

> Source/WebCore/ChangeLog:8
> +        isEndOfDocument to isEndOfEditableOrNonEditableContent because that's what this function checks.

I believe you're missing the word "Changed" at the beginning :)

> Source/WebCore/ChangeLog:11
> +        Also added new versions of isStartOfDocument and isEndOfDocument that correctly cross editing boundaries
> +        to be used in selectFrameElementInParentIfFullySelected.

Since you didn't create a isStartOfEditableOrNonEditableContent (man that's long) was isStartOfDocument just never used? Is isEndOfDocument still used after this patch?

[Ryosuke Niwa]

Comment on attachment 143050 [details]
Put back the missing changes

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

>> Source/WebCore/ChangeLog:11
>> +        to be used in selectFrameElementInParentIfFullySelected.
> 
> Since you didn't create a isStartOfEditableOrNonEditableContent (man that's long) was isStartOfDocument just never used? Is isEndOfDocument still used after this patch?

isEndOfDocument and isStartOfDocument are still used in selectFrameElementInParentIfFullySelected.
See my comment #1.

[Emil A Eklund]

Comment on attachment 143050 [details]
Put back the missing changes

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

> Source/WebCore/editing/visible_units.cpp:1400
> +bool isEndOfEditableOrNonEditableContent(const VisiblePosition &p)

This is still a pretty poor name. How about isAtEditableBoundary or even isAtEndOfEditableBoundary?

[Ryosuke Niwa]

(In reply to comment #17)
> (From update of attachment 143050 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143050&action=review
> 
> > Source/WebCore/editing/visible_units.cpp:1400
> > +bool isEndOfEditableOrNonEditableContent(const VisiblePosition &p)
> 
> This is still a pretty poor name. How about isAtEditableBoundary or even isAtEndOfEditableBoundary?

isAtEditableBoundary is inaccurate. isAtEndOfEditableBoundary doesn't make sense because an editing boundary is a point in DOM and a point/position doesn't have a start/end.

[Emil A Eklund]

> isAtEditableBoundary is inaccurate. isAtEndOfEditableBoundary doesn't make sense because an editing boundary is a point in DOM and a point/position doesn't have a start/end.

Then what is isEndOfEditableOrNonEditableContent trying to communicate if it isn't a boundary? I'm confused.

[Levi Weintraub]

Comment on attachment 143050 [details]
Put back the missing changes

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

>> Source/WebCore/ChangeLog:8
>> +        isEndOfDocument to isEndOfEditableOrNonEditableContent because that's what this function checks.
> 
> I believe you're missing the word "Changed" at the beginning :)

Make this a sentence.

>>> Source/WebCore/editing/visible_units.cpp:1400
>>> +bool isEndOfEditableOrNonEditableContent(const VisiblePosition &p)
>> 
>> This is still a pretty poor name. How about isAtEditableBoundary or even isAtEndOfEditableBoundary?
> 
> isAtEditableBoundary is inaccurate. isAtEndOfEditableBoundary doesn't make sense because an editing boundary is a point in DOM and a point/position doesn't have a start/end.

I'm not in love with this name, but it's at least accurate in what it portrays :)

[Ryosuke Niwa]

Committed r117813: <http://trac.webkit.org/changeset/117813>