[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 #243186|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

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

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

The tricky part here is the binding, not the C++ function.

We need more test coverage. I don’t see anything here about passing, say, a string that can be converted to a number, or a boolean which can be converted to a number. We also want to cover things like the smallest integer, the largest integer, the numbers just outside the integer range, infinity, etc.

Currently the unusual bindings case is breaking for all the languages other than JavaScript, such as Objective-C, so that needs to be addressed before we can land this.

> Source/WebCore/html/HTMLSelectElement.cpp:237
> +    HTMLElement* beforeElement = downcast<HTMLElement>(options()->item(beforeIndex));
> +
> +    add(element, beforeElement, ec);

I’d leave out the blank line and maybe the local variable entirely.

> Source/WebCore/html/HTMLSelectElement.h:61
>      void add(HTMLElement*, HTMLElement* beforeElement, ExceptionCode&);
>  
> +    void add(HTMLElement*, int beforeIndex, ExceptionCode&);

I’d leave out the blank line and have these be in one paragraph.

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

It’s strange to have a blank line between the “add” lines and not between this and the previous and next functions. Lets leave out the blank line.

In this new overload, the arguments don’t need to be optional, [Default=Undefined] is also unneeded/wrong and ObjCLegacyUnnamedParameters is incorrect.

-- 
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/20141214/c6f5d979/attachment-0002.html>