Implement IDBVersionChangeEvent ctor w/ IDBVersionChangeEventInit dict

Implement IDBVersionChangeEvent ctor w/ IDBVersionChangeEventInit dict

A rarely used part of the IndexedDB spec was overlooked - like most
other events, script can call the constructor with a dict to populate
the fields.

This required some finessing: the fields were stored as 'IDBAny'
but our automagical code gen wiring for EventInit prefers ScriptValue
or SerializedScriptValue. I went with the latter, and as an aside
eliminate a call to v8::Isolate::GetCurrent() by SSV - serializing
simple values such as null or numbers can be done with no isolate.

BUG=345881
R=dgrogan,haraken

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

[jsbell, 2014-02-24 21:29:59]

dgrogan@, haraken@ - PTAL?

[dgrogan, 2014-02-24 21:42:13]

IDB behavior LGTM

[haraken, 2014-02-25 01:14:38]

LGTM with comments.

https://codereview.chromium.org/177633006/diff/1/LayoutTests/storage/indexedd...
File LayoutTests/storage/indexeddb/event-init.html (right):

https://codereview.chromium.org/177633006/diff/1/LayoutTests/storage/indexedd...
LayoutTests/storage/indexeddb/event-init.html:12: finishJSTest();

Is this necessary?

https://codereview.chromium.org/177633006/diff/1/Source/bindings/v8/Serialize...
File Source/bindings/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/177633006/diff/1/Source/bindings/v8/Serialize...
Source/bindings/v8/SerializedScriptValue.cpp:2299: v8::Isolate* isolate = 0;

I'm not really happy about this.

When we have an Isolate*, we want to assume that the Isolate* is non-NULL.
(We've eliminated almost all NULL Isolate* from the code base.)

If Writer starts using the maybe-NULL isolate in the future, it will cause
crashes since other code expects a non-NULL isolate.

Shall we pass an Isolate to nullValue() or keep v8::Isolate::GetCurrent()? I
agree that we should remove v8::Isolate::GetCurrent() but I think that
v8::Isolate::GetCurrent() is better than using 0.

https://codereview.chromium.org/177633006/diff/1/Source/bindings/v8/Serialize...
Source/bindings/v8/SerializedScriptValue.cpp:2308: v8::Isolate* isolate = 0;

Ditto.

https://codereview.chromium.org/177633006/diff/1/Source/modules/indexeddb/IDB...
File Source/modules/indexeddb/IDBOpenDBRequest.cpp (right):

https://codereview.chromium.org/177633006/diff/1/Source/modules/indexeddb/IDB...
Source/modules/indexeddb/IDBOpenDBRequest.cpp:72: RefPtr<SerializedScriptValue>
newVersionSSV = (m_version == IDBDatabaseMetadata::DefaultIntVersion) ?
SerializedScriptValue::nullValue() :
SerializedScriptValue::numberValue(m_version);

Shall we add a test for nullValue as well?

[jsbell, 2014-02-25 17:42:09]

https://codereview.chromium.org/177633006/diff/1/Source/bindings/v8/Serialize...
File Source/bindings/v8/SerializedScriptValue.cpp (right):

https://codereview.chromium.org/177633006/diff/1/Source/bindings/v8/Serialize...
Source/bindings/v8/SerializedScriptValue.cpp:2299: v8::Isolate* isolate = 0;
On 2014/02/25 01:14:38, haraken wrote:
> 
> I'm not really happy about this.
> 
> When we have an Isolate*, we want to assume that the Isolate* is non-NULL.
> (We've eliminated almost all NULL Isolate* from the code base.)
>
> If Writer starts using the maybe-NULL isolate in the future, it will cause
> crashes since other code expects a non-NULL isolate.

Okay. Since this is just packing bytes into a buffer using the writer, there's
no need for an isolate. It seemed like with tests for these cases (static
xxxValue() constructors) tests would catch any mistakes - it should have unit
tests at the very least, though. That would complicate further reworking of SSV,
though, so I understand if we want to avoid that.
 
> Shall we pass an Isolate to nullValue() or keep v8::Isolate::GetCurrent()? I
> agree that we should remove v8::Isolate::GetCurrent() but I think that
> v8::Isolate::GetCurrent() is better than using 0.

The sites that construct a new IDBVersionChangeEvent do so without v8 on the
stack. In the current (IDBAny) approach, creating the new v8 value is deferred
until script accesses the property. In this patch, the SSV is used to defer the
creation instead.

I'll ponder.

[haraken, 2014-02-26 01:42:06]

On 2014/02/25 17:42:09, jsbell wrote:
>
https://codereview.chromium.org/177633006/diff/1/Source/bindings/v8/Serialize...
> File Source/bindings/v8/SerializedScriptValue.cpp (right):
> 
>
https://codereview.chromium.org/177633006/diff/1/Source/bindings/v8/Serialize...
> Source/bindings/v8/SerializedScriptValue.cpp:2299: v8::Isolate* isolate = 0;
> On 2014/02/25 01:14:38, haraken wrote:
> > 
> > I'm not really happy about this.
> > 
> > When we have an Isolate*, we want to assume that the Isolate* is non-NULL.
> > (We've eliminated almost all NULL Isolate* from the code base.)
> >
> > If Writer starts using the maybe-NULL isolate in the future, it will cause
> > crashes since other code expects a non-NULL isolate.
> 
> Okay. Since this is just packing bytes into a buffer using the writer, there's
> no need for an isolate. It seemed like with tests for these cases (static
> xxxValue() constructors) tests would catch any mistakes - it should have unit
> tests at the very least, though. That would complicate further reworking of
SSV,
> though, so I understand if we want to avoid that.
>  
> > Shall we pass an Isolate to nullValue() or keep v8::Isolate::GetCurrent()? I
> > agree that we should remove v8::Isolate::GetCurrent() but I think that
> > v8::Isolate::GetCurrent() is better than using 0.
> 
> The sites that construct a new IDBVersionChangeEvent do so without v8 on the
> stack. In the current (IDBAny) approach, creating the new v8 value is deferred
> until script accesses the property. In this patch, the SSV is used to defer
the
> creation instead.
> 
> I'll ponder.

It seems that Writer shouldn't hold m_isolate (because Writer is used when V8 is
not on the stack). Then maybe we can drop Writer::m_isolate and make Writer get
the Isolate when he really needs it.

[jsbell, 2014-02-26 18:06:17]

On 2014/02/26 01:42:06, haraken wrote:
> It seems that Writer shouldn't hold m_isolate (because Writer is used when V8
is
> not on the stack). Then maybe we can drop Writer::m_isolate and make Writer
get
> the Isolate when he really needs it.

Ooooh, good point. It looks like Writer never actually needs the isolate; the
Serializer holds it and passes it in to all but one method along side the
Writer.

[haraken, 2014-02-26 22:46:46]

On 2014/02/26 18:06:17, jsbell wrote:
> On 2014/02/26 01:42:06, haraken wrote:
> > It seems that Writer shouldn't hold m_isolate (because Writer is used when
V8
> is
> > not on the stack). Then maybe we can drop Writer::m_isolate and make Writer
> get
> > the Isolate when he really needs it.
> 
> Ooooh, good point. It looks like Writer never actually needs the isolate; the
> Serializer holds it and passes it in to all but one method along side the
> Writer.

