Fix the HTMLSelectElement.prototype.remove() method

Fix the HTMLSelectElement.prototype.remove() method

Fix the HTMLSelectElement.prototype.remove() method so that it behaves like
Element.remove() if no argument is passed (from ChildNode). This behavior
is consistent with Firefox.

Also get rid of custom code for HTMLSelectElement.remove() and
HTMLOptionsCollection.remove() by leveraging operation overloading in Web
IDL.

BUG=277660
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156917

[haraken, 2013-08-29 07:52:50]

Given that this is a non-standard feature and that the behavior aligns with
Firefox, LGTM. Needs an approval from an API owner.

https://codereview.chromium.org/23549004/diff/1/Source/core/html/HTMLSelectEl...
File Source/core/html/HTMLSelectElement.idl (right):

https://codereview.chromium.org/23549004/diff/1/Source/core/html/HTMLSelectEl...
Source/core/html/HTMLSelectElement.idl:43: [RaisesException] void remove();

Don't you need to add [RaisesException] to other overloaded remove()'s ?

[do-not-use, 2013-08-29 07:55:27]

https://codereview.chromium.org/23549004/diff/1/Source/core/html/HTMLSelectEl...
File Source/core/html/HTMLSelectElement.idl (right):

https://codereview.chromium.org/23549004/diff/1/Source/core/html/HTMLSelectEl...
Source/core/html/HTMLSelectElement.idl:43: [RaisesException] void remove();
On 2013/08/29 07:52:50, haraken wrote:
> 
> Don't you need to add [RaisesException] to other overloaded remove()'s ?

Why? the others do not throw. Only the one from ChildNode (aka Node::remove())
throws.

[tkent, 2013-08-29 08:00:00]

lgtm

[haraken, 2013-08-29 08:12:56]

https://codereview.chromium.org/23549004/diff/1/Source/core/html/HTMLSelectEl...
File Source/core/html/HTMLSelectElement.idl (right):

https://codereview.chromium.org/23549004/diff/1/Source/core/html/HTMLSelectEl...
Source/core/html/HTMLSelectElement.idl:43: [RaisesException] void remove();
On 2013/08/29 07:55:27, Christophe Dumez wrote:
> On 2013/08/29 07:52:50, haraken wrote:
> > 
> > Don't you need to add [RaisesException] to other overloaded remove()'s ?
> 
> Why? the others do not throw. Only the one from ChildNode (aka Node::remove())
> throws.

Sorry, I misread the implementation. You're right.

[commit-bot: I haz the power, 2013-08-29 08:33:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/23549004/1

[commit-bot: I haz the power, 2013-08-29 11:10:05]

Change committed as 156917

[arv (Not doing code reviews), 2013-08-29 13:33:22]

LGTM

https://codereview.chromium.org/23549004/diff/1/Source/core/html/HTMLOptionsC...
File Source/core/html/HTMLOptionsCollection.idl (right):

https://codereview.chromium.org/23549004/diff/1/Source/core/html/HTMLOptionsC...
Source/core/html/HTMLOptionsCollection.idl:36: void remove([Default=Undefined]
optional unsigned long index);
We should make this non optional as required by the spec. I cannot believe
anyone would use that without an argument.

var select = ...
select.options.remove(); // so unclear what to remove here?

[do-not-use, 2013-08-29 14:27:23]

https://codereview.chromium.org/23549004/diff/1/Source/core/html/HTMLOptionsC...
File Source/core/html/HTMLOptionsCollection.idl (right):

https://codereview.chromium.org/23549004/diff/1/Source/core/html/HTMLOptionsC...
Source/core/html/HTMLOptionsCollection.idl:36: void remove([Default=Undefined]
optional unsigned long index);
HTMLOptionsCollection is not an Element, therefore, I think it is less confusing
(it does not have a remove() function).

I am however in favor of making the argument to
HTMLOptionsCollection.remove(index) mandatory to match the specification and
avoid confusion. tkent@, what do you think?

[arv (Not doing code reviews), 2013-08-29 14:36:41]

I find it confusing because it is unclear which option to remove from the
options collection. Last? First? Selected? All?


On Thu, Aug 29, 2013 at 10:27 AM, <ch.dumez@sisa.samsung.com> wrote:

>
> https://codereview.chromium.**org/23549004/diff/1/Source/**core/html/**
>
HTMLOptionsCollection.idl<https://codereview.chromium.org/23549004/diff/1/Source/core/html/HTMLOptionsCollection.idl>
> File Source/core/html/**HTMLOptionsCollection.idl (right):
>
> https://codereview.chromium.**org/23549004/diff/1/Source/**core/html/**
>
HTMLOptionsCollection.idl#**newcode36<https://codereview.chromium.org/23549004/diff/1/Source/core/html/HTMLOptionsCollection.idl#newcode36>
> Source/core/html/**HTMLOptionsCollection.idl:36: void
> remove([Default=Undefined] optional unsigned long index);
> HTMLOptionsCollection is not an Element, therefore, I think it is less
> confusing (it does not have a remove() function).
>
> I am however in favor of making the argument to
> HTMLOptionsCollection.remove(**index) mandatory to match the specification
> and avoid confusion. tkent@, what do you think?
>
>
https://codereview.chromium.**org/23549004/<https://codereview.chromium.org/2...
>



-- 
erik

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[do-not-use, 2013-08-29 14:42:16]

On 2013/08/29 14:36:41, arv wrote:
> I find it confusing because it is unclear which option to remove from the
> options collection. Last? First? Selected? All?

Sadly we have a lot of those in Blink:
git grep index Source/ | grep idl | grep optional

In practice, we remove the first one because of [Default=Undefined] and the
undefined gets converted into a 0.
I don't like it either but Blink/WebKit has behaved this way "forever". I'd love
to fix this though if an API owner approves.

[tkent, 2013-08-30 00:55:29]

On 2013/08/29 14:42:16, Christophe Dumez wrote:
> In practice, we remove the first one because of [Default=Undefined] and the
> undefined gets converted into a 0.
> I don't like it either but Blink/WebKit has behaved this way "forever". I'd
love
> to fix this though if an API owner approves.

If the specification says the argument is mandatory and other browsers work so,
we should follow, of course.