Bug 60454 – Make member variables of CaretBase private

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

Right now, all member variables of CaretBase are protected and therefore visible to FrameSelection and DragCaretController.  We should make all member variables private instead.

[Ryosuke Niwa]

Created attachment 92999 [details]
cleanup

[Early Warning System Bot]

Comment on attachment 92999 [details]
cleanup

Attachment 92999 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8687033

[Ryosuke Niwa]

In void QWebPagePrivate::inputMethodEvent(QInputMethodEvent *ev):

we have:
        case QInputMethodEvent::Cursor: {
            frame->selection()->setCaretVisibility(a.length ? CaretIsVisible : CaretIsHidden);
            if (a.length > 0) {
                RenderObject* caretRenderer = frame->selection()->caretRenderer();
                if (caretRenderer) {
                    QColor qcolor = a.value.value<QColor>();
                    caretRenderer->style()->setColor(Color(makeRGBA(qcolor.red(), qcolor.green(), qcolor.blue(), qcolor.alpha())));
                }
            }
            break;
        }

which was added by http://trac.webkit.org/changeset/48967.  As far as I know, this is the only WebKit code that calls setCaretVisibility and I suspect something is wrong here.

[Darin Adler]

Comment on attachment 92999 [details]
cleanup

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.

[Chang Shu]

Why do we have to make setCaretVisibility() private?

[Ryosuke Niwa]

(In reply to comment #5)
> Why do we have to make setCaretVisibility() private?

Because it's almost always wrong to call it directly.

[Chang Shu]

(In reply to comment #6)
> (In reply to comment #5)
> > Why do we have to make setCaretVisibility() private?
> 
> Because it's almost always wrong to call it directly.

I see. Is there a way to replace the qt code using public API?

[Ryosuke Niwa]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Why do we have to make setCaretVisibility() private?
> > 
> > Because it's almost always wrong to call it directly.
> 
> I see. Is there a way to replace the qt code using public API?

As far as I know, no.  However, looking at http://doc.qt.nokia.com/4.3/qinputmethodevent.html, it seems like the current Qt code is correct so I guess I'll make it public again :(

[Ryosuke Niwa]

Comment on attachment 92999 [details]
cleanup

View in context: https://bugs.webkit.org/attachment.cgi?id=92999&action=review

>> Source/WebCore/editing/FrameSelection.cpp:74
>> +CaretBase::CaretBase(CaretVisibility caretVisibility)
> 
> I’d just name the argument “visibility”.

Done.

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

Done.

>> Source/WebCore/editing/FrameSelection.cpp:1086
>> +    // is either the renderer we just found, or one of its containers)
> 
> And one here too.

Done.

>> 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.

Good idea; done.

>> 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.

Renamed to localCaretRectWithoutUpdate.

> Source/WebCore/editing/FrameSelection.h:-190
> -    void setCaretVisible(bool = true);

I added back a public inline function that calls setCaretVisibility to make qt happy.

[Ryosuke Niwa]

Created attachment 93020 [details]
patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 93020 [details]:

java/lc3/JavaObject/JavaObjectToByte-006.html bug 60333 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 93020 [details]
patch for landing

Clearing flags on attachment: 93020

Committed r86193: <http://trac.webkit.org/changeset/86193>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.