[webkit-reviews] review denied: [Bug 60273] Extract a DragCaretController from FrameSelection : [Attachment 92518] cleanup

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 60273: Extract a DragCaretController from FrameSelection
https://bugs.webkit.org/show_bug.cgi?id=60273

Attachment 92518: cleanup
https://bugs.webkit.org/attachment.cgi?id=92518&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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: