[webkit-reviews] review denied: [Bug 114819] Show a block caret for overtype mode : [Attachment 199026] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Sergio Villar Senin
<svillar at igalia.com>'s request for review:
Bug 114819: Show a block caret for overtype mode
https://bugs.webkit.org/show_bug.cgi?id=114819

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=199026&action=review


> Source/WebCore/editing/Editor.cpp:3132
> +	   m_overwriteModeSelection = adoptPtr(new FrameSelection(frame(),
true));

I'm not keen on the fact we're going to create another FrameSelection.
Can't we add a single Position to FrameSelection and change updateAppearance to
paint the selection depending on the mode?

> Source/WebCore/editing/FrameSelection.cpp:108
> +FrameSelection::FrameSelection(Frame* frame, bool isOverwriteModeSelection)

Please use enum instead.

> Source/WebCore/editing/FrameSelection.cpp:345
> +	   m_frame->editor()->updateOverwriteModeSelection();

This is going to fire a second set of selectionchange event, etc... r- because
of this.

> Source/WebCore/editing/FrameSelection.cpp:1775
> +    // In overtype mode, the internal FrameSelection will paint the
selection pretending to be a caret.
> +    if (m_frame->editor()->isOverwriteModeEnabled() !=
m_isOverwriteModeSelection)
> +	   return;
> +

Oh no :( circular dependencies.