Filing for future yak shaving opportunities. In fixing bug 8931, an extra instance of SelectionController was added to Page, specifically to track the caret when dragging text into a subframe. The commit had this useful comment: // FIXME: Replace this with a function on the selection controller or change it to Selection instead? This comment was removed in future refactoring, which is sad. Over time, the relationship between this instance and the one on Frame grew complicated. We should come back to this, untangle and remove the extra SelectionController instance on Page.
We should definitely do this.
No one should have to see their FIXME's pop back up over 5 years from writing them... No one!
Created attachment 92503 [details] work in progress Here's my work-in-progress patch. I split FrameSelection into 3 classes: CaretBase, DragCaretController, and FrameSelection. CaretBase is the base class for DragCaretController and FrameSelection, which is responsible for keeping the states for and painting the caret. Ideally, DragCaretController and FrameSelection will have CaretBase internally rather than inheriting from it but that should probably be done in a separate patch.
Attachment 92503 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.exp.in', u'Source/W..." exit_code: 1 Source/WebCore/editing/FrameSelection.h:58: The parameter name "type" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/editing/FrameSelection.h:62: The parameter name "view" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/editing/FrameSelection.h:65: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/editing/FrameSelection.h:86: The parameter name "frame" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/editing/FrameSelection.h:86: The parameter name "p" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 5 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 92518 [details] cleanup
Anyone interested in reviewing this?
Comment on attachment 92518 [details] cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=92518&action=review I’m going to say review- because of my concerns about DragCaretController::nodeWillBeRemoved. > Source/WebCore/editing/FrameSelection.cpp:241 > +void DragCaretController::nodeWillBeRemoved(Node* node) Where is the call to this function? > Source/WebCore/editing/FrameSelection.cpp:246 > + if (node && highestAncestor(node)->nodeType() == Node::DOCUMENT_FRAGMENT_NODE) Seems much better to check if the highest ancestor is the document rather than specifically checking that it's a document fragment. I’m also wondering how the result of this would differ from the much faster inDocument check, which is not O(height) of the DOM tree. On the other hand, the drag caret is almost always non-visible, so the performance of this code will probably rarely be important. > Source/WebCore/editing/FrameSelection.cpp:249 > + if (removingNodeRemovesPosition(node, m_position.deepEquivalent())) { We use early return for the other two conditions in this function. I suggest using it for this one too. > Source/WebCore/editing/FrameSelection.cpp:253 > + RefPtr<Document> document = m_position.deepEquivalent().anchorNode()->document(); > + document->updateStyleIfNeeded(); > + if (RenderView* view = toRenderView(document->renderer())) > + view->clearSelection(); I'd like to see this sharing code with FrameSelection::respondToNodeModification instead of having a copied and pasted version of the sequence. > Source/WebCore/editing/FrameSelection.cpp:1120 > + if (!node) > + return false; > + return !isTableElement(node) && !editingIgnoresContent(node); I think it would read better as a single return; seems you just moved the code, but still I would like that better. > Source/WebCore/editing/FrameSelection.h:54 > +protected: Nothing private? That seems like a mistake. Can anything be private? I think we absolutely should be making the data members private instead of protected, even if that does mean more code changes. Maybe in a followup? > Source/WebCore/editing/FrameSelection.h:90 > + bool isNone() const { return m_position.isNull(); } I think this is an awkward function name. In the case of a selection where it can be none, caret, or range, this is barely tolerable, but in a caret class where it’s either a caret or no caret, “none” is a curious way to talk about things. > Source/WebCore/editing/FrameSelection.h:93 > + const VisiblePosition& caret() { return m_position; } > + void setCaret(const VisiblePosition&); In CaretBase functions, this is called “the caret position”, but here you are calling it “the caret”. I think we should use consistent terminology. > Source/WebCore/editing/FrameSelection.h:97 > +private: Please put a blank line before private:
Comment on attachment 92518 [details] cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=92518&action=review >> Source/WebCore/editing/FrameSelection.cpp:241 >> +void DragCaretController::nodeWillBeRemoved(Node* node) > > Where is the call to this function? It's called in Document::nodeChildrenWillBeRemoved and Document::nodeWillBeRemoved. >> Source/WebCore/editing/FrameSelection.cpp:246 >> + if (node && highestAncestor(node)->nodeType() == Node::DOCUMENT_FRAGMENT_NODE) > > Seems much better to check if the highest ancestor is the document rather than specifically checking that it's a document fragment. I’m also wondering how the result of this would differ from the much faster inDocument check, which is not O(height) of the DOM tree. On the other hand, the drag caret is almost always non-visible, so the performance of this code will probably rarely be important. You're right. We can just call inDocument here. >> Source/WebCore/editing/FrameSelection.cpp:249 >> + if (removingNodeRemovesPosition(node, m_position.deepEquivalent())) { > > We use early return for the other two conditions in this function. I suggest using it for this one too. Done. >> Source/WebCore/editing/FrameSelection.cpp:253 >> + view->clearSelection(); > > I'd like to see this sharing code with FrameSelection::respondToNodeModification instead of having a copied and pasted version of the sequence. Extracted clearRenderViewSelection. >> Source/WebCore/editing/FrameSelection.cpp:1120 >> + return !isTableElement(node) && !editingIgnoresContent(node); > > I think it would read better as a single return; seems you just moved the code, but still I would like that better. Done. >> Source/WebCore/editing/FrameSelection.h:54 >> +protected: > > Nothing private? That seems like a mistake. Can anything be private? I think we absolutely should be making the data members private instead of protected, even if that does mean more code changes. Maybe in a followup? Absolutely. I wanted to make this changeset small and do a cleanup in a followup but I can do that here if you'd prefer that. >> Source/WebCore/editing/FrameSelection.h:90 >> + bool isNone() const { return m_position.isNull(); } > > I think this is an awkward function name. In the case of a selection where it can be none, caret, or range, this is barely tolerable, but in a caret class where it’s either a caret or no caret, “none” is a curious way to talk about things. Removed this function and added hasCaret.
Created attachment 92747 [details] Fixed per comments
Comment on attachment 92747 [details] Fixed per comments View in context: https://bugs.webkit.org/attachment.cgi?id=92747&action=review > Source/WebCore/editing/FrameSelection.cpp:155 > +void DragCaretController::setCaretPosition(const VisiblePosition& position) > +{ > + if (Node* node = m_position.deepEquivalent().containerNode()) > + invalidateCaretRect(node); > + m_position = position; > + m_caretRectNeedsUpdate = true; > + Document* document = 0; > + if (Node* node = m_position.deepEquivalent().containerNode()) { > + invalidateCaretRect(node); > + document = node->document(); > + } > + updateCaretRect(document, m_position, m_position.isNotNull() ? VisibleSelection::CaretSelection : VisibleSelection::NoSelection, m_position.isOrphan()); > +} Should this function have an early exit for the case where the position is not changing? > Source/WebCore/editing/FrameSelection.cpp:261 > +void FrameSelection::nodeWillBeRemoved(Node *node) Should be Node* rather than Node *.
Thanks for the review! (In reply to comment #10) > Should this function have an early exit for the case where the position is not changing? That's a good point. Added. > > Source/WebCore/editing/FrameSelection.cpp:261 > > +void FrameSelection::nodeWillBeRemoved(Node *node) > > Should be Node* rather than Node *. Fixed.
Committed r86039: <http://trac.webkit.org/changeset/86039>
http://trac.webkit.org/changeset/86039 might have broken SnowLeopard Intel Release (WebKit2 Tests)
(In reply to comment #11) > Thanks for the review! > > (In reply to comment #10) > > Should this function have an early exit for the case where the position is not changing? > > That's a good point. Added. It turned out that this wasn't so great. Caret isn't drawn by drag caret controller anymore :( I'll remove it in the follow up patch for the bug 60454.
Build fix for !ENABLE(TEXT_CARET) committed as r86214. <http://trac.webkit.org/changeset/86214>
(In reply to comment #15) > Build fix for !ENABLE(TEXT_CARET) committed as r86214. > <http://trac.webkit.org/changeset/86214> Oops, sorry about breaking the build & thanks for the fix!