[Webkit-unassigned] [Bug 139179] HTMLSelectElement add() should support index as second argument

[bugzilla-daemon at webkit.org]

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

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #243968|review?, commit-queue?      |review+, commit-queue-
              Flags|                            |

--- Comment #57 from Darin Adler <darin at apple.com> ---
Comment on attachment 243968
  --> https://bugs.webkit.org/attachment.cgi?id=243968
Patch-Updated-Review11

View in context: https://bugs.webkit.org/attachment.cgi?id=243968&action=review

We are almost there. There is still a bit of unnecessary overloading and unneeded syntax in the IDL files.

> Source/WebCore/html/HTMLOptionsCollection.idl:35
> +#if defined(LANGUAGE_JAVASCRIPT) && LANGUAGE_JAVASCRIPT
> +    [RaisesException] void add([Default=Undefined] HTMLElement element, [Default=Undefined] optional HTMLElement? before);
> +    [RaisesException] void add([Default=Undefined] HTMLElement element, [Default=Undefined] optional long index);
> +#endif
> +    [RaisesException] void add([Default=Undefined] HTMLOptionElement option, [Default=Undefined] optional unsigned long index);

Here’s what this really should say:

    #if !defined(LANGUAGE_OBJECTIVE_C) || !LANGUAGE_OBJECTIVE_C
    [RaisesException] void add([Default=Undefined] HTMLElement element, [Default=Undefined] optional HTMLElement? before);
    [RaisesException] void add([Default=Undefined] HTMLElement element, optional long index);
    #else
    [RaisesException] void add(HTMLOptionElement option, optional unsigned long index);
    #endif

I removed some of the [Default=Undefined] that are not needed, but I’m also not sure which if any of those remaining [Default=Undefined] are needed. Maybe we can remove them all? Tests should make that clear.

> Source/WebCore/html/HTMLSelectElement.idl:45
> +    [ObjCLegacyUnnamedParameters, RaisesException] void add([Default=Undefined] HTMLElement element,
> +    [Default=Undefined] optional HTMLElement? before);

This is indented wrong. It should all be on one line, or the second line should be indented by 4 spaces. I’m not certain we still need the [Default=Undefined] here.

> Source/WebCore/html/HTMLSelectElement.idl:47
> +    [RaisesException] void add([Default=Undefined] HTMLElement element, [Default=Undefined] optional long index);

We don’t need [Default=Undefined] for index and I’m also not sure we need it for element.

> LayoutTests/fast/dom/HTMLSelectElement/add.html:49
> +var option1 = createOption("X1", "Y1");
> +shouldBeEqualToString('testAdd3(option1,null)', '0,1,2,Y1');

This test would be even better without all these local variables:

    shouldBeEqualToString('testAdd3(createOption("X1", "Y1"))', '0,1,2,Y1');

Also not sure that having separate arguments for "text" and "value" for createOption is helpful to the test. Could just generate the text by adding something to the value, since the text has no effect on the outcome of the test. And I would put the resetSelection call into the test function so it doesn’t have to be repeated over and over again.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150105/252fe6db/attachment-0002.html>