Use union types for HTMLSelectElement.add()'s arguments

Use union types for HTMLSelectElement.add()'s arguments

According to the specification, the first argument should be
  (HTMLOptionElement or HTMLOptGroupElement)
and the second argument should be
  (HTMLElement or long)?
(and also be optional and default to null).

This changes behavior for the first argument (previously HTMLElement)
where a call was a silent no-op for invalid element types, and where
HTMLHRElement was allowed against the specification and with no
observable effects.

The new behavior is to throw TypeError for all invalid arguments, which is
compatible with Firefox and MSIE.

Also apply the same change to HTMLOptionsCollection.add(), which is little
more than a short-hand for calling add() on the owning select element.
Also drop the custom bindings implementation for that method, which was
essentially implementing the unionized second argument.

The behavior of HTMLOptionsCollection.add() is changed to ignore all
invalid (out of range) integer second arguments, rather than to throw for
negative ones below than -1. This change is also per specification and
compatible with other implementations.

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

[Jens Widell, 2014-11-11 11:50:21]

jl@opera.com changed reviewers:
+ bashi@chromium.org, haraken@chromium.org

[Jens Widell, 2014-11-11 11:50:22]

PTAL

Note that this is not quite ready to land: having a union that contains
HTMLElement causes core/dom/GlobalEventHandlers.h to be included into the
generated UnionTypesCore.cpp, which causes compilation errors. That header
defines static functions that are used by e.g. V8HTMLElement.cpp to implement
the event handlers, but are unused in UnionTypesCore.cpp.

I haven't really thought about how to best fix that. Any ideas, other than some
kind of custom exception in v8_union.py? (Which ought to be simple enough.)

[Jens Widell, 2014-11-11 12:08:44]

https://codereview.chromium.org/716773002/diff/1/Source/core/html/HTMLSelectE...
File Source/core/html/HTMLSelectElement.cpp (right):

https://codereview.chromium.org/716773002/diff/1/Source/core/html/HTMLSelectE...
Source/core/html/HTMLSelectElement.cpp:502: add(element, HTMLElementOrLong(),
exceptionState);
Could simplify a bit by using appendChild() instead here. All add() brings in
addition is a setNeedsValidityCheck() call, and we have one in this function too
already.

[bashi, 2014-11-11 23:51:27]

Thank you for working on this!

On 2014/11/11 11:50:22, Jens Widell wrote:
> PTAL
> 
> Note that this is not quite ready to land: having a union that contains
> HTMLElement causes core/dom/GlobalEventHandlers.h to be included into the
> generated UnionTypesCore.cpp, which causes compilation errors. That header
> defines static functions that are used by e.g. V8HTMLElement.cpp to implement
> the event handlers, but are unused in UnionTypesCore.cpp.
> 
> I haven't really thought about how to best fix that. Any ideas, other than
some
> kind of custom exception in v8_union.py? (Which ought to be simple enough.)

Hmm, I don't have a good solution, but just removing GlobalEventHandlers.h in
v8_union.py would be a simple workaround as you said.

[Jens Widell, 2014-11-12 07:51:49]

On 2014/11/11 23:51:27, bashi1 wrote:
> Thank you for working on this!
> 
> On 2014/11/11 11:50:22, Jens Widell wrote:
> > PTAL
> > 
> > Note that this is not quite ready to land: having a union that contains
> > HTMLElement causes core/dom/GlobalEventHandlers.h to be included into the
> > generated UnionTypesCore.cpp, which causes compilation errors. That header
> > defines static functions that are used by e.g. V8HTMLElement.cpp to
implement
> > the event handlers, but are unused in UnionTypesCore.cpp.
> > 
> > I haven't really thought about how to best fix that. Any ideas, other than
> some
> > kind of custom exception in v8_union.py? (Which ought to be simple enough.)
> 
> Hmm, I don't have a good solution, but just removing GlobalEventHandlers.h in
> v8_union.py would be a simple workaround as you said.

PS#3 adds a simple workaround in v8_union.py.

