[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 #14 from Dinu Jacob <dinu.jacob at Nokia.com>  2012-05-29 06:25:18 PST ---
(In reply to comment #13)
> (From update of attachment 144141 [details])
> 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
>
Please let me know if there is any other way of getting this information. 
> > 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.
> 
Sure. I will use that.
> > 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 ?
> 
Will fix it.
> 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?
> 
I did this is to avoid making changes in other ports. I can add new methods for QT that splits this into two.

> > 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?
> 
For multi-select, more than one index can be in selected state. The smallest indexed item is what is displayed. The selected indices are sorted to determine the smallest when finally closing the popup menu.
> > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.h:71
> > +    bool m_allowMultipleSelections;
> 
> Maybe this should be an enum? SelectionStyle { SingleSelection, MultiSelection }, just wondering
Yes, it might be better to use an enum

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