Make CustomEvent#initCustomEvent match the spec

Make CustomEvent#initCustomEvent match the spec

BUG=701555
Review-Url: https://codereview.chromium.org/2754713002
Cr-Commit-Position: refs/heads/master@{#458572}
Committed: https://chromium.googlesource.com/chromium/src/+/9764445b1b6c87a3cabf085b63af1d549b079a6b

[lunalu1, 2017-03-15 20:44:28]

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

[commit-bot: I haz the power, 2017-03-15 20:45:14]

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-15 22:36:13]

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

[commit-bot: I haz the power, 2017-03-15 22:36:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[lunalu1, 2017-03-16 18:15:55]

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

[commit-bot: I haz the power, 2017-03-16 18:16:46]

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-16 18:28:44]

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

[lunalu1, 2017-03-16 18:28:59]

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

[lunalu1, 2017-03-16 18:28:59]

PTAL

[commit-bot: I haz the power, 2017-03-16 18:29:14]

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 20:14:32]

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

[commit-bot: I haz the power, 2017-03-16 20:14:33]

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 21:55:41]

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

[commit-bot: I haz the power, 2017-03-16 21:56:15]

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 23:25:39]

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

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

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[foolip, 2017-03-21 08:14:38]

lgtm % DHECK_GE

https://codereview.chromium.org/2754713002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp
(right):

https://codereview.chromium.org/2754713002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp:109:
DCHECK_GE(info.Length(), 1);
I think it's OK to just remove the assert, it was there to show why the info[3]
access was safe, but now we have a conditional instead since we can reach this
code with <4 arguments.

[lunalu1, 2017-03-21 17:32:41]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2017-03-21 17:32:42]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2754713002/#ps80001
(title: "Codereview: nit + Update tests expect after rebase")

[lunalu1, 2017-03-21 17:33:23]

Done. Thanks

https://codereview.chromium.org/2754713002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp
(right):

https://codereview.chromium.org/2754713002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp:109:
DCHECK_GE(info.Length(), 1);
On 2017/03/21 08:14:38, foolip_UTC9 wrote:
> I think it's OK to just remove the assert, it was there to show why the
info[3]
> access was safe, but now we have a conditional instead since we can reach this
> code with <4 arguments.

This doesn't make sense. If we want to show why info[3] is access-safe,
shouldn't 
we be checking info.length() >= 4? But I will remove the ASSERT / DCHECK
anyways.

[commit-bot: I haz the power, 2017-03-21 17:34:15]

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

[lunalu1, 2017-03-21 18:37:08]

The CQ bit was checked by lunalu@chromium.org

[lunalu1, 2017-03-21 18:37:09]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2754713002/#ps100001
(title: "Update test expect")

[commit-bot: I haz the power, 2017-03-21 18:37:59]

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

[lunalu1, 2017-03-21 18:40:40]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-03-21 19:59:28]

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

[commit-bot: I haz the power, 2017-03-21 19:59:29]

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

[lunalu1, 2017-03-21 20:45:05]

The CQ bit was checked by lunalu@chromium.org

[commit-bot: I haz the power, 2017-03-21 20:45:43]

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 22:03:44]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490129105447340,
"parent_rev": "0c51a5fdf9f27c4758e4ae2b2b54dff2b8890181", "commit_rev":
"9764445b1b6c87a3cabf085b63af1d549b079a6b"}

[commit-bot: I haz the power, 2017-03-21 22:04:34]

Description was changed from

==========
Make CustomEvent#initCustomEvent match the spec

BUG=701555
==========

to

==========
Make CustomEvent#initCustomEvent match the spec

BUG=701555

Review-Url: https://codereview.chromium.org/2754713002
Cr-Commit-Position: refs/heads/master@{#458572}
Committed:
https://chromium.googlesource.com/chromium/src/+/9764445b1b6c87a3cabf085b63af...
==========

[commit-bot: I haz the power, 2017-03-21 22:04:36]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/9764445b1b6c87a3cabf085b63af...

[foolip, 2017-03-22 05:38:40]

https://codereview.chromium.org/2754713002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp
(right):

https://codereview.chromium.org/2754713002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp:109:
DCHECK_GE(info.Length(), 1);
On 2017/03/21 17:33:23, loonybear wrote:
> On 2017/03/21 08:14:38, foolip_UTC9 wrote:
> > I think it's OK to just remove the assert, it was there to show why the
> info[3]
> > access was safe, but now we have a conditional instead since we can reach
this
> > code with <4 arguments.
> 
> This doesn't make sense. If we want to show why info[3] is access-safe,
> shouldn't 
> we be checking info.length() >= 4? But I will remove the ASSERT / DCHECK
> anyways. 

The info[3] access is now guarded by if (info.length() >= 4), making that
clearly safe. What remained of the assert I think would have essentially been
verifying that the generated code for initCustomEventMethod correctly returns
early if given no arguments, but for no particular reason, and not helping make
any guarantees for the code that follows.