binding: Makes Dictionary throw an exception (constructor).

binding: Makes Dictionary throw an exception (constructor).

As part of an effort to make Dictionary handle an exception
correctly, this CL makes Dictionary's ctor take an
ExceptionState and rethrow an exception if any when converting
an ECMAScript value to an IDL dictionary value.

BUG=666661
Committed: https://crrev.com/1f6b9a3ebd8b058a5f07ae5393bbd7469ad030bf
Cr-Commit-Position: refs/heads/master@{#433529}

[Yuki, 2016-11-16 14:51:49]

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

[commit-bot: I haz the power, 2016-11-16 14:52:11]

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

[Yuki, 2016-11-16 15:03:25]

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

[Yuki, 2016-11-16 15:03:26]

Could you guys review this CL?

This is a follow-up CL of http://crrev.com/2496533002 .

[commit-bot: I haz the power, 2016-11-16 16:12:49]

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

[commit-bot: I haz the power, 2016-11-16 16:12:50]

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

[bashi, 2016-11-16 17:01:54]

Change it self looks good. Could you fix unit test failures?

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ArrayValue.cpp (right):

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ArrayValue.cpp:66: //
TODO(bashi,yukishiino): Should rethrow the exception.
nit: File a bug to track this issue?

[haraken, 2016-11-16 17:28:57]

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp (right):

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp:47: v8::TryCatch
tryCatch(isolate);

I'd prefer checking IsObject and throw TypeError before calling ToObject.

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right):

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/Dictionary.h:88:
v8::Local<v8::Object> m_dictObj;  // an Object (same as m_value) or empty

Do we need m_dictObj? You're storing m_dictObj to m_value immediately after
converting the dictionary.

m_dictObj => m_dictionaryObject

[Yuki, 2016-11-17 07:38:53]

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right):

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/Dictionary.h:88:
v8::Local<v8::Object> m_dictObj;  // an Object (same as m_value) or empty
On 2016/11/16 17:28:57, haraken wrote:
> 
> Do we need m_dictObj? You're storing m_dictObj to m_value immediately after
> converting the dictionary.
> 
> m_dictObj => m_dictionaryObject

We need to distinguish undefined / null / an Object.  Without
m_dictionaryObject, we need to do
    m_value->IsObject()
and
    m_value.As<v8::Object>().

The advantages of having m_dictionaryObject are:
a) !m_dictionaryObject.IsEmpty() is faster than m_value->IsObject()
b) m_dictionaryObject is shorter than m_value.As<v8::Object>() and easy to read.
c) it's trivial that m_dictionaryObject is a dictionary object while m_value can
be anything.

Since we often use m_dictionaryObject as v8::Object, I'd like these advantages.

[haraken, 2016-11-17 08:16:45]

On 2016/11/17 07:38:53, Yuki wrote:
>
https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right):
> 
>
https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/Dictionary.h:88:
> v8::Local<v8::Object> m_dictObj;  // an Object (same as m_value) or empty
> On 2016/11/16 17:28:57, haraken wrote:
> > 
> > Do we need m_dictObj? You're storing m_dictObj to m_value immediately after
> > converting the dictionary.
> > 
> > m_dictObj => m_dictionaryObject
> 
> We need to distinguish undefined / null / an Object.  Without
> m_dictionaryObject, we need to do
>     m_value->IsObject()
> and
>     m_value.As<v8::Object>().
> 
> The advantages of having m_dictionaryObject are:
> a) !m_dictionaryObject.IsEmpty() is faster than m_value->IsObject()
> b) m_dictionaryObject is shorter than m_value.As<v8::Object>() and easy to
read.
> c) it's trivial that m_dictionaryObject is a dictionary object while m_value
can
> be anything.
> 
> Since we often use m_dictionaryObject as v8::Object, I'd like these
advantages.

Thanks, makes sense. LGTM.

m_value => m_originalValue
m_dictObj => m_convertedObject

