Converted IDBVersionChangeEvent.dataLoss to an enumeration: IDBDataLossAmount

Previous implementation was a DOMString. This change also validates values
when constructing IDBVersionChangeEvent instances only allowing the
correct enumeration values.

BUG=358067
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=178517

[cmumford, 2014-07-11 17:47:35]

This is the last IDB enumeration IDL change - the one required to start
implementing crbug.com/358067 since this will be the *only* IDL in Blink that
has both an enumeration attribute initialized by the event constructor.

[cmumford, 2014-07-11 17:49:30]

+nbarth for bindings owner.

[Nils Barth (inactive), 2014-07-11 18:07:47]

On 2014/07/11 17:49:30, cmumford wrote:
> +nbarth for bindings owner.

Looks fine AFAICT, but +haraken is a better reference for Dictionary.
(+bashi, who's implementing proper dictionaries, and +tasak, who's been doing
some refactoring lately)

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

not lgtm

* v8-specific code is leaking out of bindings/
* Module-specific logic is leaking into bindings/

I think this should wait until we have Dictionary+Enum support in the bindings
code generator. bashi@, can you comment on the status of that?

https://codereview.chromium.org/386883008/diff/40001/Source/bindings/modules/...
File Source/bindings/modules/v8/DictionaryHelperForModules.cpp (right):

https://codereview.chromium.org/386883008/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/DictionaryHelperForModules.cpp:43: #include
"public/platform/WebIDBTypes.h"
It'd be nice to drop this dependency...

https://codereview.chromium.org/386883008/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/DictionaryHelperForModules.cpp:77: template <typename
T>
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.

https://codereview.chromium.org/386883008/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBVersionChangeEvent.h (right):

https://codereview.chromium.org/386883008/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBVersionChangeEvent.h:63: static bool
toNativeDataLoss(v8::Handle<v8::Value>, blink::WebIDBDataLoss*);
We shouldn't be passing v8::XXX types in modules/

https://codereview.chromium.org/386883008/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBVersionChangeEvent.idl (right):

https://codereview.chromium.org/386883008/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBVersionChangeEvent.idl:28: enum IDBDataLossAmount {
Add note (similar to below) that this is non-standard

[cmumford, 2014-07-11 20:53:06]

https://codereview.chromium.org/386883008/diff/40001/Source/bindings/modules/...
File Source/bindings/modules/v8/DictionaryHelperForModules.cpp (right):

https://codereview.chromium.org/386883008/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/DictionaryHelperForModules.cpp:43: #include
"public/platform/WebIDBTypes.h"
On 2014/07/11 18:42:48, jsbell wrote:
> It'd be nice to drop this dependency...

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?

https://codereview.chromium.org/386883008/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/DictionaryHelperForModules.cpp:77: template <typename
T>
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:
> 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.

https://codereview.chromium.org/386883008/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBVersionChangeEvent.h (right):

https://codereview.chromium.org/386883008/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBVersionChangeEvent.h:63: static bool
toNativeDataLoss(v8::Handle<v8::Value>, blink::WebIDBDataLoss*);
On 2014/07/11 18:42:48, jsbell wrote:
> We shouldn't be passing v8::XXX types in modules/

OK - I'll convert to String or something suitable. You're right - I don't see
any other V8 stuff in modules which makes sense.

https://codereview.chromium.org/386883008/diff/40001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBVersionChangeEvent.idl (right):

https://codereview.chromium.org/386883008/diff/40001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBVersionChangeEvent.idl:28: enum IDBDataLossAmount {
On 2014/07/11 18:42:48, jsbell wrote:
> Add note (similar to below) that this is non-standard

Done.

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

https://codereview.chromium.org/386883008/diff/40001/Source/bindings/modules/...
File Source/bindings/modules/v8/DictionaryHelperForModules.cpp (right):

https://codereview.chromium.org/386883008/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/DictionaryHelperForModules.cpp:77: template <typename
T>
On 2014/07/11 20:53:05, cmumford wrote:
> 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.

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

[haraken, 2014-07-12 15:48:46]

+tasak for the Dictionary stuff

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

It seems that what this CL wants to do is IDL enum -> C++ enum conversion by
using Dictionary{,Helper} classes, and this CL has nothing to do with (IDL)
dictionary. I'm writing an WIP patch for IDL dictionary support, but I'm afraid
my patch doesn't help your situation..

https://codereview.chromium.org/386883008/diff/40001/Source/bindings/modules/...
File Source/bindings/modules/v8/DictionaryHelperForModules.cpp (right):

https://codereview.chromium.org/386883008/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/DictionaryHelperForModules.cpp:77: template <typename
T>
On 2014/07/11 23:08:30, jsbell wrote:
> On 2014/07/11 20:53:05, cmumford wrote:
> > 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.
> 
> 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

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.

