Bug 60273 – Extract a DragCaretController from FrameSelection

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Dimitri Glazkov (Google)]

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.

[Ryosuke Niwa]

We should definitely do this.

[Levi Weintraub]

No one should have to see their FIXME's pop back up over 5 years from writing them... No one!

[Ryosuke Niwa]

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.

[WebKit Review Bot]

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.

[Ryosuke Niwa]

Created attachment 92518 [details]
cleanup

[Ryosuke Niwa]

Anyone interested in reviewing this?

[Darin Adler]

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:

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 92747 [details]
Fixed per comments

[Darin Adler]

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 *.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r86039: <http://trac.webkit.org/changeset/86039>

[WebKit Review Bot]

http://trac.webkit.org/changeset/86039 might have broken SnowLeopard Intel Release (WebKit2 Tests)

[Ryosuke Niwa]

(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.

[David Kilzer (:ddkilzer)]

Build fix for !ENABLE(TEXT_CARET) committed as r86214.
<http://trac.webkit.org/changeset/86214>

[Ryosuke Niwa]

(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!