Summary: | Make member variables of CaretBase private | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Enhancement | CC: | commit-queue, cshu, darin, hausmann, joseph.ligman, tonikitoo, yael | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 60273, 60505 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Ryosuke Niwa
2011-05-08 20:35:24 PDT
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. |