Bug 60454

Created attachment 92999 [details] cleanup

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

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.

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.

Why do we have to make setCaretVisibility() private?

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

(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?

(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 :(

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.

Created attachment 93020 [details] patch for landing

The commit-queue encountered the following flaky tests while processing attachment 93020 [details]: java/lc3/JavaObject/JavaObjectToByte-006.html bug 60333 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 93020 [details] patch for landing Clearing flags on attachment: 93020 Committed r86193: <http://trac.webkit.org/changeset/86193>

All reviewed patches have been landed. Closing bug.