[bashi, 2014-11-12 08:25:57]

On 2014/11/12 07:51:49, Jens Widell wrote:
> On 2014/11/11 23:51:27, bashi1 wrote:
> > Thank you for working on this!
> > 
> > On 2014/11/11 11:50:22, Jens Widell wrote:
> > > PTAL
> > > 
> > > Note that this is not quite ready to land: having a union that contains
> > > HTMLElement causes core/dom/GlobalEventHandlers.h to be included into the
> > > generated UnionTypesCore.cpp, which causes compilation errors. That header
> > > defines static functions that are used by e.g. V8HTMLElement.cpp to
> implement
> > > the event handlers, but are unused in UnionTypesCore.cpp.
> > > 
> > > I haven't really thought about how to best fix that. Any ideas, other than
> > some
> > > kind of custom exception in v8_union.py? (Which ought to be simple
enough.)
> > 
> > Hmm, I don't have a good solution, but just removing GlobalEventHandlers.h
in
> > v8_union.py would be a simple workaround as you said.
> 
> PS#3 adds a simple workaround in v8_union.py.

union type usage and the v8_union.py change LGTM.

[haraken, 2014-11-12 08:34:34]

LGTM

https://codereview.chromium.org/716773002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_union.py (right):

https://codereview.chromium.org/716773002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:18: # compilation errors.

Add a FIXME?

[Jens Widell, 2014-11-12 08:40:00]

The CQ bit was checked by jl@opera.com

[Jens Widell, 2014-11-12 08:40:14]

https://codereview.chromium.org/716773002/diff/40001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_union.py (right):

https://codereview.chromium.org/716773002/diff/40001/Source/bindings/scripts/...
Source/bindings/scripts/v8_union.py:18: # compilation errors.
On 2014/11/12 08:34:34, haraken wrote:
> 
> Add a FIXME?

Done.

[commit-bot: I haz the power, 2014-11-12 08:40:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/716773002/60001

[commit-bot: I haz the power, 2014-11-12 10:43:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-12 10:43:38]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/36031)

[Jens Widell, 2014-11-12 10:48:37]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-11-12 10:49:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/716773002/60001

[commit-bot: I haz the power, 2014-11-12 11:31:35]

Committed patchset #4 (id:60001) as 185199

[tkent, 2014-11-13 00:33:46]

> HTMLHRElement was allowed against the specification and with no
> observable effects.

It's used in the Encoding menu on chrome://settings/fonts.

[Jens Widell, 2014-11-13 08:25:32]

On 2014/11/13 00:33:46, tkent wrote:
> > HTMLHRElement was allowed against the specification and with no
> > observable effects.
> 
> It's used in the Encoding menu on chrome://settings/fonts.

Great to know, thanks!

AFAICT, this still works, and if I found the right code, the <hr> is added using
Node.appendChild(), so not affected by this CL.

I don't know how I tested when I came to the conclusion that adding a <hr> had
no observable effects, however. It quite clearly adds a thin line between
options. (Not the greatest visual effect IMO, but it's certainly there.)

Do we think that we should restore this extended functionality of
HTMLSelectElement.add()?

[tkent, 2014-11-14 00:20:24]

On 2014/11/13 08:25:32, Jens Widell wrote:
> AFAICT, this still works, and if I found the right code, the <hr> is added
using
> Node.appendChild(), so not affected by this CL.

Wow, we're lucky.

> I don't know how I tested when I came to the conclusion that adding a <hr> had
> no observable effects, however. It quite clearly adds a thin line between
> options. (Not the greatest visual effect IMO, but it's certainly there.)
> 
> Do we think that we should restore this extended functionality of
> HTMLSelectElement.add()?

I don't think we need any additional actions for this.
 - chrome://settings/fonts works
 - New behavior is compatible with IE and Firefox.  Normal web pages don't use
<hr> in <select>.
 - Even for Chrome extensions, the usage should be almost zero.  I have never
seen any articles about this behavior, and extension developers don't know this.