IDL: Add PropertyBag class

IDL: Add PropertyBag class

This CL adds PropertyBag class, which is similar to Dictionary{,Helper},
but has some differences.

Before this patch, we used DictionaryHelper to get native values from
a V8 object in IDL dictionary's toNative(). However,
DictionaryHelper::get() returns true when a dictionary member is
specified as undefined. Since we want to treat undefined members as
'not specified', we can't use DictionaryHelper to retrive native values.
PropertyBag's get() method returns false when a member is undefined
or null.

BUG=321462

[bashi, 2014-08-27 12:01:34]

Hi Haraken-san,

This is a follow-up work-in-progress CL to
https://codereview.chromium.org/429853002/

What do you think about this approach?

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
File Source/bindings/core/v8/PropertyBag.h (right):

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
Source/bindings/core/v8/PropertyBag.h:27: struct PropertyBagTraits<int> {
We would need to add specialized traits for all primitive types and container
types eventually, but we should avoid adding traits for (IDL) interface impl
types by hand (as DictionaryHelper does). I'll try that if this approach looks
reasonable so far.

[bashi, 2014-08-27 12:01:52]

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

[haraken, 2014-08-27 16:07:44]

It's a shame that we have to duplicate a lot of code between DictionaryHelper
and PropertyBag, but I'm fine if you're planning to deprecate DictionaryHelper
in a reasonable timeline.

https://codereview.chromium.org/508073002/diff/1/LayoutTests/fast/dom/idl-dic...
File LayoutTests/fast/dom/idl-dictionary-unittest.html (right):

https://codereview.chromium.org/508073002/diff/1/LayoutTests/fast/dom/idl-dic...
LayoutTests/fast/dom/idl-dictionary-unittest.html:43: booleanOrNullMember:
null});

Shall we add a test case to pass undefined to booleanOrNullMember?

Actually we might want to make the test cases more complete. For example:

dictionary InternalDictionary {
  boolean booleanMember;  // test for normal type
  boolean booleanMemberWithDefault = true;  // test for normal type with a
default value
  boolean? booleanOrNullMember;  // test for nullable type
  boolean? booleanOrNullMemberWithDefault = true;  // test for nullable type
with a default value
  long longMember;  // test for other types
  double doubleMember;  // test for other types
  DOMString stringMember;  // test for other types
  sequence<DOMString> stringSequenceMember;  // test for sequence type
  sequence<DOMString>? stringSequenceOrNullMember;  // test for nullable
sequence type
};

Then you can add test cases that pass null, undefined, a normal value or missing
value to each member. You can make the change in a follow-up CL.

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
File Source/bindings/core/v8/PropertyBag.h (right):

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
Source/bindings/core/v8/PropertyBag.h:20: TOSTRING_DEFAULT(V8StringResource<>,
stringValue, v8Value, false);

This can throw an exception, but is it OK? The same comment for other conversion
methods like ToInt32, ToBoolean etc.

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
Source/bindings/core/v8/PropertyBag.h:74: class PropertyBag {

Add a comment about the relationship between Dictionary(Helper) and PropertyBag.

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
Source/bindings/core/v8/PropertyBag.h:77: PropertyBag(const
v8::Handle<v8::Object>& object, v8::Isolate* isolate)

Put the Isolate parameter first. (Putting the Isolate parameter last is an old
rule, although there remain a lot of methods that have the Isolate parameter
last.)

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
Source/bindings/core/v8/PropertyBag.h:85: {

Add an m_object.IsEmpty() check or ASSERT(!m_object.IsEmpty()).

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
Source/bindings/core/v8/PropertyBag.h:90: v8::Local<v8::Value> v8Value =
m_object->Get(v8Key);

Do we need to issue both Has and Get? Can't we know that the key doesn't exist
just by checking the return value of Get?

https://codereview.chromium.org/508073002/diff/1/Source/bindings/templates/di...
File Source/bindings/templates/dictionary_v8.cpp (right):

https://codereview.chromium.org/508073002/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.cpp:19: if (!v8Value->IsObject())

You'll need to check v8Value.IsEmpty() before checking !v8Value->IsObject().
Otherwise, v8Value->IsObject() crashes if the v8Value is empty.

  if (v8Value.IsEmpty() || !v8Value->IsObject())

[bashi, 2014-08-29 05:21:17]

PTAL?

Yeah, it's a shame to have duplication, but it's somewhat difficult to refactor
DictionaryHelper now. We should be able to remove DictionaryHelper when we
implement enough support for IDL dictionary.

I changed the structure of PropertyBag a bit. Moved type specific getter logic
from PropertyBagTraits::get() to PropertyBag::getInternal(). The IDL compiler
generates PropertyBagTraits<T>::type for each IDL interface to make
getInternal() workable for IDL interface types. I don't think this addition
impacts on binary size; I expect that C++ compilers instantiate getInternal() if
only needed.

https://codereview.chromium.org/508073002/diff/1/LayoutTests/fast/dom/idl-dic...
File LayoutTests/fast/dom/idl-dictionary-unittest.html (right):

https://codereview.chromium.org/508073002/diff/1/LayoutTests/fast/dom/idl-dic...
LayoutTests/fast/dom/idl-dictionary-unittest.html:43: booleanOrNullMember:
null});
On 2014/08/27 16:07:43, haraken wrote:
> 
> Shall we add a test case to pass undefined to booleanOrNullMember?
> 
> Actually we might want to make the test cases more complete. For example:
> 
> dictionary InternalDictionary {
>   boolean booleanMember;  // test for normal type
>   boolean booleanMemberWithDefault = true;  // test for normal type with a
> default value
>   boolean? booleanOrNullMember;  // test for nullable type
>   boolean? booleanOrNullMemberWithDefault = true;  // test for nullable type
> with a default value
>   long longMember;  // test for other types
>   double doubleMember;  // test for other types
>   DOMString stringMember;  // test for other types
>   sequence<DOMString> stringSequenceMember;  // test for sequence type
>   sequence<DOMString>? stringSequenceOrNullMember;  // test for nullable
> sequence type
> };
> 
> Then you can add test cases that pass null, undefined, a normal value or
missing
> value to each member. You can make the change in a follow-up CL.

