Enable TypeError for dictionary members of non-nullable interface type

Enable TypeError for dictionary members of non-nullable interface type

In dictionary_v8.cpp, the extra null check was wrong or redudant
depending on nullabilty:

 * If the member isn't nullable, then passing null should throw
   TypeError.

 * If the member is nullable, then a null check is already emitted
   earlier, and so that part of the condition is always true.

The affected dictionaries were identified by looking at the resulting
changes in the generated bindings. Any affected member was made nullable
so that this change in itself is not observable, but the right behavior
is achieved if those changes are individually reverted.

A few cases that were not given TODOs:

 * MessageEventInit, because the existing TODO is appropriate.

 * MediaStreamEventInit, because it has no spec and MediaStreamEvent's
   corresponding attribute is already nullable.

 * SpeechRecognitionEventInit, because it has no spec.
   SpeechRecognitionEvent's attributes were made nullable because they
   can now in fact be null.

A number of spec issues were discovered, linked with the TODOs.

BUG=647693
Committed: https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f
Cr-Commit-Position: refs/heads/master@{#419344}

[foolip, 2016-09-16 15:47:34]

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

[commit-bot: I haz the power, 2016-09-16 15:47:46]

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

[foolip, 2016-09-16 16:31:37]

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

[foolip, 2016-09-16 16:31:38]

PTAL

[commit-bot: I haz the power, 2016-09-16 17:04:12]

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

[commit-bot: I haz the power, 2016-09-16 17:04:13]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-09-16 23:59:26]

LGTM on my side

[foolip, 2016-09-17 00:17:53]

The CQ bit was checked by foolip@chromium.org

[commit-bot: I haz the power, 2016-09-17 00:18:19]

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, 2016-09-17 00:25:03]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-17 00:29:26]

Description was changed from

==========
Enable TypeError for dictionary members of non-nullable interface type

In dictionary_v8.cpp, the extra null check was wrong or redudant
depending on nullabilty:

 * If the member isn't nullable, then passing null should throw
   TypeError.

 * If the member is nullable, then a null check is already emitted
   earlier, and so that part of the condition is always true.

The affected dictionaries were identified by looking at the resulting
changes in the generated bindings. Any affected member was made nullable
so that this change in itself is not observable, but the right behavior
is achieved if those changes are individually reverted.

A few cases that were not given TODOs:

 * MessageEventInit, because the existing TODO is appropriate.

 * MediaStreamEventInit, because it has no spec and MediaStreamEvent's
   corresponding attribute is already nullable.

 * SpeechRecognitionEventInit, because it has no spec.
   SpeechRecognitionEvent's attributes were made nullable because they
   can now in fact be null.

A number of spec issues were discovered, linked with the TODOs.

BUG=647693
==========

to

==========
Enable TypeError for dictionary members of non-nullable interface type

In dictionary_v8.cpp, the extra null check was wrong or redudant
depending on nullabilty:

 * If the member isn't nullable, then passing null should throw
   TypeError.

 * If the member is nullable, then a null check is already emitted
   earlier, and so that part of the condition is always true.

The affected dictionaries were identified by looking at the resulting
changes in the generated bindings. Any affected member was made nullable
so that this change in itself is not observable, but the right behavior
is achieved if those changes are individually reverted.

A few cases that were not given TODOs:

 * MessageEventInit, because the existing TODO is appropriate.

 * MediaStreamEventInit, because it has no spec and MediaStreamEvent's
   corresponding attribute is already nullable.

 * SpeechRecognitionEventInit, because it has no spec.
   SpeechRecognitionEvent's attributes were made nullable because they
   can now in fact be null.

A number of spec issues were discovered, linked with the TODOs.

BUG=647693

Committed: https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f
Cr-Commit-Position: refs/heads/master@{#419344}
==========

[commit-bot: I haz the power, 2016-09-17 00:29:28]

Patchset 1 (id:??) landed as
https://crrev.com/7f8965f2d8ef4ac323cc4654769e853174267f6f
Cr-Commit-Position: refs/heads/master@{#419344}

[hta - Chromium, 2016-09-19 18:19:09]

Impressive fix!

[foolip, 2016-09-19 18:49:01]

On 2016/09/19 18:19:09, hta - Chromium wrote:
> Impressive fix!

Thanks, nothing now block MediaStreamTrackEvent :)

[bashi, 2016-09-19 23:48:30]

On 2016/09/19 18:49:01, foolip wrote:
> On 2016/09/19 18:19:09, hta - Chromium wrote:
> > Impressive fix!
> 
> Thanks, nothing now block MediaStreamTrackEvent :)

