Bug 60273 - Extract a DragCaretController from FrameSelection
Summary: Extract a DragCaretController from FrameSelection
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 60454 60505
  Show dependency treegraph
 
Reported: 2011-05-05 09:00 PDT by Dimitri Glazkov (Google)
Modified: 2011-05-10 22:47 PDT (History)
10 users (show)

See Also:


Attachments
work in progress (26.66 KB, patch)
2011-05-05 17:04 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
cleanup (29.84 KB, patch)
2011-05-05 18:09 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixed per comments (31.66 KB, patch)
2011-05-08 13:55 PDT, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-05-05 09:00:07 PDT
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.
Comment 1 Ryosuke Niwa 2011-05-05 09:39:15 PDT
We should definitely do this.
Comment 2 Levi Weintraub 2011-05-05 11:48:01 PDT
No one should have to see their FIXME's pop back up over 5 years from writing them... No one!
Comment 3 Ryosuke Niwa 2011-05-05 17:04:23 PDT
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.
Comment 4 WebKit Review Bot 2011-05-05 17:06:43 PDT
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.
Comment 5 Ryosuke Niwa 2011-05-05 18:09:29 PDT
Created attachment 92518 [details]
cleanup
Comment 6 Ryosuke Niwa 2011-05-06 10:18:24 PDT
Anyone interested in reviewing this?
Comment 7 Darin Adler 2011-05-08 08:26:49 PDT
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 8 Ryosuke Niwa 2011-05-08 13:54:05 PDT
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.
Comment 9 Ryosuke Niwa 2011-05-08 13:55:52 PDT
Created attachment 92747 [details]
Fixed per comments
Comment 10 Darin Adler 2011-05-08 15:54:47 PDT
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 *.
Comment 11 Ryosuke Niwa 2011-05-08 17:22:48 PDT
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.
Comment 12 Ryosuke Niwa 2011-05-08 17:40:18 PDT
Committed r86039: <http://trac.webkit.org/changeset/86039>
Comment 13 WebKit Review Bot 2011-05-08 19:55:58 PDT
http://trac.webkit.org/changeset/86039 might have broken SnowLeopard Intel Release (WebKit2 Tests)
Comment 14 Ryosuke Niwa 2011-05-09 11:45:43 PDT
(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.
Comment 15 David Kilzer (:ddkilzer) 2011-05-10 22:37:06 PDT
Build fix for !ENABLE(TEXT_CARET) committed as r86214.
<http://trac.webkit.org/changeset/86214>
Comment 16 Ryosuke Niwa 2011-05-10 22:47:17 PDT
(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!