| Summary: | isStartOfDocument and isEndOfDocument are poorly named | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
| Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | darin, dglazkov, eae, enrica, leviw, mnaganov, ojan, webkit.review.bot, xji | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | 82875 | ||||||||||||
| Bug Blocks: | |||||||||||||
| Attachments: |
|
||||||||||||
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? Created attachment 142833 [details]
Renames isEndOfDocument and adds fixed versions functions
Created attachment 142836 [details]
Removed erroneous change in FrameSelection
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 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 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 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. 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 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 Created attachment 142847 [details]
Fixed builds
For now, I kept selectFrameElementInParentIfFullySelected intact (in fact I'm probably fixing a bug in it). 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. (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. Created attachment 143050 [details]
Put back the missing changes
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? 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. 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? (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. > 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.
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 :) Committed r117813: <http://trac.webkit.org/changeset/117813> |
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.