Thanks for working on this! However, is this really a correct fix? According to
http://heycam.github.io/webidl/#es-dictionary, we should convert null ->
undefined and only throw a type error when the member is required. Am I missing
something?

[foolip, 2016-09-20 08:59:29]

On 2016/09/19 23:48:30, bashi1 (slow til Sep 26) wrote:
> On 2016/09/19 18:49:01, foolip wrote:
> > On 2016/09/19 18:19:09, hta - Chromium wrote:
> > > Impressive fix!
> > 
> > Thanks, nothing now block MediaStreamTrackEvent :)
> 
> Thanks for working on this! However, is this really a correct fix? According
to
> http://heycam.github.io/webidl/#es-dictionary, we should convert null ->
> undefined and only throw a type error when the member is required. Am I
missing
> something?

I think you're misreading the spec in the same way that I did in
https://crbug.com/647234. The V in http://heycam.github.io/webidl/#es-dictionary
is actually the value being converted as a dictionary, not the value for the
members being iterated. Implementing it like this would save the "Missing
required member(s):" strings from the binary, but it would perhaps make the code
a tiny bit slower due to repeated null test. It'd save this confusion though :)

[bashi, 2016-09-20 09:57:35]

On 2016/09/20 08:59:29, foolip wrote:
> On 2016/09/19 23:48:30, bashi1 (slow til Sep 26) wrote:
> > On 2016/09/19 18:49:01, foolip wrote:
> > > On 2016/09/19 18:19:09, hta - Chromium wrote:
> > > > Impressive fix!
> > > 
> > > Thanks, nothing now block MediaStreamTrackEvent :)
> > 
> > Thanks for working on this! However, is this really a correct fix? According
> to
> > http://heycam.github.io/webidl/#es-dictionary, we should convert null ->
> > undefined and only throw a type error when the member is required. Am I
> missing
> > something?
> 
> I think you're misreading the spec in the same way that I did in
> https://crbug.com/647234. The V in
http://heycam.github.io/webidl/#es-dictionary
> is actually the value being converted as a dictionary, not the value for the
> members being iterated. Implementing it like this would save the "Missing
> required member(s):" strings from the binary, but it would perhaps make the
code
> a tiny bit slower due to repeated null test. It'd save this confusion though
:)

Actually I was misreading. Thanks for the clarification!

[foolip, 2016-09-20 10:28:04]

On 2016/09/20 09:57:35, bashi1 (slow til Sep 26) wrote:
> On 2016/09/20 08:59:29, foolip wrote:
> > On 2016/09/19 23:48:30, bashi1 (slow til Sep 26) wrote:
> > > On 2016/09/19 18:49:01, foolip wrote:
> > > > On 2016/09/19 18:19:09, hta - Chromium wrote:
> > > > > Impressive fix!
> > > > 
> > > > Thanks, nothing now block MediaStreamTrackEvent :)
> > > 
> > > Thanks for working on this! However, is this really a correct fix?
According
> > to
> > > http://heycam.github.io/webidl/#es-dictionary, we should convert null ->
> > > undefined and only throw a type error when the member is required. Am I
> > missing
> > > something?
> > 
> > I think you're misreading the spec in the same way that I did in
> > https://crbug.com/647234. The V in
> http://heycam.github.io/webidl/#es-dictionary
> > is actually the value being converted as a dictionary, not the value for the
> > members being iterated. Implementing it like this would save the "Missing
> > required member(s):" strings from the binary, but it would perhaps make the
> code
> > a tiny bit slower due to repeated null test. It'd save this confusion though
> :)
> 
> Actually I was misreading. Thanks for the clarification!

I'm glad I wasn't the only one, I wasted about half a day on that
misunderstanding :)