?

[Yuki, 2016-11-17 08:24:01]

On 2016/11/17 08:16:45, haraken wrote:
> On 2016/11/17 07:38:53, Yuki wrote:
> >
>
https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right):
> > 
> >
>
https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/core/v8/Dictionary.h:88:
> > v8::Local<v8::Object> m_dictObj;  // an Object (same as m_value) or empty
> > On 2016/11/16 17:28:57, haraken wrote:
> > > 
> > > Do we need m_dictObj? You're storing m_dictObj to m_value immediately
after
> > > converting the dictionary.
> > > 
> > > m_dictObj => m_dictionaryObject
> > 
> > We need to distinguish undefined / null / an Object.  Without
> > m_dictionaryObject, we need to do
> >     m_value->IsObject()
> > and
> >     m_value.As<v8::Object>().
> > 
> > The advantages of having m_dictionaryObject are:
> > a) !m_dictionaryObject.IsEmpty() is faster than m_value->IsObject()
> > b) m_dictionaryObject is shorter than m_value.As<v8::Object>() and easy to
> read.
> > c) it's trivial that m_dictionaryObject is a dictionary object while m_value
> can
> > be anything.
> > 
> > Since we often use m_dictionaryObject as v8::Object, I'd like these
> advantages.
> 
> Thanks, makes sense. LGTM.
> 
> m_value => m_originalValue
> m_dictObj => m_convertedObject
> 
> ?

If you're not comfortable with having m_value, then we can use an enum value.
    enum {kUndefined, kNull, kObject} m_valueType;
    v8::Local<v8::Object> m_dictionaryObject;  // empty or an Object

The reason why we need m_value or m_valueType is just to support
    v8::Local<v8::Value> Dictioanry::v8Value() const;
What we need here is to return undefined / null / an Object correctly, not to
return the *original* value.  So, I don't like m_*original*Value.

[haraken, 2016-11-17 15:02:24]

On 2016/11/17 08:24:01, Yuki wrote:
> On 2016/11/17 08:16:45, haraken wrote:
> > On 2016/11/17 07:38:53, Yuki wrote:
> > >
> >
>
https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
> > > File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
> > > third_party/WebKit/Source/bindings/core/v8/Dictionary.h:88:
> > > v8::Local<v8::Object> m_dictObj;  // an Object (same as m_value) or empty
> > > On 2016/11/16 17:28:57, haraken wrote:
> > > > 
> > > > Do we need m_dictObj? You're storing m_dictObj to m_value immediately
> after
> > > > converting the dictionary.
> > > > 
> > > > m_dictObj => m_dictionaryObject
> > > 
> > > We need to distinguish undefined / null / an Object.  Without
> > > m_dictionaryObject, we need to do
> > >     m_value->IsObject()
> > > and
> > >     m_value.As<v8::Object>().
> > > 
> > > The advantages of having m_dictionaryObject are:
> > > a) !m_dictionaryObject.IsEmpty() is faster than m_value->IsObject()
> > > b) m_dictionaryObject is shorter than m_value.As<v8::Object>() and easy to
> > read.
> > > c) it's trivial that m_dictionaryObject is a dictionary object while
m_value
> > can
> > > be anything.
> > > 
> > > Since we often use m_dictionaryObject as v8::Object, I'd like these
> > advantages.
> > 
> > Thanks, makes sense. LGTM.
> > 
> > m_value => m_originalValue
> > m_dictObj => m_convertedObject
> > 
> > ?
> 
> If you're not comfortable with having m_value, then we can use an enum value.
>     enum {kUndefined, kNull, kObject} m_valueType;
>     v8::Local<v8::Object> m_dictionaryObject;  // empty or an Object

Yeah, that sounds nicer to me :)

> 
> The reason why we need m_value or m_valueType is just to support
>     v8::Local<v8::Value> Dictioanry::v8Value() const;
> What we need here is to return undefined / null / an Object correctly, not to
> return the *original* value.  So, I don't like m_*original*Value.