https://codereview.chromium.org/386883008/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/DictionaryHelperForModules.cpp:107: }
This looks exactly the same as DictionaryHelper::convert() in
DictionaryHelperForCore.cpp. Can't we merge them?

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

Am researching requested change to remove enum conversion form DictionaryHelper.

https://codereview.chromium.org/386883008/diff/40001/Source/bindings/modules/...
File Source/bindings/modules/v8/DictionaryHelperForModules.cpp (right):

https://codereview.chromium.org/386883008/diff/40001/Source/bindings/modules/...
Source/bindings/modules/v8/DictionaryHelperForModules.cpp:107: }
On 2014/07/13 04:53:32, bashi1 wrote:
> This looks exactly the same as DictionaryHelper::convert() in
> DictionaryHelperForCore.cpp. Can't we merge them?

Done.

[cmumford, 2014-07-16 20:40:32]

Fixed implementation to push IDB enumeration out of bindings. PTAL.

[jsbell, 2014-07-16 22:48:36]

https://codereview.chromium.org/386883008/diff/80001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBVersionChangeEvent.cpp (right):

https://codereview.chromium.org/386883008/diff/80001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBVersionChangeEvent.cpp:64: ASSERT_NOT_REACHED(); //
Should have been validated by the bindings
Is this actually enforced by the bindings now? (I didn't think we quite had
Dictionary/enum support together - if so, awesome!)

Can you add in a test case that passes an invalid value and ensure the event
constructor throws?

[bashi, 2014-07-17 01:47:58]

https://codereview.chromium.org/386883008/diff/80001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBVersionChangeEvent.cpp (right):

https://codereview.chromium.org/386883008/diff/80001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBVersionChangeEvent.cpp:64: ASSERT_NOT_REACHED(); //
Should have been validated by the bindings
On 2014/07/16 22:48:36, jsbell wrote:
> Is this actually enforced by the bindings now? (I didn't think we quite had
> Dictionary/enum support together - if so, awesome!)
IIUC, the bindings checks whether the value is valid.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> Can you add in a test case that passes an invalid value and ensure the event
> constructor throws?

[cmumford, 2014-07-17 17:14:13]

On 2014/07/17 01:47:58, bashi1 wrote:
>
https://codereview.chromium.org/386883008/diff/80001/Source/modules/indexeddb...
> File Source/modules/indexeddb/IDBVersionChangeEvent.cpp (right):
> 
>
https://codereview.chromium.org/386883008/diff/80001/Source/modules/indexeddb...
> Source/modules/indexeddb/IDBVersionChangeEvent.cpp:64: ASSERT_NOT_REACHED();
//
> Should have been validated by the bindings
> On 2014/07/16 22:48:36, jsbell wrote:
> > Is this actually enforced by the bindings now? (I didn't think we quite had
> > Dictionary/enum support together - if so, awesome!)
> IIUC, the bindings checks whether the value is valid.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > Can you add in a test case that passes an invalid value and ensure the event
> > constructor throws?

Can't because it isn't validated. I'm finishing up another change
(crrev.com/368753002) which *will* validate enum values during construction, and
it does come with that new test.

[cmumford, 2014-07-17 17:14:24]

https://codereview.chromium.org/386883008/diff/80001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBVersionChangeEvent.cpp (right):

https://codereview.chromium.org/386883008/diff/80001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBVersionChangeEvent.cpp:64: ASSERT_NOT_REACHED(); //
Should have been validated by the bindings
On 2014/07/16 22:48:36, jsbell wrote:
> Is this actually enforced by the bindings now? (I didn't think we quite had
> Dictionary/enum support together - if so, awesome!)
> 
> Can you add in a test case that passes an invalid value and ensure the event
> constructor throws?

Not checked - That's my follow-up change (crbug.com/358067). I've removed the
assertion, and will re-add it later.

[jsbell, 2014-07-17 17:50:39]

lgtm

[haraken, 2014-07-17 23:23:13]

LGTM in terms of implementation.

[cmumford, 2014-07-18 18:21:19]

bashi1, tasak: Any other changes you'd like to see in this CL?

[bashi, 2014-07-19 01:27:57]

On 2014/07/18 18:21:19, cmumford wrote:
> bashi1, tasak: Any other changes you'd like to see in this CL?

LGTM

[cmumford, 2014-07-19 14:48:39]

On 2014/07/19 01:27:57, bashi1 wrote:
> On 2014/07/18 18:21:19, cmumford wrote:
> > bashi1, tasak: Any other changes you'd like to see in this CL?
> 
> LGTM

I got an earlier email from tasak saying that he was in agreement with jsbell
and bashi1's comments. This CL seems to have enough approvals so I am going to
take the liberty of submitting it.

[cmumford, 2014-07-19 14:48:46]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2014-07-19 14:49:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cmumford@chromium.org/386883008/120001

[commit-bot: I haz the power, 2014-07-19 15:48:52]

Change committed as 178517