[Webkit-unassigned] [Bug 85527] [Qt][WK2] Add support for multi-select list

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=85527





--- Comment #13 from Kenneth Rohde Christiansen <kenneth at webkit.org>  2012-05-26 01:17:23 PST ---
(From update of attachment 144141)
View in context: https://bugs.webkit.org/attachment.cgi?id=144141&action=review

Just skimming thought... Would be great if Caio could have a look

> Source/WebKit2/Shared/PlatformPopupMenuData.cpp:67
> +#elif PLATFORM(QT)

I thnk there were another if def used in the theme itself when delegation of multiply selection was used

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior/tst_multiSelect.qml:8
> +// FIXME: Moved to Desktop tests because we want to have mouseClick() to open the <select> tag. We can move it back
> +// when TestCase starts supporting touch events, see https://bugreports.qt.nokia.com/browse/QTBUG-23083.

Can you use experimental.test? I have some local patch adding better touch support there.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/multiselect.html:12
> +    var seloptions =document.getElementById('sel').options;
> +    var opt;
> +    var str = new String("");
> +    var value = new String("");
> +    for (var i=0, len=seloptions.length; i<len; i++)
> +    {

=document ?
len=sel ?

We should use consistent coding style

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2733
> +void WebPageProxy::valueChangedForPopupMenu(WebPopupMenuProxy*, int32_t newSelectedIndex, bool closePopupMenu)

shouldClosePopuoMenu? closeWhenDone?

It really seems hacked in here. Why not a separate method/message?

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:196
> +        // Update the selected indices list

punctuation at end.

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:206
> +    } else {
> +
> +        int oldIndex = -1;

Why the added newline?

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:244
> +    qSort(list);

Why is the sortign needed?

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.h:71
> +    bool m_allowMultipleSelections;

Maybe this should be an enum? SelectionStyle { SingleSelection, MultiSelection }, just wondering

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.