[Yuki, 2016-11-18 08:30:10]

Description was changed from

==========
binding: Makes Dictionary throw an exception (constructor).

As part of an effort to make Dictionary handle an exception
correctly, this CL makes Dictionary's ctor take an
ExceptionState and rethrow an exception if any when converting
an ECMAScript value to an IDL dictionary value.

BUG=
==========

to

==========
binding: Makes Dictionary throw an exception (constructor).

As part of an effort to make Dictionary handle an exception
correctly, this CL makes Dictionary's ctor take an
ExceptionState and rethrow an exception if any when converting
an ECMAScript value to an IDL dictionary value.

BUG=666661
==========

[Yuki, 2016-11-18 09:12:34]

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

[commit-bot: I haz the power, 2016-11-18 09:12:54]

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, 2016-11-18 10:57:58]

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

[commit-bot: I haz the power, 2016-11-18 10:57:58]

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_...)

[Yuki, 2016-11-18 11:22:52]

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

[commit-bot: I haz the power, 2016-11-18 11:23:10]

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

[Yuki, 2016-11-18 11:30:07]

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

[commit-bot: I haz the power, 2016-11-18 11:30:24]

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

[Yuki, 2016-11-18 13:37:53]

Could you guys take another look?

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ArrayValue.cpp (right):

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ArrayValue.cpp:66: //
TODO(bashi,yukishiino): Should rethrow the exception.
On 2016/11/16 17:01:54, bashi1 wrote:
> nit: File a bug to track this issue?

Done.  http://crbug.com/666661

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp (right):

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp:47: v8::TryCatch
tryCatch(isolate);
On 2016/11/16 17:28:57, haraken wrote:
> 
> I'd prefer checking IsObject and throw TypeError before calling ToObject.

Done.

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right):

https://codereview.chromium.org/2509713002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/Dictionary.h:88:
v8::Local<v8::Object> m_dictObj;  // an Object (same as m_value) or empty
On 2016/11/16 17:28:57, haraken wrote:
> 
> Do we need m_dictObj? You're storing m_dictObj to m_value immediately after
> converting the dictionary.
> 
> m_dictObj => m_dictionaryObject

Done.

[commit-bot: I haz the power, 2016-11-18 14:11:24]

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

[commit-bot: I haz the power, 2016-11-18 14:11:24]

Dry run: This issue passed the CQ dry run.

[bashi, 2016-11-18 15:03:55]

still lgtm w/ nits

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp (right):

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp:40: //
https://heycam.github.io/webidl/#es-dictionary
I'm not sure "Dictionary" class is a "WebIDL dictionary". I think "Dictionary"
class may not conform "WebIDL dictionary" in its nature (e.g. we call [[Get]]
multiple time)

I'd prefer not adding this link. But I'll defer to you.

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp:95: //
TODO(bashi,yukishiino): Should rethrow the exception.
nit: wouldn't it worth referring a bug entry ?

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right):

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/Dictionary.h:55: Dictionary&
operator=(const Dictionary&) = default;
Just curious, why do we need this explicit operator=()?

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/modules/v8/serialization/V8ScriptValueSerializerForModulesTest.cpp
(right):

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/serialization/V8ScriptValueSerializerForModulesTest.cpp:245:
NonThrowableExceptionState exceptionState;
Should we add a comment ("Should rethrow the exception") here?

[haraken, 2016-11-18 15:52:41]

LGTM

[Yuki, 2016-11-21 12:40:27]

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp (right):

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp:40: //
https://heycam.github.io/webidl/#es-dictionary
On 2016/11/18 15:03:55, bashi1 wrote:
> I'm not sure "Dictionary" class is a "WebIDL dictionary". I think "Dictionary"
> class may not conform "WebIDL dictionary" in its nature (e.g. we call [[Get]]
> multiple time)
> 
> I'd prefer not adding this link. But I'll defer to you.

