[webkit-reviews] review denied: [Bug 70496] Cannot select multiple options by mouse dragging in <select multiple="multiple" size="7"> list : [Attachment 114672] Updated patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Rakesh <rakesh.kn at motorola.com>'s
request for review:
Bug 70496: Cannot select multiple options by mouse dragging in <select
multiple="multiple" size="7"> list
https://bugs.webkit.org/show_bug.cgi?id=70496

Attachment 114672: Updated patch
https://bugs.webkit.org/attachment.cgi?id=114672&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=114672&action=review


>
LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag-with-options-le
ss-than-size.html:30
> +<script>
> +    if (window.eventSender) {

You don't need to indent the whole content of <script>.

>
LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag-with-options-le
ss-than-size.html:50
> +	   eventSender.mouseMoveTo(x,y);

should have a space after ","

>
LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag-with-options-le
ss-than-size.html:59
> +	   eventSender.mouseDown();
> +	   eventSender.mouseDown(0, "addSelectionKey");
> +	   eventSender.mouseMoveTo(x, y + (optionHeight * 3));
> +	   eventSender.mouseUp(0, "addSelectionKey");
> +	   testOptionSelection(0, 4, "true", "selectId");

This test is meaningless. We should test a case where
 - an option is selected.  e.g. options[0] is selected
 - start dragging with addSelectionKey at another option. e.g. starting at
options[2]
 - end dragging with addSelectionKey at yet another option.  e.g. ending at
options[4]
 - then check if the selection is correctly added; options[0], [2] [3] [4]
should be selected.

>
LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag-with-options-le
ss-than-size.html:64
> +	   eventSender.mouseMoveTo(x,y);

should have a space after ","

>
LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag-with-options-le
ss-than-size.html:87
> +	   eventSender.mouseMoveTo(x,y);

ditto.

>
LayoutTests/fast/forms/select-multiple-elements-with-mouse-drag-with-options-le
ss-than-size.html:103
> +	   eventSender.mouseMoveTo(x,y);

ditto.