Bug 88678 – IndexedDB: Add chromium webkit API for metadata snapshots

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

[Joshua Bell]

IndexedDB: Add new type (and chromium webkit API) for metadata snapshot

[Joshua Bell]

Created attachment 146621 [details]
Patch

[Joshua Bell]

Needs an xcode project update to add Modules/indexeddb/IDBMetadata.h

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Darin Fisher (:fishd, Google)]

Comment on attachment 146621 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146621&action=review

> Source/WebKit/chromium/public/WebIDBMetadata.h:68
> +class WebIDBObjectStoreMetadata {

nit: one file per top-level class, sorry :(

> Source/WebKit/chromium/public/WebIDBMetadata.h:93
> +    WEBKIT_EXPORT WebIDBIndexMetadata(const WebIDBIndexMetadata&);

nit: the usual pattern is to export an assign method instead, and just inline
the constructor as a call to assign.

is there a reason to hide the default constructor and destructor?  it seems a
bit odd to hide those using WEBKIT_IMPLEMENTATION.  what are you trying to 
achieve by doing that?  normally WEBKIT_IMPLEMENTATION is only used to hide
methods that use WebCore types.

[Joshua Bell]

(In reply to comment #4)
> (From update of attachment 146621 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146621&action=review
> 
> > Source/WebKit/chromium/public/WebIDBMetadata.h:68
> > +class WebIDBObjectStoreMetadata {
> 
> nit: one file per top-level class, sorry :(

Since the usage of these types are relatively scoped, any opinions on this approach:

struct WebIDBMetadata {
    struct ObjectStoreMetadata;
    struct IndexMetadata;

    WebString name;
    WebString version;
    WebVector<ObjectStoreMetadata> objectStores;

    struct ObjectStoreMetadata {
        WebString name;
        WebIDBKeyPath keyPath;
        bool autoIncrement;
        WebVector<IndexMetadata> indexes;
    };

    struct IndexMetadata {
        WebString name;
        WebIDBKeyPath keyPath;
        bool unique;
        bool multiEntry;
    };
};

"WebIDBMetadata::ObjectStoreMetadata" is a bit of a mouthful though.

> > Source/WebKit/chromium/public/WebIDBMetadata.h:93
> > +    WEBKIT_EXPORT WebIDBIndexMetadata(const WebIDBIndexMetadata&);
> 
> nit: the usual pattern is to export an assign method instead, and just inline
> the constructor as a call to assign.
> 
> is there a reason to hide the default constructor and destructor?  it seems a
> bit odd to hide those using WEBKIT_IMPLEMENTATION.  what are you trying to 
> achieve by doing that?  normally WEBKIT_IMPLEMENTATION is only used to hide
> methods that use WebCore types.

No good reason - just cargo-cult structuring. I've dramatically simplified it in an upcoming iteration - basically to just the above plus default constructors.

[Joshua Bell]

Created attachment 147103 [details]
Patch

[Joshua Bell]

(In reply to comment #5)
> 
> "WebIDBMetadata::ObjectStoreMetadata" is a bit of a mouthful though.
> 
... so simplified further to WebIDBMetadata::ObjectStore since the context is provided.

This is the minimum to get the Chromium side landed (http://codereview.chromium.org/10533057/) but doesn't include the WebCore type. That could be added here, in the final patch (https://bugs.webkit.org/show_bug.cgi?id=88467), or in another intermediate patch.

[Joshua Bell]

https://bugs.webkit.org/show_bug.cgi?id=88467 updated with the "final" patch - it includes the WebIDBMetadata.cpp implementation which are minimal methods for converting between WebKit and WebCore values.

I can move that (and the WebCore type) to this patch if that's a better split.

[Alec Flett]

Comment on attachment 147103 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147103&action=review

> Source/WebKit/chromium/src/WebIDBKeyPath.cpp:67
> +    return *this;

LGTM - I'm personally not a huge fan of operator=() in general, but especially when the implementation includes memory allocation, but it seems consistent with related stuff I've seen here.

(I actually wonder if in the future, m_private could just be a local copy of the entire IDBKey rather than a pointer to a tiny new'ed object?)

[Joshua Bell]

(In reply to comment #9)
> (From update of attachment 147103 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147103&action=review
> 
> > Source/WebKit/chromium/src/WebIDBKeyPath.cpp:67
> > +    return *this;
> 
> LGTM - I'm personally not a huge fan of operator=() in general, but especially when the implementation includes memory allocation, but it seems consistent with related stuff I've seen here.
> 
> (I actually wonder if in the future, m_private could just be a local copy of the entire IDBKey rather than a pointer to a tiny new'ed object?)

In the future I don't want this exposed at all. :) But yeah - I think my WebIDBKeyPath impl uses the worst parts from other WebXXX classes, and could use a do-over.

[Darin Fisher (:fishd, Google)]

Comment on attachment 147103 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147103&action=review

> Source/WebKit/chromium/public/WebIDBKeyPath.h:48
>      WEBKIT_EXPORT WebIDBKeyPath(const WebIDBKeyPath&);

nit: the normal pattern is to implement copy constructor and assignment operator
inline in terms of an assign function.  they don't need to be exported then.
see WebNode.h for example.

> Source/WebKit/chromium/public/WebIDBMetadata.h:31
> +#include "platform/WebPrivateOwnPtr.h"

nit: looks like you don't need this header.

> Source/WebKit/chromium/public/WebIDBMetadata.h:35
> +namespace WebCore {

looks like you don't need these forward declarations

> Source/WebKit/chromium/public/WebIDBMetadata.h:50
> +    WebIDBMetadata()

nit: no need to write this constructor

> Source/WebKit/chromium/public/WebIDBMetadata.h:58
> +        WebVector<Index> indexes;

nit: indexes -> indices?

> Source/WebKit/chromium/public/WebIDBMetadata.h:60
> +            : name(WebString())

nit: no need for explicit constructor here

> Source/WebKit/chromium/public/WebIDBMetadata.h:71
> +            : name(WebString())

ditto

[Joshua Bell]

Comment on attachment 147103 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147103&action=review

>> Source/WebKit/chromium/public/WebIDBKeyPath.h:48
>>      WEBKIT_EXPORT WebIDBKeyPath(const WebIDBKeyPath&);
> 
> nit: the normal pattern is to implement copy constructor and assignment operator
> inline in terms of an assign function.  they don't need to be exported then.
> see WebNode.h for example.

Awesome, thanks for the pointer. Corrected.

>> Source/WebKit/chromium/public/WebIDBMetadata.h:31
>> +#include "platform/WebPrivateOwnPtr.h"
> 
> nit: looks like you don't need this header.

Oops, yes. Simplified but left these behind. Done.

>> Source/WebKit/chromium/public/WebIDBMetadata.h:35
>> +namespace WebCore {
> 
> looks like you don't need these forward declarations

Ditto. Removed.

>> Source/WebKit/chromium/public/WebIDBMetadata.h:50
>> +    WebIDBMetadata()
> 
> nit: no need to write this constructor

Removed.

>> Source/WebKit/chromium/public/WebIDBMetadata.h:58
>> +        WebVector<Index> indexes;
> 
> nit: indexes -> indices?

I waffled on that myself. The interwebz say both are considered correct English at this point. The IDB spec uses "indexes" in prose (neither in the API itself - just "indexNames") and we use "indexes" elsewhere in WebKit so that's what I've gone with.

>> Source/WebKit/chromium/public/WebIDBMetadata.h:60
>> +            : name(WebString())
> 
> nit: no need for explicit constructor here

Removed.

>> Source/WebKit/chromium/public/WebIDBMetadata.h:71
>> +            : name(WebString())
> 
> ditto

Done.

[Joshua Bell]

Created attachment 147866 [details]
Patch

[Joshua Bell]

fishd@ or anyone - r?

[WebKit Review Bot]

Comment on attachment 147866 [details]
Patch

Clearing flags on attachment: 147866

Committed r120753: <http://trac.webkit.org/changeset/120753>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.