bindings: Ignores the last undefined arguments when counting the args.

bindings: Ignores the last undefined arguments when counting the args.

foo(arg1, arg2, undefined) should be interpreted as foo(arg1, arg2).
This CL checks if the last arguments are undefined or not, and counts
the number of actual arguments.

With this change, overloaded C++ functions, such as foo(int, int, T*)
and foo(int, int), will be resolved correctly.  Note that
    DOMTokenList::toggle("name")
and
    DOMTokenList::toggle("name", false)
behave *differently*.  (Oh, my...)
So, it's important to count the actual number of arguments correctly.

BUG=489665
Committed: https://crrev.com/a9a901bc9a158c5e60cda9bbb96fa50ce0c76489
Cr-Commit-Position: refs/heads/master@{#362635}

[Yuki, 2015-11-26 10:06:26]

yukishiino@chromium.org changed reviewers:
+ bashi@chromium.org

[Yuki, 2015-11-26 10:06:26]

Could you review this CL?

[bashi, 2015-11-26 23:37:18]

The change itself looks good. Could you confirm that the behavior change aligns
with Firefox?

https://codereview.chromium.org/1476863003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/1476863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_methods.py:155: any(True for
argument_context in argument_contexts
Can we have a helper function for |has_optional_argument_without_default_value|
and use it in both v8_method.py and v8_interface.py?

https://codereview.chromium.org/1476863003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/1476863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/methods.cpp:112: {% if
argument.is_optional and not argument.has_default and
{% if method.has_optional_argument_without_default_value %}

[Yuki, 2015-11-30 06:53:23]

For the issue 489665, this CL behaves the same as Firefox.

I also confirmed that CanvasRenderingContext2D.clip() behaves the same for
undefined values for optional arguments.

https://codereview.chromium.org/1476863003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_methods.py (right):

https://codereview.chromium.org/1476863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_methods.py:155: any(True for
argument_context in argument_contexts
On 2015/11/26 23:37:18, bashi1 wrote:
> Can we have a helper function for
|has_optional_argument_without_default_value|
> and use it in both v8_method.py and v8_interface.py?

Done.

https://codereview.chromium.org/1476863003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/1476863003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/methods.cpp:112: {% if
argument.is_optional and not argument.has_default and
On 2015/11/26 23:37:18, bashi1 wrote:
> {% if method.has_optional_argument_without_default_value %}

It's not |method|, but done.

[bashi, 2015-11-30 23:13:26]

Thank you for the clarification! LGTM.

[haraken, 2015-11-30 23:36:57]

LGTM

[jsbell, 2015-12-01 00:31:02]

jsbell@chromium.org changed reviewers:
+ jsbell@chromium.org

[jsbell, 2015-12-01 00:31:02]

Awesome! Quick notes about the indexeddb tests...

https://codereview.chromium.org/1476863003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/imported/web-platform-tests/IndexedDB/idbfactory_open9-expected.txt
(right):

https://codereview.chromium.org/1476863003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/imported/web-platform-tests/IndexedDB/idbfactory_open9-expected.txt:12:
FAIL Calling open() with version argument undefined should throw TypeError.
assert_throws: function "function () {
It looks like the new behavior will match Firefox here - yay!

That means the test is wrong. Can you file a bug against web-platform-tests ?

https://codereview.chromium.org/1476863003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/storage/indexeddb/resources/intversion-bad-parameters.js
(right):

https://codereview.chromium.org/1476863003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/intversion-bad-parameters.js:28:
evalAndLog("indexedDB.open(dbname, undefined)");
Can you change this to shouldNotThrow() and add a comment here, since this
assertion no longer makes sense without knowing the history of the file?

[jsbell, 2015-12-01 00:35:26]

On 2015/12/01 00:31:02, jsbell wrote:
> That means the test is wrong. Can you file a bug against web-platform-tests ?

Never mind, I filed it:

https://github.com/w3c/web-platform-tests/issues/2389

[Yuki, 2015-12-01 05:20:44]

On 2015/12/01 00:35:26, jsbell wrote:
> On 2015/12/01 00:31:02, jsbell wrote:
> > That means the test is wrong. Can you file a bug against web-platform-tests
?
> 
> Never mind, I filed it:
> 
> https://github.com/w3c/web-platform-tests/issues/2389

Thanks for filing the bug.

[Yuki, 2015-12-01 05:57:44]

https://codereview.chromium.org/1476863003/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/storage/indexeddb/resources/intversion-bad-parameters.js
(right):

https://codereview.chromium.org/1476863003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/storage/indexeddb/resources/intversion-bad-parameters.js:28:
evalAndLog("indexedDB.open(dbname, undefined)");
On 2015/12/01 00:31:02, jsbell wrote:
> Can you change this to shouldNotThrow() and add a comment here, since this
> assertion no longer makes sense without knowing the history of the file?

Done.

[jsbell, 2015-12-01 17:21:14]

lgtm!

[Yuki, 2015-12-02 04:58:00]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2015-12-02 04:58:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, bashi@chromium.org
Link to the patchset: https://codereview.chromium.org/1476863003/#ps100001
(title: "Addressed a review comment.")

[commit-bot: I haz the power, 2015-12-02 04:58:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1476863003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1476863003/100001

[commit-bot: I haz the power, 2015-12-02 05:04:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-12-02 05:05:33]

Description was changed from

==========
bindings: Ignores the last undefined arguments when counting the args.

foo(arg1, arg2, undefined) should be interpreted as foo(arg1, arg2).
This CL checks if the last arguments are undefined or not, and counts
the number of actual arguments.

With this change, overloaded C++ functions, such as foo(int, int, T*)
and foo(int, int), will be resolved correctly.  Note that
    DOMTokenList::toggle("name")
and
    DOMTokenList::toggle("name", false)
behave *differently*.  (Oh, my...)
So, it's important to count the actual number of arguments correctly.

BUG=489665
==========

to

==========
bindings: Ignores the last undefined arguments when counting the args.

foo(arg1, arg2, undefined) should be interpreted as foo(arg1, arg2).
This CL checks if the last arguments are undefined or not, and counts
the number of actual arguments.

With this change, overloaded C++ functions, such as foo(int, int, T*)
and foo(int, int), will be resolved correctly.  Note that
    DOMTokenList::toggle("name")
and
    DOMTokenList::toggle("name", false)
behave *differently*.  (Oh, my...)
So, it's important to count the actual number of arguments correctly.

BUG=489665

Committed: https://crrev.com/a9a901bc9a158c5e60cda9bbb96fa50ce0c76489
Cr-Commit-Position: refs/heads/master@{#362635}
==========

[commit-bot: I haz the power, 2015-12-02 05:05:33]

Patchset 6 (id:??) landed as
https://crrev.com/a9a901bc9a158c5e60cda9bbb96fa50ce0c76489
Cr-Commit-Position: refs/heads/master@{#362635}