Bug 35927 – Add IndexedDatabase class and plumbing out of Chromium port

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

[Jeremy Orlow]

This change is mostly just adding the plumbing necessary for the IndexedDatabaseRequest and IndexedDatabaseSync (not written yet).

This code is non-functional, so no tests (yet).

[Jeremy Orlow]

Created attachment 50326 [details]
patch

[WebKit Review Bot]

Attachment 50326 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/storage/IndexedDatabaseImpl.cpp:47:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/storage/IndexedDatabaseImpl.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

Comment on attachment 50326 [details]
patch

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

[Darin Fisher (:fishd, Google)]

Those are just style nits.  The patch looks great otherwise.

[Jeremy Orlow]

Given that we're in different time zones and there are many more patches coming where I can clean up other issues if they arise, can I get a r+ please?

As for s_ IIRC, this is one of the accepted styles, but not the preferred one.  I'll figure out which is preferred and change it, but I still think s_ is much better than any of the other ones.

[Jeremy Orlow]

> > +++ 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?

There isn't.  More code justUsesThisStyle.  Some uses s_.  I think using local variable syntax is really terrible for readability, but I'll change it since you asked.

> 
> 
> > +++ b/WebKit/chromium/public/WebKitClient.h
> 
> > -    // Database ------------------------------------------------------------
> > +    // HTML5 Database ------------------------------------------------------
> 
> "SQL Database" would be better since this API is not part of HTML5.

I added the HTML 5 because it's out of order alphabetically without it (and that's what the other file has).  If we wanted to change it, it probably should be in a subsequent patch that moves the code as well.

[Jeremy Orlow]

http://trac.webkit.org/changeset/55784