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

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

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

Nice work. I think we are almost there. review- because the Mac build is still failing

At this point it’s a little unclear to me the total set of changes in behavior we are introducing. For changes that are incompatible with older versions of WebKit, I am concerned that there may be web content or perhaps apps on iOS or Mac that depend on the old behavior, even if it’s not consistent with other browsers. So I want to be really clear about what we are changing.

The change that is explicit here is handling numeric indices as the second argument to both add functions.

But I think another thing this changes is that the first argument to HTMLSelectElement's add is now type checked and we throw an exception if it's not the right type. But we have left it so you can omit this first argument and it will silently do nothing. Is that really what other web browsers do and what the standard calls for? Or are we just retaining a WebKit quirk by allowing the argument to be omitted?

What about extra arguments? Where’s the test that covers extra arguments to see if we raise an exception for the extra ones or just ignore them silently? This is another thing that might have changed when we changed how the bindings worked.

I can’t tell if we got all the expectations right or not because it’s hard for me to read through the test to see what is expected, and this worries me slightly about a change that affects web-visible API. What I want to know is exactly what we are changing, how this relates to what WebKit used to do, what other browsers do, and what the standard says.

> Source/WebCore/ChangeLog:10
> +        Also This matches the behavior of Chrome and FireFox.

Stray capital letter "This".

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

ObjCLegacyUnnamedParameters is incorrect here and should be omitted. That’s why the Mac build is failing.

Indentation of the second line is wrong. Correct style for WebKit would be to put this all on one line, or to indent the second line by 4 spaces. Indenting by 24 spaces is strange.

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

I would have written:

    add(element, item(beforeIndex), ec);

I like my version better because there’s no need for an additional type cast.

> Source/WebCore/html/HTMLSelectElement.idl:50
> +    [RaisesException] void add(HTMLElement element, long before);

I think it’s a little strange to put this after the "remove" function just to share the #if. It puts "remove" between the two overloads of "add", and it puts "add" between the two alternative versions of "remove". Seems like we should move this at least above the "// In JavaScript ..." comment for clarity.

> LayoutTests/ChangeLog:10
> +        * fast/dom/HTMLSelectElement/select-add-optgroup-options.html: Added.

The name of this test is not so great. I would call it "options-collection-add.html" since it’s in the HTMLSelectElement directory. Or put it in a new HTMLOptionsCollection directory and name it "add.html".

> LayoutTests/ChangeLog:11
> +        * fast/dom/HTMLSelectElement/select-add-optgroup.html:

The name of this test is no longer all that good. The test covers many different aspects of the add function, not just the handling of optgroup. Also, since it’s in the HTMLSelectElement directory I don’t think the test needs "select" in its name. I would just name it "add.html".

> LayoutTests/fast/dom/incompatible-operations-expected.txt:15
> +PASS aSelect.add(aDOMImplementation, aDOMImplementation) threw exception TypeError: Type error.

To me it seems strange that this test that seems to be intended to to cover DOM bindings in general is actually only testing HTMLSelectElement’s add function. I think we should be adding coverage of some other DOM function if the intent of this test is not specific to the add function. Worth researching when this test was added.

> LayoutTests/fast/dom/HTMLSelectElement/select-add-optgroup-options.html:74
> +mySelect.options.add(new Option("X", "Y", false, false), null);
> +shouldBeEqualToString('deepCopy()', '0,1,2,Y');

When creating a test like this one it's useful to have a substantive part of the test in the "should be" expression so we can see what is being tested. Typically we make helper functions so that we can write a small exception and it can be clear what we are testing. For example, parts of the test would look like this:

    shouldBeEqualToString('testAdd(0)', 'Y,0,1,2');
    shouldBeEqualToString('testAdd(1)', '0,Y,1,2');

    shouldBeEqualToString('testAdd("0")', 'Y,0,1,2');
    shouldBeEqualToString('testAdd("1")', 'Y,0,1,2');

    shouldBeEqualToString('testAdd(null)', '0,1,2,Y');

And so on. Note how this makes both the test code itself, and the test output, easier to read. Just a tip for future tests. Not insisting that you improve this one, although you are welcome to.

If you had written the test like this it would have been easier for me to read it and see what the expectations are. I’m having a bit of a hard time seeing what's being tested. It would be especially useful to use a helper function to create the option elements because the repetitive "new Option('X', 'X', false, false)" is distracting.

-- 
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/20141230/9d4f8e0c/attachment-0002.html>