[webkit-reviews] review granted: [Bug 67344] [Qt] Transform QtFallbackWebPopupCombo into QtWebComboBox : [Attachment 105868] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has granted Caio Marcelo de Oliveira Filho
<cmarcelo at webkit.org>'s request for review:
Bug 67344: [Qt] Transform QtFallbackWebPopupCombo into QtWebComboBox
https://bugs.webkit.org/show_bug.cgi?id=67344

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

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105868&action=review


Beautiful refactor! r=me

> Source/WebKit/qt/ChangeLog:11
> +	   signal) and a convenience to show its popup in the cursor position.

convenience method*

>> Source/WebKit/qt/WebCoreSupport/QtWebComboBox.cpp:64
>> +#endif // QT_NO_IM
> 
> I would just remove this, it is broken anyway and might bite us. It is quite
dangeous resetting the input context behind our backs as it can easily get out
of sync, for instance if I have a composition, then this would reset that from
the internal state of the input context and it will get our of sync with
webcore

Agreed.

> Source/WebKit/qt/WebCoreSupport/QtWebComboBox.h:37
> +    void showPopupInCursorPosition();

Would sound slightly better as showPopupAtCursorPosition().