Bug 124562 - Add WebIDBServerConnection and DatabaseProcessIDBConnection stubs
Summary: Add WebIDBServerConnection and DatabaseProcessIDBConnection stubs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 124521
  Show dependency treegraph
 
Reported: 2013-11-18 22:19 PST by Brady Eidson
Modified: 2013-11-19 12:06 PST (History)
4 users (show)

See Also:


Attachments
patch v1 (82.60 KB, patch)
2013-11-18 22:28 PST, Brady Eidson
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2013-11-18 22:19:30 PST
Add WebIDBServerConnection and DatabaseProcessIDBConnection stubs

Also remove Web/DatabaseProcessDatabaseBackend stubs, as that is no longer the abstraction layer.
Comment 1 Brady Eidson 2013-11-18 22:28:02 PST
Created attachment 217274 [details]
patch v1
Comment 2 Alexey Proskuryakov 2013-11-19 10:42:54 PST
Comment on attachment 217274 [details]
patch v1

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?
Comment 3 Brady Eidson 2013-11-19 11:58:01 PST
(In reply to comment #2)

Most of these changed without comment.

> (From update of attachment 217274 [details])
> 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 ref countable?

I think it will shortly.

> > 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.

Chatted with Alexey about this in person, came up with a slight alteration to the patch that will be landed soon.
Comment 4 Brady Eidson 2013-11-19 12:06:07 PST
http://trac.webkit.org/changeset/159511