[webkit-reviews] review denied: [Bug 70496] Cannot select multiple options by mouse dragging in <select multiple="multiple" size="7"> list : [Attachment 114266] Proposed 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 114266: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=114266&action=review

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


> Source/WebCore/html/HTMLSelectElement.cpp:1208
> +    } else if (event->type() == eventNames().mousemoveEvent &&
event->isMouseEvent()
> +		     && static_cast<MouseEvent*>(event)->button() == LeftButton
&& static_cast<MouseEvent*>(event)->buttonDown()
> +		     && !static_cast<MouseEvent*>(event)->ctrlKey() &&
!toRenderBox(renderer())->canBeScrolledAndHasScrollableArea()) {

* The indentation looks wrong for folded lines. You don't need to fold a long
lines.

* You don't need to check these conditions in one 'if' statement. You can
write:

} else if (event->type() == evnetNames().mousemoveEvent &&
event->isMouseEvent()) {
    MouseEvent* mouseEvent = static_cast<MouseEvent*>(event);
    If (mouseEvent->button() != LeftButton || !mouseEvent->buttonDown())
	return;
    ....

* Checking only ctrlKey() is wrong. It should be metaKey() in Mac.  Please
refeer to mousedown handling in this function.

* Do we need to check !canBeScrolledAndHasScrollableArea()?

> Source/WebCore/html/HTMLSelectElement.cpp:1209
> +	   // Convert to coords relative to the list box if needed.

This comment is not helpful.  Please remove it.