[webkit-reviews] review denied: [Bug 35927] Add IndexedDatabase class and plumbing out of Chromium port : [Attachment 50326] 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 35927: Add IndexedDatabase class and plumbing out of Chromium port
https://bugs.webkit.org/show_bug.cgi?id=35927

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

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> +++ b/WebCore/platform/chromium/ChromiumBridge.h
...
> +	   // IndexedDB
----------------------------------------------------------
> +	   static PassRefPtr<IndexedDatabase> getIndexedDatabase();

ordinarily, we drop the "get" prefix for accessors like this.  it should just
be
indexedDatabase().


> +++ b/WebCore/storage/IndexedDatabase.h
...
> +namespace WebCore {
> +
> +class IndexedDatabase : public ThreadSafeShared<IndexedDatabase> {

It might helpful to add some comments here about the threading model.


> +++ b/WebCore/storage/IndexedDatabaseImpl.cpp
...
> +namespace WebCore {
> +
> +IndexedDatabaseImpl* IndexedDatabaseImpl::s_indexedDatabaseImpl = 0;

I didn't think it was WebKit style to use the "s_" prefix.  I prefer it
myself since I think it helps code readability.  Do you know if there
is any rule about this?


> +
> +PassRefPtr<IndexedDatabaseImpl> IndexedDatabaseImpl::get()
> +{
> +    if (!s_indexedDatabaseImpl)
> +	   s_indexedDatabaseImpl = new IndexedDatabaseImpl();
> +    ASSERT(s_indexedDatabaseImpl);
> +    return s_indexedDatabaseImpl;
> +}
> +
> +IndexedDatabaseImpl::IndexedDatabaseImpl() {
> +    // FIXME: Make this thread safe.
> +    ASSERT(!s_indexedDatabaseImpl);
> +    s_indexedDatabaseImpl = this;
> +}
> +
> +IndexedDatabaseImpl::~IndexedDatabaseImpl() {

brackets on the next line.


> +++ b/WebKit/chromium/public/WebKitClient.h

> -    // Database ------------------------------------------------------------

> +    // HTML5 Database ------------------------------------------------------


"SQL Database" would be better since this API is not part of HTML5.


> +    // Indexed Database ----------------------------------------------------

> +
> +    virtual WebIndexedDatabase* getIndexedDatabase() { return 0; }

indexedDatabase