With that fix, LGTM :)

[jsbell, 2014-02-27 20:51:08]

On 2014/02/25 01:14:38, haraken wrote:
> LayoutTests/storage/indexeddb/event-init.html:12: finishJSTest();
> 
> Is this necessary?

Apparently it's been a long time since I wrote a non-async test. Removed.
 
> Shall we add a test for nullValue as well?

Yes. And chasing this through, I ended up with a quite different patch:

* I realized we don't plumb through legacy strings for oldVersion anyway, so
that can just be `unsigned long long` instead of an SSV.

* For newVersion, which needs to be `unsigned long long?` - if it's
SerializedScriptValue then we can't constrain/convert what comes from script
because the XXXEventInit dict isn't actually defined in Blink IDL, but derived
from the XXXEvent interface. I changed newVersion from SSV to `unsigned long
long?`; it mostly worked, except that explicit null wasn't handled. Chasing that
down led me to the Dictionary's convert mechanism and adding support for
nullable integer types.

So... this now introduces Nullable<T>, which is a pretty big step. It's not
plumbed through everywhere; the accessor style `int foo(bool& isNull) const` but
we could switch to `Nullable<int> foo() const` later on. Dictionary's support is
also limited to integral types, but it's easily extended.

Thoughts?

[jsbell, 2014-02-27 20:59:05]

Just some design notes.

https://codereview.chromium.org/177633006/diff/20001/Source/bindings/v8/Nulla...
File Source/bindings/v8/Nullable.h (right):

