[webkit-reviews] review denied: [Bug 36546] A few more steps towards IndexedDB : [Attachment 51529] Patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Jeremy Orlow
<jorlow at chromium.org>'s request for review:
Bug 36546: A few more steps towards IndexedDB
https://bugs.webkit.org/show_bug.cgi?id=36546

Attachment 51529: Patch
https://bugs.webkit.org/attachment.cgi?id=51529&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ 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.