Done.

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
File Source/bindings/core/v8/PropertyBag.h (right):

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
Source/bindings/core/v8/PropertyBag.h:20: TOSTRING_DEFAULT(V8StringResource<>,
stringValue, v8Value, false);
On 2014/08/27 16:07:43, haraken wrote:
> 
> This can throw an exception, but is it OK? The same comment for other
conversion
> methods like ToInt32, ToBoolean etc.

Dictionary(Helper) doesn't check exceptions so I thought it is ok. But I'm not
familiar with how exceptions should be handled in general. Is there any
guideline for handling exceptions?

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
Source/bindings/core/v8/PropertyBag.h:74: class PropertyBag {
On 2014/08/27 16:07:43, haraken wrote:
> 
> Add a comment about the relationship between Dictionary(Helper) and
PropertyBag.

Done.

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
Source/bindings/core/v8/PropertyBag.h:77: PropertyBag(const
v8::Handle<v8::Object>& object, v8::Isolate* isolate)
On 2014/08/27 16:07:43, haraken wrote:
> 
> Put the Isolate parameter first. (Putting the Isolate parameter last is an old
> rule, although there remain a lot of methods that have the Isolate parameter
> last.)

Done.

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
Source/bindings/core/v8/PropertyBag.h:85: {
On 2014/08/27 16:07:43, haraken wrote:
> 
> Add an m_object.IsEmpty() check or ASSERT(!m_object.IsEmpty()).

Done.

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
Source/bindings/core/v8/PropertyBag.h:90: v8::Local<v8::Value> v8Value =
m_object->Get(v8Key);
On 2014/08/27 16:07:43, haraken wrote:
> 
> Do we need to issue both Has and Get? Can't we know that the key doesn't exist
> just by checking the return value of Get?

Done.

https://codereview.chromium.org/508073002/diff/1/Source/bindings/templates/di...
File Source/bindings/templates/dictionary_v8.cpp (right):

https://codereview.chromium.org/508073002/diff/1/Source/bindings/templates/di...
Source/bindings/templates/dictionary_v8.cpp:19: if (!v8Value->IsObject())
On 2014/08/27 16:07:43, haraken wrote:
> 
> You'll need to check v8Value.IsEmpty() before checking !v8Value->IsObject().
> Otherwise, v8Value->IsObject() crashes if the v8Value is empty.
> 
>   if (v8Value.IsEmpty() || !v8Value->IsObject())

Done.

https://codereview.chromium.org/508073002/diff/20001/Source/core/testing/Inte...
File Source/core/testing/InternalDictionary.idl (right):

https://codereview.chromium.org/508073002/diff/20001/Source/core/testing/Inte...
Source/core/testing/InternalDictionary.idl:18: Element elementMember;
Added interface type as well. We can add this in a follow-up CL, but I added it
in this CL to show how PropertyBag::getInternal() works for interface types.

[haraken, 2014-08-29 05:44:05]

LGTM

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
File Source/bindings/core/v8/PropertyBag.h (right):

https://codereview.chromium.org/508073002/diff/1/Source/bindings/core/v8/Prop...
Source/bindings/core/v8/PropertyBag.h:20: TOSTRING_DEFAULT(V8StringResource<>,
stringValue, v8Value, false);
On 2014/08/29 05:21:17, bashi1 wrote:
> On 2014/08/27 16:07:43, haraken wrote:
> > 
> > This can throw an exception, but is it OK? The same comment for other
> conversion
> > methods like ToInt32, ToBoolean etc.
> 
> Dictionary(Helper) doesn't check exceptions so I thought it is ok. But I'm not
> familiar with how exceptions should be handled in general. Is there any
> guideline for handling exceptions?

I think it's OK to throw an exception here. It's a responsibility of the caller
side to handle the exception.

The current exception system is very complicated and broken in some aspects.
Only Jens and yhirano know how it's working :) This is an area where we should
improve.

https://codereview.chromium.org/508073002/diff/20001/Source/bindings/core/v8/...
File Source/bindings/core/v8/PropertyBag.h (right):

https://codereview.chromium.org/508073002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/PropertyBag.h:37: if (v8Value.IsEmpty() ||
isUndefinedOrNull(v8Value)) {

Just to confirm: I understand you want to return false when v8Value is
undefined, but you also want to return false when v8Value is null, right?

https://codereview.chromium.org/508073002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/PropertyBag.h:44: bool
getInternal(v8::Handle<v8::Value>& v8Value, String& value);

Drop |v8Value| and |value|.