Make Document.createTouch arguments non-optional

Make Document.createTouch arguments non-optional

BUG=701549
Review-Url: https://codereview.chromium.org/2747333003
Cr-Commit-Position: refs/heads/master@{#458909}
Committed: https://chromium.googlesource.com/chromium/src/+/6e6da892d1137c8d2b4f5cf84caaf440b5785a40

[lunalu1, 2017-03-15 19:04:15]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 19:04:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-15 19:13:13]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 19:13:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-15 19:56:41]

lunalu@chromium.org changed reviewers:
+ foolip@chromium.org

[lunalu1, 2017-03-15 19:56:41]

Make Document.createTouch non-optional to match the spec. PTAL

[commit-bot: I haz the power, 2017-03-15 20:47:08]

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

[commit-bot: I haz the power, 2017-03-15 20:47:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lunalu1, 2017-03-15 21:31:49]

Description was changed from

==========
Make Document.createTouch arguments non-optional

BUG=701549
==========

to

==========
Make Document.createTouch arguments non-optional

BUG=701549
==========

[lunalu1, 2017-03-15 21:31:49]

lunalu@chromium.org changed reviewers:
- foolip@chromium.org

[lunalu1, 2017-03-16 00:59:35]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-16 00:59:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 02:25:02]

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

[commit-bot: I haz the power, 2017-03-16 02:25:03]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lunalu1, 2017-03-16 15:20:12]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2017-03-16 15:20:31]

lunalu@chromium.org changed reviewers:
+ foolip@chromium.org

[lunalu1, 2017-03-16 15:20:32]

PTAL

[commit-bot: I haz the power, 2017-03-16 15:20:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-16 16:57:04]

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

[commit-bot: I haz the power, 2017-03-16 16:57:05]

Dry run: This issue passed the CQ dry run.

[foolip, 2017-03-21 08:01:13]

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch-expected.txt
(right):

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch-expected.txt:18:
PASS document.createTouch() threw exception TypeError: Failed to execute
'createTouch' on 'Document': 7 arguments required, but only 0 present..
If there aren't any failing tests left, can you try just removing the expected
file?

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch.html
(right):

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch.html:57:
var badParamsTouch = document.createTouch(function(x) { return x; }, 12, 'a',
100, 101, 102, 103, function(x) { return x; }, 'a', 'b', 'c');
Is this test change needed to get coverage of some type conversion?

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch.html:72:
shouldBeNonNull("detachedTouch = document.implementation.createDocument('a',
'b').createTouch(window, target, 1, 100, 101, 102, 103)");
This was a crash test, so it's probably best to preserve the old behavior as
closely as possible, by passing null or 0 as appropriate.

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/touch/touch-event-dispatch-no-crash.html
(right):

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/touch-event-dispatch-no-crash.html:9:
var box = document.createElement("div");
Why these changes? Just adding null and 0 arguments to the createTouch call
should suffice.

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.idl (right):

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.idl:168: [Default=Undefined]
optional unrestricted double radiusX,
Can you change these to "optional unrestricted double radiusX = 0" and similar
to get rid of the remaining [Default=Undefined] here?

[lunalu1, 2017-03-21 19:19:07]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[lunalu1, 2017-03-21 19:19:27]

Please see the in-line reply.

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch-expected.txt
(right):

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch-expected.txt:18:
PASS document.createTouch() threw exception TypeError: Failed to execute
'createTouch' on 'Document': 7 arguments required, but only 0 present..
On 2017/03/21 08:01:13, foolip_UTC9 wrote:
> If there aren't any failing tests left, can you try just removing the expected
> file?

Somehow even though there are all PASS'es, the bot complains about the missing
test expect.

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch.html
(right):

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch.html:57:
var badParamsTouch = document.createTouch(function(x) { return x; }, 12, 'a',
100, 101, 102, 103, function(x) { return x; }, 'a', 'b', 'c');
On 2017/03/21 08:01:13, foolip_UTC9 wrote:
> Is this test change needed to get coverage of some type conversion?

This tests that window can be a function, target can be a number, and 
identifier can be a char, and radiusX can be a function?

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch.html:72:
shouldBeNonNull("detachedTouch = document.implementation.createDocument('a',
'b').createTouch(window, target, 1, 100, 101, 102, 103)");
On 2017/03/21 08:01:13, foolip_UTC9 wrote:
> This was a crash test, so it's probably best to preserve the old behavior as
> closely as possible, by passing null or 0 as appropriate.

Done.

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/touch/touch-event-dispatch-no-crash.html
(right):

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/touch-event-dispatch-no-crash.html:9:
var box = document.createElement("div");
On 2017/03/21 08:01:13, foolip_UTC9 wrote:
> Why these changes? Just adding null and 0 arguments to the createTouch call
> should suffice.

Done.

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.idl (right):

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.idl:168: [Default=Undefined]
optional unrestricted double radiusX,
On 2017/03/21 08:01:13, foolip_UTC9 wrote:
> Can you change these to "optional unrestricted double radiusX = 0" and similar
> to get rid of the remaining [Default=Undefined] here?

