[webkit-reviews] review granted: [Bug 124562] Add WebIDBServerConnection and DatabaseProcessIDBConnection stubs : [Attachment 217274] patch v1

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 124562: Add WebIDBServerConnection and DatabaseProcessIDBConnection stubs
https://bugs.webkit.org/show_bug.cgi?id=124562

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=217274&action=review


> Source/WebKit2/DatabaseProcess/DatabaseToWebProcessConnection.cpp:84
> +    RefPtr<DatabaseProcessIDBConnection> connection =
DatabaseProcessIDBConnection::create(backendIdentifier);

Just an idle thought, is that actually something that needs to be refcountable?


> Source/WebKit2/Shared/Databases/IndexedDB/IDBUtilities.cpp:40
> +    stringBuilder.append(openingOrigin.databaseIdentifier());

Can we get rid of databaseIdentifier() in IndexedDB? A comment in
SecurityOrigin.cpp implies that we should:

    // Historically, we've used the following (somewhat non-sensical) string
    // for the databaseIdentifier of local files. We used to compute this
    // string because of a bug in how we handled the scheme for file URLs.
    // Now that we've fixed that bug, we still need to produce this string
    // to avoid breaking existing persistent state.

> Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp:42
> +    DEFINE_STATIC_LOCAL(uint64_t, identifier, (1));

ASSERT(isMainThread());

> Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp:43
> +    return identifier++;

Why not start with a 0, and return ++identifier?

> Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp:55
> +

Empty line.

> Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp:66
> +

Ditto.

> Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp:71
> +

Ditto.

> Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp:81
> +

Ditto.

> Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp:86
> +

Looks like there are a few more...

> Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.h:31
> +#if ENABLE(INDEXED_DATABASE)
> +#if ENABLE(DATABASE_PROCESS)

Some of the new files have ENABLE(INDEXED_DATABASE) &&
ENABLE(DATABASE_PROCESS), for what it's worth.

> Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.h:85
> +    explicit WebIDBServerConnection(const String& databaseName, const
WebCore::SecurityOrigin& openingOrigin, const WebCore::SecurityOrigin&
mainFrameOrigin);

Is "explicit" of any use here?