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

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

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

Looks pretty good, but enough mistakes that I’ll say review-.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:37
> +    HTMLSelectElement& imp = impl();

I suggest naming this local variable "select" instead of "imp", as is done in the other functions in this file.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:38
> +    HTMLElement* after(JSHTMLElement::toWrapped(exec->argument(0)));

It’s really strange to call this local variable “after”. This argument is the new element we are attempting to add as a child of the select element. I suggest we call it “element” or “new element”. I don’t think the word “after” makes sense for this at all.

We normally prefer "=" syntax to construction syntax for a line like this, at least when it’s hand-written, not script-generated.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:40
> +    if (exec->hadException())
> +        return jsUndefined();

This is unneeded dead code. There is nothing above this that can cause an exception.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:43
> +    if (exec->hadException())
> +        return jsUndefined();

This is unneeded dead code. There is nothing above this that can cause an exception.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:44
> +    ExceptionCode ec = 0;

It’s strange to define this up here when it’s not used until later. I would move the definition down to right where it’s actually used.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:45
> +    int index = exec->argument(1).toInt32(exec);

It would be better to not even do this conversion to an integer if argument 1 turned out to be an HTMLElement. It would probably be easy to structure the code this way by putting this code in the else branch below.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:46
> +    JSValue arg1(exec->argument(1));

Putting argument(1) into a local variable after the function has already used it twice, and then using that local variable only one time, is not good style. We should either omit the local variable entirely, or set it up before the argument is used the first time and use it all three times.

It’s also quite strange to put line of code in between the toInt32 call that might have cause an exception and the line that checks for that exception. Those two lines should be right next to each other.

> Source/WebCore/bindings/js/JSHTMLSelectElementCustom.cpp:59
> +    if (exec->argumentCount() == 1) {
> +        if (after)
> +            imp.add(after, nullptr, ec);
> +    } else {
> +        if (before || arg1.isUndefinedOrNull())
> +            imp.add(after, before, ec);
> +        else
> +            imp.add(after, index, ec);
> +    }

There is no need to check argumentCount here. The code in the "else" will do exactly the same thing without a special case for one argument, since argument 1 will be undefined and before will be nullptr. So please delete the unneeded special case for 1 argument. Also, it’s is not good to have an explicit null check for "after" here in the first branch, but not below. That null check is not needed at all, since HTMLSelectElement::add already checks it, but if we did want a null check I think we’d want it in all cases, not just one.

Do we need a check for extra arguments? I believe some DOM functions raise exceptions when you have unwanted extra arguments. Maybe this is an older function where we tolerate them silently, though. Where is the test case that covers that?

If the first argument is not an HTMLSelectElement, this entire function silently does nothing. Is that the correct behavior or are we supposed to raise an exception in that case? Where is the test case that covers that?

-- 
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/20141224/58ba2203/attachment-0002.html>