Converted IDBVersionChangeEvent.dataLoss to an enumeration: IDBDataLossAmount

Source/bindings/modules/v8/DictionaryHelperForModules.cpp

Index: Source/bindings/modules/v8/DictionaryHelperForModules.cpp
diff --git a/Source/bindings/modules/v8/DictionaryHelperForModules.cpp b/Source/bindings/modules/v8/DictionaryHelperForModules.cpp
index 931123fd3239cd204d8ec99c45d1053cc97bd4d9..930c3b5f45ba81213e2ada80e9c887c1bea1d3ca 100644
--- a/Source/bindings/modules/v8/DictionaryHelperForModules.cpp
+++ b/Source/bindings/modules/v8/DictionaryHelperForModules.cpp
@@ -36,9 +36,11 @@
 #include "bindings/modules/v8/V8SpeechRecognitionResult.h"
 #include "bindings/modules/v8/V8SpeechRecognitionResultList.h"
 #include "modules/gamepad/Gamepad.h"
+#include "modules/indexeddb/IDBVersionChangeEvent.h"
 #include "modules/mediastream/MediaStream.h"
 #include "modules/speech/SpeechRecognitionResult.h"
 #include "modules/speech/SpeechRecognitionResultList.h"
+#include "public/platform/WebIDBTypes.h"

[jsbell, 2014/07/11 18:42:48]

It'd be nice to drop this dependency...

[cmumford, 2014/07/11 20:53:05]

Yes, this is obviously so that we can get at the WebIDBDataLoss enum definition.
I was trying to avoid creating a second enum value, and functions to convert
to/from public/private enums'. I don't know - this may be a lane violation?

 
 namespace WebCore {
 
@@ -72,6 +74,38 @@ struct DictionaryHelperTraits<Headers> {
     typedef V8Headers type;
 };
 
+template <typename T>

[jsbell, 2014/07/11 18:42:48]

This is basically doing two things:

* Dig the IDL enum (IDBDataLossAmount) string value out of the Dictionary
* Convert the string enum value into the blink::WebIDBDataLoss enum

IMHO, the first part belongs here but the second part doesn't - that has nothing
to do with bindings, and should be constrained entirely to the indexeddb
implementation.

I don't know the state of Dictionary + Enum in our bindings code. If that's not
automagical yet, it would make sense to hold off on this.

[cmumford, 2014/07/11 20:53:05]

Well the string conversion is in IDBVersionChangeEvent::toNativeDataLoss(). I
don't think we currently map enumeration values to actual enum's on the native
side - this may be the first implementation. I requested tasak@ to get some
guidance on this.

On 2014/07/11 18:42:48, jsbell wrote:

[jsbell, 2014/07/11 23:08:30]

Right... I'm assuming that the DictionaryHelper.convert should yield a String
for IDL enum types. Any mapping from String -> C++ enum should be done in the
module itself - that's not the job of bindings

[bashi, 2014/07/13 04:53:32]

I agree with jsbell here. Some classes are doing that (e.g.
modules/serviceworkers/Request.cpp). This way we don't need to add special cases
for each enums (WebIDBDataLoss in this CL) and can remove dependencies of C++
enums (public/platform/WebIDBTypes.h).

I guess you are trying to avoid redundant enum mappings. But if we want to do
IDL -> C++ enum conversion automatically, we should add such feature in the code
generator and we shouldn't handle each case separately.

+struct IntegralTypeTraits {
+};
+
+template <>
+struct IntegralTypeTraits<blink::WebIDBDataLoss> {
+    static inline blink::WebIDBDataLoss toIntegral(v8::Handle<v8::Value> value, IntegerConversionConfiguration configuration, ExceptionState& exceptionState)
+    {
+        blink::WebIDBDataLoss dataLoss = blink::WebIDBDataLossNone;
+        if (!IDBVersionChangeEvent::toNativeDataLoss(value, &dataLoss))
+            exceptionState.throwTypeError("Invalid IDBDataLossAmount value");
+        return dataLoss;
+        }
+    static const String typeName() { return "IDBDataLossAmount"; }
+};
+
+template<typename T>
+bool DictionaryHelper::convert(const Dictionary& dictionary, Dictionary::ConversionContext& context, const String& key, T& value)
+{
+    Dictionary::ConversionContextScope scope(context);
+
+    v8::Local<v8::Value> v8Value;
+    if (!dictionary.get(key, v8Value))
+        return true;
+
+    value = IntegralTypeTraits<T>::toIntegral(v8Value, NormalConversion, context.exceptionState());
+    if (context.exceptionState().throwIfNeeded())
+        return false;
+
+    return true;
+}

[bashi, 2014/07/13 04:53:32]

This looks exactly the same as DictionaryHelper::convert() in
DictionaryHelperForCore.cpp. Can't we merge them?

[cmumford, 2014/07/14 22:09:37]

Done.

+
 template bool DictionaryHelper::get(const Dictionary&, const String& key, Member<MIDIPort>& value);
 template bool DictionaryHelper::get(const Dictionary&, const String& key, Member<SpeechRecognitionResult>& value);
 template bool DictionaryHelper::get(const Dictionary&, const String& key, Member<SpeechRecognitionResultList>& value);
@@ -85,5 +119,6 @@ template bool DictionaryHelper::convert(const Dictionary&, Dictionary::Conversio
 template bool DictionaryHelper::convert(const Dictionary&, Dictionary::ConversionContext&, const String& key, Member<Gamepad>& value);
 template bool DictionaryHelper::convert(const Dictionary&, Dictionary::ConversionContext&, const String& key, Member<MediaStream>& value);
 template bool DictionaryHelper::convert(const Dictionary&, Dictionary::ConversionContext&, const String& key, RefPtrWillBeMember<Headers>& value);
+template bool DictionaryHelper::convert(const Dictionary&, Dictionary::ConversionContext&, const String& key, blink::WebIDBDataLoss& value);
 
 } // namespace WebCore