[webkit-reviews] review denied: [Bug 114819] Show a block caret for overtype mode : [Attachment 199666] 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 199666: Patch
https://bugs.webkit.org/attachment.cgi?id=199666&action=review

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


New approach is much saner than the original patch but it still introduces
undesirable circular dependencies and makes updateAppearance update the caret
visibility, which we don't want.

> Source/WebCore/ChangeLog:10
> +	   The 1-pixel long caret will be replaced in overtype mode by a

Caret & selection is the same thing. Caret = collapsed selection.
We need to revise this change log entry.

> Source/WebCore/editing/Editor.cpp:3130
> +    frame()->selection()->setCaretVisible(!m_overwriteModeEnabled);

This seems wrong. We might want to update the appearance but we shouldn't be
updating the caret visibility like this.
FrameSelection talking back to editor to get isOverwriteModeEnabled is also
bad.
We probably want to add another member variable to FrameSelection saying that
we want to show a block caret whenever possible.

> Source/WebCore/editing/FrameSelection.cpp:1765
> +    bool shouldFakeCaretVisibility =
m_frame->editor()->isOverwriteModeEnabled() && m_selection.isCaret();
> +    if (shouldFakeCaretVisibility) {

I don't like this variable name. "shouldFakeCaretVisibility" doesn't tell us
why it's fake or in what sense it is fake.
It's also strange that this is the special case instead of a block caret inside
a line being a special case.

> Source/WebCore/editing/FrameSelection.cpp:1767
> +	   shouldFakeCaretVisibility = forwardPosition.isNotNull();

And this flag is even updated here!

> Source/WebCore/editing/FrameSelection.cpp:1768
> +	   setCaretVisibility(shouldFakeCaretVisibility ? Hidden : Visible);

updateAppearance shouldn't be updating caret visibility. You do realize that
setCaretVisibility calls updateAppearance, right?
r- because of this.