https://codereview.chromium.org/177633006/diff/20001/Source/bindings/v8/Nulla...
Source/bindings/v8/Nullable.h:10: namespace WebCore {
Could be in wtf, but I started conservatively.

https://codereview.chromium.org/177633006/diff/20001/Source/bindings/v8/Nulla...
Source/bindings/v8/Nullable.h:15: Nullable()
I also considered Nullable<type>::null() as a static constructor, and/or
allowing Nullable<type>(nullptr) via wtf/NullPtr.h. I have no strong
preferences.

Unfortunately, callers can't write the concise:

Nullable<t> value(c ? a : nullptr);

... since the type of the ternary operator's branches are not consistent.

https://codereview.chromium.org/177633006/diff/20001/Source/bindings/v8/Nulla...
Source/bindings/v8/Nullable.h:34: T get() const { ASSERT(!m_isNull); return
m_value; }
I did NOT expose an `operator T()` conversion, favoring an explicit get().

https://codereview.chromium.org/177633006/diff/20001/Source/bindings/v8/Nulla...
Source/bindings/v8/Nullable.h:37: operator bool() const { return !m_isNull &&
m_value; }
This was required by the Dictionary. I'll admit I didn't dig too deeply into
why.

[haraken, 2014-02-27 23:09:15]

LGTM

[haraken, 2014-02-27 23:11:35]

Probably Nullable is also helpful to simplify auto-generated code. We're
currently using an isNull flag to know if a given value is null or not.

[jsbell, 2014-02-28 17:51:23]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-02-28 17:51:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/177633006/20001

[commit-bot: I haz the power, 2014-02-28 17:51:43]

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

[commit-bot: I haz the power, 2014-02-28 17:51:43]

Failed to apply patch for Source/modules/indexeddb/IDBVersionChangeEvent.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/indexeddb/IDBVersionChangeEvent.cpp
  Hunk #1 FAILED at 31.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/modules/indexeddb/IDBVersionChangeEvent.cpp.rej

Patch:       Source/modules/indexeddb/IDBVersionChangeEvent.cpp
Index: Source/modules/indexeddb/IDBVersionChangeEvent.cpp
diff --git a/Source/modules/indexeddb/IDBVersionChangeEvent.cpp
b/Source/modules/indexeddb/IDBVersionChangeEvent.cpp
index
13a89c095d5f474a87cafbe177bcc7d692b6a36c..710e8fe22b0e96cb831622639e1874139445c923
100644
--- a/Source/modules/indexeddb/IDBVersionChangeEvent.cpp
+++ b/Source/modules/indexeddb/IDBVersionChangeEvent.cpp
@@ -31,12 +31,19 @@
 
 namespace WebCore {
 
-PassRefPtr<IDBVersionChangeEvent>
IDBVersionChangeEvent::create(PassRefPtr<IDBAny> oldVersion, PassRefPtr<IDBAny>
newVersion, const AtomicString& eventType, blink::WebIDBDataLoss dataLoss, const
String& dataLossMessage)
+
+IDBVersionChangeEventInit::IDBVersionChangeEventInit()
+    : oldVersion(0)
 {
-    return adoptRef(new IDBVersionChangeEvent(oldVersion, newVersion,
eventType, dataLoss, dataLossMessage));
 }
 
-IDBVersionChangeEvent::IDBVersionChangeEvent(PassRefPtr<IDBAny> oldVersion,
PassRefPtr<IDBAny> newVersion, const AtomicString& eventType,
blink::WebIDBDataLoss dataLoss, const String& dataLossMessage)
+IDBVersionChangeEvent::IDBVersionChangeEvent()
+    : m_dataLoss(blink::WebIDBDataLossNone)
+{
+    ScriptWrappable::init(this);
+}
+
+IDBVersionChangeEvent::IDBVersionChangeEvent(const AtomicString& eventType,
unsigned long long oldVersion, const Nullable<unsigned long long>& newVersion,
blink::WebIDBDataLoss dataLoss, const String& dataLossMessage)
     : Event(eventType, false /*canBubble*/, false /*cancelable*/)
     , m_oldVersion(oldVersion)
     , m_newVersion(newVersion)
@@ -46,20 +53,19 @@
IDBVersionChangeEvent::IDBVersionChangeEvent(PassRefPtr<IDBAny> oldVersion, Pass
     ScriptWrappable::init(this);
 }
 
-IDBVersionChangeEvent::~IDBVersionChangeEvent()
-{
-}
-
-ScriptValue IDBVersionChangeEvent::oldVersion(ExecutionContext* context) const
+IDBVersionChangeEvent::IDBVersionChangeEvent(const AtomicString& eventType,
const IDBVersionChangeEventInit& initializer)
+    : Event(eventType, false /*canBubble*/, false /*cancelable*/)
+    , m_oldVersion(initializer.oldVersion)
+    , m_newVersion(initializer.newVersion)
+    , m_dataLoss(blink::WebIDBDataLossNone)
 {
-    DOMRequestState requestState(context);
-    return idbAnyToScriptValue(&requestState, m_oldVersion);
+    ScriptWrappable::init(this);
 }
 
-ScriptValue IDBVersionChangeEvent::newVersion(ExecutionContext* context) const
+unsigned long long IDBVersionChangeEvent::newVersion(bool& isNull) const
 {
-    DOMRequestState requestState(context);
-    return idbAnyToScriptValue(&requestState, m_newVersion);
+    isNull = m_newVersion.isNull();
+    return isNull ? 0 : m_newVersion.get();
 }
 
 const AtomicString& IDBVersionChangeEvent::dataLoss() const

[jsbell, 2014-02-28 18:50:06]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-02-28 18:50:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jsbell@chromium.org/177633006/40001

[commit-bot: I haz the power, 2014-02-28 20:18:04]

Change committed as 168162