[webkit-reviews] review denied: [Bug 76389] Form select option not de-selecting : [Attachment 124049] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Pablo Flouret
<pablof at motorola.com>'s request for review:
Bug 76389: Form select option not de-selecting
https://bugs.webkit.org/show_bug.cgi?id=76389

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

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


> Source/WebCore/ChangeLog:8
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Test: fast/forms/select-option-selecting.html

You had better mention rationale of the change. e.g. specification says so,
other browsers work so.

>> Source/WebCore/html/HTMLSelectElement.cpp:771
>> +	    selectOption(m_multiple || size() > 1 ? -1 :
nextSelectableListIndex(-1));
> 
> I think for clarity we tend to parenthesize the whole conditional even though
rules of operator priority makes this correct anyway.
> 
> However, given the more complex expression, I would instead opt to just break
this up into an "else if" and "else" clause, for readability and parallelism.

Should this be !usesMenuList()?

>> LayoutTests/ChangeLog:9
>> +	    * fast/forms/select-option-selecting.html: Added.
> 
> I've never quite understood where to place these tests-- I assume to keep the
folder manageable we should be putting new tests in fast/forms/select/ instead?
It looks like newer tests are being added there.

Jon is right.  We had better put tests for <select> into fast/forms/select/.   
Bug 65915

> LayoutTests/fast/forms/select-option-selecting.html:33
> +var normal = document.forms[0].elements.normal;

'normal' is not a helpful name.  It should be menuListWithoutSize or something.


> LayoutTests/fast/forms/select-option-selecting.html:37
> +normal.options[2].selected = true;
> +normal.options[2].selected = false;

Please add
 debug('Set and reset normal.options[2].selected');
to improve readability of test result.

> LayoutTests/fast/forms/select-option-selecting.html:42
> +var defaultSizedNormal = document.forms[0].elements.defaultSizedNormal;

'defaultSize' is confusing with having no size.  Should be 'menuListWIthSize1' 
or something?

> LayoutTests/fast/forms/select-option-selecting.html:51
> +var sizedNormal = document.forms[0].elements.sizedNormal;

'sizedNormal' is not a good name.  'singleListBox'?

> LayoutTests/fast/forms/select-option-selecting.html:60
> +sizedNormal.selectedIndex = 1;
> +shouldBe("sizedNormal.selectedIndex", "1");

The test result is not readable.  This should be:
  shouldBe("sizedNormal.selectedIndex = 1; sizedNormal.selectedIndex", "1");
or
 debug("Setting 1 to selectedIndex");
 sizedNormal.selectedIndex = 1;
 shouldBe(...)

> LayoutTests/fast/forms/select-option-selecting.html:64
> +sizedNormal.selectedIndex = -1;
> +shouldBe("sizedNormal.selectedIndex", "-1");

ditto.

> LayoutTests/fast/forms/select-option-selecting.html:69
> +var multiple = document.forms[0].elements.multiple;

How about changing the name to 'multipleListBox'?