[webkit-reviews] review granted: [Bug 60454] Make member variables of CaretBase private : [Attachment 92999] cleanup

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 60454: Make member variables of CaretBase private
https://bugs.webkit.org/show_bug.cgi?id=60454

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=92999&action=review

r=me but you’ll need to resolve the Qt issue and you should consider a better
name for “fastLocalCaretRect”.

> Source/WebCore/editing/FrameSelection.cpp:74
> -CaretBase::CaretBase()
> +CaretBase::CaretBase(CaretVisibility caretVisibility)

I’d just name the argument “visibility”.

> Source/WebCore/editing/FrameSelection.cpp:1081
> +    // First compute a rect local to the renderer at the selection start

You should add a period.

> Source/WebCore/editing/FrameSelection.cpp:1086
> +    // Get the renderer that will be responsible for painting the caret
(which
> +    // is either the renderer we just found, or one of its containers)

And one here too.

> Source/WebCore/editing/FrameSelection.h:56
> +    CaretBase(CaretVisibility = Hidden);

Should mark this explicit, since we wouldn’t want this to be used as a
typecast.

> Source/WebCore/editing/FrameSelection.h:67
> +    const IntRect& fastLocalCaretRect() const { return m_caretLocalRect; }

I don’t think the meaning of “fast” here is clear. Everyone wants things to be
as fast as possible, so it’s critical to define who should and should not call
this. I think the callers are sites that want the rect without it being
updated, perhaps? I think we need a better name.