Well, ... but you said that the call sites of Dictionary should call get() only
once.

I understand that this Dictionary class does NOT perfectly conform to the spec,
but this is the closest one, isn't it?  As we don't have a plan to add
IDLDictionary class so far, and as we're not perfect at many places / things, we
don't need to be so nervous about such a violation.

The reason why we check the type here and why only undefined, null and object
are allowed is just because Web IDL accepts these things for IDL Dictionary. 
Thus, the link is very informative, I think.

Don't be too nervous.  ;)

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/Dictionary.cpp:95: //
TODO(bashi,yukishiino): Should rethrow the exception.
On 2016/11/18 15:03:55, bashi1 wrote:
> nit: wouldn't it worth referring a bug entry ?

Done.

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/Dictionary.h (right):

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/Dictionary.h:55: Dictionary&
operator=(const Dictionary&) = default;
On 2016/11/18 15:03:55, bashi1 wrote:
> Just curious, why do we need this explicit operator=()?

Technically no need.  But I think having "= default" is good because we can
distinguish "making it intentionally copyable" from "making it accidentally
copyable".

I think "= default" was partially designed for this kind of usage.

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/modules/v8/serialization/V8ScriptValueSerializerForModulesTest.cpp
(right):

https://codereview.chromium.org/2509713002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/modules/v8/serialization/V8ScriptValueSerializerForModulesTest.cpp:245:
NonThrowableExceptionState exceptionState;
On 2016/11/18 15:03:55, bashi1 wrote:
> Should we add a comment ("Should rethrow the exception") here?

Unlike TrackExceptionState, NonThrowableExceptionState checks throwing an
exception with NOTREACHED(), so it's okay.  If something throws an exception,
then the test fails.  That's it.

[Yuki, 2016-11-21 12:40:47]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2016-11-21 12:40:48]

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/2509713002/#ps60001
(title: "Addressed review comments.")

[commit-bot: I haz the power, 2016-11-21 12:41:00]

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-11-21 14:13:30]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1479732047938720,
"parent_rev": ["fd1db9b27c0baf33b590cb4aa0319eeb51b60b0a", null], "commit_rev":
["19b0f70ea278b9fb410761b5dbb424692f927c1c", null]}

[commit-bot: I haz the power, 2016-11-21 14:13:52]

Description was changed from

==========
binding: Makes Dictionary throw an exception (constructor).

As part of an effort to make Dictionary handle an exception
correctly, this CL makes Dictionary's ctor take an
ExceptionState and rethrow an exception if any when converting
an ECMAScript value to an IDL dictionary value.

BUG=666661
==========

to

==========
binding: Makes Dictionary throw an exception (constructor).

As part of an effort to make Dictionary handle an exception
correctly, this CL makes Dictionary's ctor take an
ExceptionState and rethrow an exception if any when converting
an ECMAScript value to an IDL dictionary value.

BUG=666661
==========

[commit-bot: I haz the power, 2016-11-21 14:13:53]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-21 14:16:16]

Description was changed from

==========
binding: Makes Dictionary throw an exception (constructor).

As part of an effort to make Dictionary handle an exception
correctly, this CL makes Dictionary's ctor take an
ExceptionState and rethrow an exception if any when converting
an ECMAScript value to an IDL dictionary value.

BUG=666661
==========

to

==========
binding: Makes Dictionary throw an exception (constructor).

As part of an effort to make Dictionary handle an exception
correctly, this CL makes Dictionary's ctor take an
ExceptionState and rethrow an exception if any when converting
an ECMAScript value to an IDL dictionary value.

BUG=666661

Committed: https://crrev.com/1f6b9a3ebd8b058a5f07ae5393bbd7469ad030bf
Cr-Commit-Position: refs/heads/master@{#433529}
==========

[commit-bot: I haz the power, 2016-11-21 14:16:17]

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