Bug 36546 – A few more steps towards IndexedDB

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

A few more steps towards IndexedDB.  Work towards hooking up IndexedDatabaseProxy to something.

[Jeremy Orlow]

Created attachment 51529 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 51529 [details]
Patch

> +++ b/WebCore/storage/IDBCallbacks.h
...
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +#ifndef IDBCallbacks_h

nit: insert a new line after the comment block for consistency with other
source files.


> +++ b/WebCore/storage/IDBDatabase.h
...
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +#ifndef IDBDatabase_h

^^^ ditto


> +++ b/WebCore/storage/IndexedDatabase.h
...
> +    virtual void open(const String& name, const String& description, bool modifyDatabase, ExceptionCode&, PassRefPtr<IDBCallbacks<IDBDatabase> >) = 0;

I wonder if a typedef wouldn't be handy for IDBCallbacks<IDBDatabase>... maybe:

typedef IDBCallbacks<IDBDatabase> IDBDatabaseCallbacks ??


> +++ b/WebCore/storage/IndexedDatabaseImpl.cpp
...
> +void IndexedDatabaseImpl::open(const String& name, const String& description, bool modifyDatabase, ExceptionCode&, PassRefPtr<IDBCallbacks<IDBDatabase> >)
>  {
>      // FIXME: Write.
> +    ASSERT(0);

ASSERT_NOT_REACHED()


> +++ b/WebKit/chromium/public/WebIDBCallbacks.h
...
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +#ifndef WebIDBCallbacks_h

same nit about inserting a new line


> +template <typename ResultType>
> +class WebIDBCallbacks {
> +public:
> +    virtual ~WebIDBCallbacks() { }
> +
> +    virtual void onSuccess(ResultType*) = 0;
> +    virtual void onError(const WebIDBDatabaseError&) = 0;
> +};

Does ResultType need to be mutable?  Does it need to be passed as
a pointer?  Could it be passed as 'const ResultType&'?


> +++ b/WebKit/chromium/public/WebIDBDatabase.h
> @@ -0,0 +1,45 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +#ifndef WebIDBDatabase_h
> +#define WebIDBDatabase_h
> +
> +#include "WebCommon.h"
> +
> +namespace WebKit {
> +
> +// See comment in WebIndexedDatabase for a high level overview these classes.
> +class WebIDBDatabase {
> +public:
> +    virtual ~WebIDBDatabase() { }
> +
> +    // FIXME: Implement.
> +};
> +
> +} // namespace WebKit
> +
> +#endif // WebIDBDatabase_h
> diff --git a/WebKit/chromium/public/WebIDBDatabaseError.h b/WebKit/chromium/public/WebIDBDatabaseError.h
> new file mode 100644
> index 0000000..10763a0
> --- /dev/null
> +++ b/WebKit/chromium/public/WebIDBDatabaseError.h
> @@ -0,0 +1,69 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +#ifndef WebIDBDatabaseError_h
> +#define WebIDBDatabaseError_h
> +
> +#include "WebCommon.h"
> +#include "WebPrivatePtr.h"
> +#include "WebString.h"
> +
> +namespace WebCore { class IDBDatabaseError; }
> +
> +namespace WebKit {
> +
> +// See comment in WebIndexedDatabase for a high level overview these classes.
> +class WebIDBDatabaseError {

Why not call this WebIDBError?  Same goes for IDBDatabaseError.  Doesn't
IDB imply Database?


> +public:
> +    ~WebIDBDatabaseError();
> +
> +    WebIDBDatabaseError(unsigned short code, const WebString& message);
...
> +    void assign(const WebIDBDatabaseError&);
> +
> +    unsigned short code() const;
> +    WebString message() const;

non-virtual, public methods need to be annotated with WEBKIT_API


> +++ b/WebKit/chromium/public/WebSerializedScriptValue.h
> @@ -53,13 +53,13 @@ public:
>  
>      WEBKIT_API static WebSerializedScriptValue fromString(const WebString&);
>  
> -    WEBKIT_API void reset();
> -    WEBKIT_API void assign(const WebSerializedScriptValue&);
> +    void reset();
> +    void assign(const WebSerializedScriptValue&);
>  
>      bool isNull() const { return m_private.isNull(); }
>  
>      // Returns a string representation of the WebSerializedScriptValue.
> -    WEBKIT_API WebString toString() const;
> +    WebString toString() const;

non-virtual, public methods need to be annotated with WEBKIT_API


> +++ b/WebKit/chromium/src/IDBCallbacksProxy.h
...
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +#ifndef IDBCallbacksProxy_h

same nit about inserting a new line


> +++ b/WebKit/chromium/src/IDBDatabaseProxy.cpp
...
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +#include "config.h"

ditto


> +++ b/WebKit/chromium/src/IDBDatabaseProxy.h
...
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +#ifndef IDBDatabaseProxy_h

ditto


> +++ b/WebKit/chromium/src/IndexedDatabaseProxy.cpp

A "using namespace WebKit" would be nice here to avoid the need for the
WebKit:: prefixes.


> +++ b/WebKit/chromium/src/WebIDBDatabaseError.cpp

> +WebIDBDatabaseError::~WebIDBDatabaseError()
> +{
> +    m_private.reset();
> +}
> +
> +WebIDBDatabaseError::WebIDBDatabaseError(unsigned short code, const WebString& message)
> +    : m_private(IDBDatabaseError::create(code, message))
> +{
> +}

Ordinarily the WebKit API makes constructors and destructors inline.
We add init and reset methods decorated with WEBKIT_API to do the
actual initialization and destruction.  Those methods could be made
private to prevent the consumer from calling them directly.

[Jeremy Orlow]

> 
> > +template <typename ResultType>
> > +class WebIDBCallbacks {
> > +public:
> > +    virtual ~WebIDBCallbacks() { }
> > +
> > +    virtual void onSuccess(ResultType*) = 0;
> > +    virtual void onError(const WebIDBDatabaseError&) = 0;
> > +};
> 
> Does ResultType need to be mutable?  Does it need to be passed as
> a pointer?  Could it be passed as 'const ResultType&'?

WebIDBDatabaseError is just POD.  ResultType is an interface that's implemented in the renderer that gets passed back into WebKit, so it needs to be a pointer.  Ownership is being transfered into the WebKit layer, so it's ok for it not to be const and such.

> > +// See comment in WebIndexedDatabase for a high level overview these classes.
> > +class WebIDBDatabaseError {
> 
> Why not call this WebIDBError?  Same goes for IDBDatabaseError.  Doesn't
> IDB imply Database?

IDBDatabaseError is the name the spec uses.  It's better to match the spec 1:1 than save a few characters, I think.



Everything else is done.  (Will upload in a moment.)

[Jeremy Orlow]

Created attachment 51731 [details]
rev

[Jeremy Orlow]

Btw, I didn't change anything except what you asked me to (no need to hunt for changes).

[Darin Fisher (:fishd, Google)]

(In reply to comment #3)
> > 
> > > +template <typename ResultType>
> > > +class WebIDBCallbacks {
> > > +public:
> > > +    virtual ~WebIDBCallbacks() { }
> > > +
> > > +    virtual void onSuccess(ResultType*) = 0;
> > > +    virtual void onError(const WebIDBDatabaseError&) = 0;
> > > +};
> > 
> > Does ResultType need to be mutable?  Does it need to be passed as
> > a pointer?  Could it be passed as 'const ResultType&'?
> 
> WebIDBDatabaseError is just POD.  ResultType is an interface that's implemented
> in the renderer that gets passed back into WebKit, so it needs to be a pointer.
>  Ownership is being transfered into the WebKit layer, so it's ok for it not to
> be const and such.

OK, this ownership transfer needs to be documented in the interface.


> 
> > > +// See comment in WebIndexedDatabase for a high level overview these classes.
> > > +class WebIDBDatabaseError {
> > 
> > Why not call this WebIDBError?  Same goes for IDBDatabaseError.  Doesn't
> > IDB imply Database?
> 
> IDBDatabaseError is the name the spec uses.  It's better to match the spec 1:1
> than save a few characters, I think.

OK... should the spec change?

[Jeremy Orlow]

(In reply to comment #6)
> (In reply to comment #3)
> > > 
> > > > +template <typename ResultType>
> > > > +class WebIDBCallbacks {
> > > > +public:
> > > > +    virtual ~WebIDBCallbacks() { }
> > > > +
> > > > +    virtual void onSuccess(ResultType*) = 0;
> > > > +    virtual void onError(const WebIDBDatabaseError&) = 0;
> > > > +};
> > > 
> > > Does ResultType need to be mutable?  Does it need to be passed as
> > > a pointer?  Could it be passed as 'const ResultType&'?
> > 
> > WebIDBDatabaseError is just POD.  ResultType is an interface that's implemented
> > in the renderer that gets passed back into WebKit, so it needs to be a pointer.
> >  Ownership is being transfered into the WebKit layer, so it's ok for it not to
> > be const and such.
> 
> OK, this ownership transfer needs to be documented in the interface.

Will do.

> 
> > 
> > > > +// See comment in WebIndexedDatabase for a high level overview these classes.
> > > > +class WebIDBDatabaseError {
> > > 
> > > Why not call this WebIDBError?  Same goes for IDBDatabaseError.  Doesn't
> > > IDB imply Database?
> > 
> > IDBDatabaseError is the name the spec uses.  It's better to match the spec 1:1
> > than save a few characters, I think.
> 
> OK... should the spec change?

There are enough outstanding spec issues that I think it'd be a mistake to bring up now...but yes, it should probably change.

[Darin Fisher (:fishd, Google)]

Comment on attachment 51731 [details]
rev

r=me w/ the comment change

[Jeremy Orlow]

Committed r56713: <http://trac.webkit.org/changeset/56713>