Add overload support for non-wrapper type vs. primitive type

Add overload support for non-wrapper type vs. primitive type

Currently the overload resolution algorithm cannot handle non-wrapper types,
notably Dictionary.
This is causing problems for Web Animations, as per below CL,
specifically for Dictionary vs. double.
This is a conservative change, adding basic support for overloading
non-wrapper vs. primitive types.

This is simple, requiring that definitions appear in a given order:
methods with non-wrapper type arguments before
methods with primitive type arguments.

This bug can't be fixed without completely rewriting the overload algorithm,
b/c currently it only processes one method at a time,
but actually needs to consider the full overload set all at once,
which is much, much more complicated.
(Alternatively we could add a !IsObject to all checks of primitive arguments,
 but that's slow and unnecessary.)

Reference CL:
Web Animations API: Start implementation of timing input objects.
https://codereview.chromium.org/105273010/

BUG=293561
R=haraken

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

[Nils Barth (inactive), 2014-01-23 05:07:55]

Quick Perl CL per Renée's req.

See that correct overload is being computed in:
V8TestObject.cpp

Direct link:
https://codereview.chromium.org/131973012/diff/1/Source/bindings/tests/result...

[haraken, 2014-01-23 05:14:06]

LGTM. This CL is not correct but improves our situation.

rjwright: would you check if this CL works for you?

[Nils Barth (inactive), 2014-01-23 05:15:20]

As reflected in the change to the constructor test file, this potentially
changes existing bindings, due to added IsObject test.

In practice this results in one change, to IDBObjectStore.idl,
(specifically in createIndexMethod)
due to:
IDBIndex createIndex(DOMString name, sequence<DOMString> keyPath, optional
Dictionary options);
IDBIndex createIndex(DOMString name, DOMString keyPath, optional Dictionary
options);

(Note the Dictionary argument at the end.)
In practice this makes no difference, as these are distinguished by the 2nd
argument (array or not), so the third argument doesn't matter.

Note also that this IDB IDL file also requires that definitions
be in order of "primitive last", since
sequence<DOMString>
...has a test but
DOMString
...doesn't.

Exact diff is:
<     if (((info.Length() == 2) && (info[1]->IsArray())) || ((info.Length() ==
3) && (info[1]->IsArray()))) {
---
>     if (((info.Length() == 2) && (info[1]->IsArray())) || ((info.Length() ==
3) && (info[1]->IsArray()) && (info[2]->IsObject()))) {
390c390
<     if (((info.Length() == 2)) || ((info.Length() == 3))) {
---
>     if (((info.Length() == 2)) || ((info.Length() == 3) &&
(info[2]->IsObject()))) {

[Nils Barth (inactive), 2014-01-23 05:20:18]

On 2014/01/23 05:14:06, haraken wrote:
> LGTM. This CL is not correct but improves our situation.

Thanks haraken!
Turns out it's no less correct than existing overloads
(e.g., sequence<DOMString> vs. DOMString), as used in real IDL files.

I've updated the comment to make this clear.

> rjwright: would you check if this CL works for you?

I'll hold off on landing this until verify that it works.

[Nils Barth (inactive), 2014-01-23 06:15:30]

Test failures due to changed error message:
parameter 3 ('options') is not an object
=>
No function was found that matched the signature provided

Rebaselined.

Joshua, you may be able to remove some typechecking in IDB, now that we're doing
it in the generated bindings.

[Nils Barth (inactive), 2014-01-23 06:17:06]

On 2014/01/23 05:20:18, Nils Barth wrote:
> I'll hold off on landing this until verify that it works.

Confirmed working
(for suitable choice of IDL, and at least w/o Custom bindings);
committing.

[commit-bot: I haz the power, 2014-01-23 06:17:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/131973012/40007

[Nils Barth (inactive), 2014-01-23 06:20:53]

On 2014/01/23 06:15:30, Nils Barth wrote:
> Joshua, you may be able to remove some typechecking in IDB, now that we're
doing
> it in the generated bindings.

Sorry, this is wrong:
both errors are from the bindings,
but we're now catching it in the overload resolution method,
not in the resolved method.

So Joshua, nothing to see here for you.
(Other than "overloading works a bit better")

[commit-bot: I haz the power, 2014-01-23 09:21:31]

Change committed as 165620

[jsbell, 2014-01-23 17:03:28]

On 2014/01/23 06:20:53, Nils Barth wrote:
> So Joshua, nothing to see here for you.
> (Other than "overloading works a bit better")

:)

Keep up the awesome!

[Nils Barth (inactive), 2014-02-04 02:18:39]

This caused a bug:
IndexedDB: createIndex fails if undefined is passed for options
https://code.google.com/p/chromium/issues/detail?id=340458

...resolved in this CL:
IDL: allow optional values to be undefined in overload resolution
https://codereview.chromium.org/152413005/

(We got stricter about undefined, and so we added an explicit test to allow
undefined for optional arguments.)