Bug 36830 – IndexedDB: Finish hooking up bindings for IndexedDatabaseRequest

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

IndexedDB: Finish hooking up bindings for IndexedDatabaseRequest

[Jeremy Orlow]

Created attachment 52044 [details]
Patch

[Nate Chapin]

Comment on attachment 52044 [details]
Patch

r+ with a nit.

> +private:
> +    V8CustomIDBCallbacks(v8::Local<v8::Value> onSuccess, v8::Local<v8::Value> onError, Frame* frame)
> +        : m_onSuccessNull(!onSuccess->IsObject())
> +        , m_onSuccess(v8::Persistent<v8::Object>::New(onSuccess->ToObject()))
> +        , m_onErrorNull(!onError->IsObject())
> +        , m_onError(v8::Persistent<v8::Object>::New(onError->ToObject()))
> +        , m_frame(frame)
> +    {
> +    }
> +
> +    // FIXME: Should these be v8::Functions?  For some reason, VoidCallback (which this copied) uses v8::Objects.
> +    bool m_onSuccessNull;
> +    v8::Persistent<v8::Object> m_onSuccess;
> +    bool m_onErrorNull;

Why do we need separate bools here?  It seems like unnecessary duplication of information to me.

> +    v8::Persistent<v8::Object> m_onError;
> +    RefPtr<Frame> m_frame;
> +};
> +
> +}
> +
> +#endif
> +
> +#endif // V8CustomIDBCallbacks_h

[Jeremy Orlow]

(In reply to comment #2)
> (From update of attachment 52044 [details])
> r+ with a nit.
> 
> > +private:
> > +    V8CustomIDBCallbacks(v8::Local<v8::Value> onSuccess, v8::Local<v8::Value> onError, Frame* frame)
> > +        : m_onSuccessNull(!onSuccess->IsObject())
> > +        , m_onSuccess(v8::Persistent<v8::Object>::New(onSuccess->ToObject()))
> > +        , m_onErrorNull(!onError->IsObject())
> > +        , m_onError(v8::Persistent<v8::Object>::New(onError->ToObject()))
> > +        , m_frame(frame)
> > +    {
> > +    }
> > +
> > +    // FIXME: Should these be v8::Functions?  For some reason, VoidCallback (which this copied) uses v8::Objects.
> > +    bool m_onSuccessNull;
> > +    v8::Persistent<v8::Object> m_onSuccess;
> > +    bool m_onErrorNull;
> 
> Why do we need separate bools here?  It seems like unnecessary duplication of
> information to me.

How can I tell when a v8 object is valid or not?  I'm sure there is some way.

Going to commit this and continue working, but I am interested in making this cleaner.

+ mads since he might know.

[Jeremy Orlow]

Committed r56834: <http://trac.webkit.org/changeset/56834>

[Mads Ager]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 52044 [details] [details])
> > r+ with a nit.
> > 
> > > +private:
> > > +    V8CustomIDBCallbacks(v8::Local<v8::Value> onSuccess, v8::Local<v8::Value> onError, Frame* frame)
> > > +        : m_onSuccessNull(!onSuccess->IsObject())
> > > +        , m_onSuccess(v8::Persistent<v8::Object>::New(onSuccess->ToObject()))
> > > +        , m_onErrorNull(!onError->IsObject())
> > > +        , m_onError(v8::Persistent<v8::Object>::New(onError->ToObject()))
> > > +        , m_frame(frame)
> > > +    {
> > > +    }
> > > +
> > > +    // FIXME: Should these be v8::Functions?  For some reason, VoidCallback (which this copied) uses v8::Objects.
> > > +    bool m_onSuccessNull;
> > > +    v8::Persistent<v8::Object> m_onSuccess;
> > > +    bool m_onErrorNull;
> > 
> > Why do we need separate bools here?  It seems like unnecessary duplication of
> > information to me.
> 
> How can I tell when a v8 object is valid or not?  I'm sure there is some way.
> 
> Going to commit this and continue working, but I am interested in making this
> cleaner.
> 
> + mads since he might know.

Sorry for the late reply, was on vacation.

You could leave the two persistent handles m_onSuccess and m_onError empty if the arguments are not objects. Disposing empty handles is valid and your check would be m_onSuccess.IsEmpty() instead of testing the boolean flag. Also, if the arguments are objects, you do not need to call ToObject() you can use handle cast instead (but it doesn't really matter since the ToObject call will essentially do that in this case).