OK. The spec didn't mention these arguments. So maybe we should confirm with the
someone on the team if this is safe to do?

[commit-bot: I haz the power, 2017-03-21 19:20:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 19:49:25]

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

[commit-bot: I haz the power, 2017-03-21 19:49:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[foolip, 2017-03-22 05:16:24]

Oh, right, can you remove the DocumentCreateTouchLessThanSevenArguments use
counter too? I added it just to see if a change like this would be safe.

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch-expected.txt
(right):

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch-expected.txt:18:
PASS document.createTouch() threw exception TypeError: Failed to execute
'createTouch' on 'Document': 7 arguments required, but only 0 present..
On 2017/03/21 19:19:27, loonybear wrote:
> On 2017/03/21 08:01:13, foolip_UTC9 wrote:
> > If there aren't any failing tests left, can you try just removing the
expected
> > file?
> 
> Somehow even though there are all PASS'es, the bot complains about the missing
> test expect. 

Oh right, this isn't a testharness.js test :)

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch.html
(right):

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch.html:57:
var badParamsTouch = document.createTouch(function(x) { return x; }, 12, 'a',
100, 101, 102, 103, function(x) { return x; }, 'a', 'b', 'c');
On 2017/03/21 19:19:27, loonybear wrote:
> On 2017/03/21 08:01:13, foolip_UTC9 wrote:
> > Is this test change needed to get coverage of some type conversion?
> 
> This tests that window can be a function, target can be a number, and 
> identifier can be a char, and radiusX can be a function?

But pageX/Y and screenX/Y are now tested with numbers, which are the correct
argument types. I don't know why 104 was used here before, but unless there's a
type conversion that was untested, I'd leave this part of the test unchanged.

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.idl (right):

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.idl:168: [Default=Undefined]
optional unrestricted double radiusX,
On 2017/03/21 19:19:27, loonybear wrote:
> On 2017/03/21 08:01:13, foolip_UTC9 wrote:
> > Can you change these to "optional unrestricted double radiusX = 0" and
similar
> > to get rid of the remaining [Default=Undefined] here?
> 
> OK. The spec didn't mention these arguments. So maybe we should confirm with
the
> someone on the team if this is safe to do? 

The generated code will be slightly different, but it'll amount to the same
thing, so it's a safe change to make.

[foolip, 2017-03-22 05:16:37]

lgtm % remaining nit

[lunalu1, 2017-03-22 17:55:06]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2017-03-22 17:55:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-22 17:56:03]

I already handled the remaining nit (updating the idl file and keep minimal
changes of the test) in the last patch?

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch.html
(right):

https://codereview.chromium.org/2747333003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/events/touch/document-create-touch.html:57:
var badParamsTouch = document.createTouch(function(x) { return x; }, 12, 'a',
100, 101, 102, 103, function(x) { return x; }, 'a', 'b', 'c');
On 2017/03/22 05:16:24, foolip_UTC9 wrote:
> On 2017/03/21 19:19:27, loonybear wrote:
> > On 2017/03/21 08:01:13, foolip_UTC9 wrote:
> > > Is this test change needed to get coverage of some type conversion?
> > 
> > This tests that window can be a function, target can be a number, and 
> > identifier can be a char, and radiusX can be a function?
> 
> But pageX/Y and screenX/Y are now tested with numbers, which are the correct
> argument types. I don't know why 104 was used here before, but unless there's
a
> type conversion that was untested, I'd leave this part of the test unchanged.

Right. It was 104 in the original test (so I guess I will just leave it as it
id?). 
If I leave this part of the test unchanged this will crash as we no longer able
to
convert char to double.

[lunalu1, 2017-03-22 17:59:32]

The CQ bit was checked by lunalu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 18:00:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lunalu1, 2017-03-22 18:00:34]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-03-22 19:16:20]

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

[commit-bot: I haz the power, 2017-03-22 19:16:21]

Dry run: This issue passed the CQ dry run.

[lunalu1, 2017-03-22 20:28:37]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2017-03-22 20:29:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 22:48:23]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490214517421860,
"parent_rev": "83d76e4a71a6d7acb908fb858165bcdd74040266", "commit_rev":
"6e6da892d1137c8d2b4f5cf84caaf440b5785a40"}

[commit-bot: I haz the power, 2017-03-22 22:49:06]

Description was changed from

==========
Make Document.createTouch arguments non-optional

BUG=701549
==========

to

==========
Make Document.createTouch arguments non-optional

BUG=701549

Review-Url: https://codereview.chromium.org/2747333003
Cr-Commit-Position: refs/heads/master@{#458909}
Committed:
https://chromium.googlesource.com/chromium/src/+/6e6da892d1137c8d2b4f5cf84caa...
==========

[commit-bot: I haz the power, 2017-03-22 22:49:08]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/6e6da892d1137c8d2b4f5cf84caa...

[foolip, 2017-03-23 07:01:43]

On 2017/03/22 17:56:03, loonybear wrote:
> I already handled the remaining nit (updating the idl file and keep minimal
> changes of the test) in the last patch?

The remaining nit was the UseCounter.h change.