Bug 88678 - IndexedDB: Add chromium webkit API for metadata snapshots
Summary: IndexedDB: Add chromium webkit API for metadata snapshots
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks: 88467
  Show dependency treegraph
 
Reported: 2012-06-08 11:55 PDT by Joshua Bell
Modified: 2012-06-19 14:20 PDT (History)
8 users (show)

See Also:


Attachments
Patch (20.82 KB, patch)
2012-06-08 11:57 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (6.70 KB, patch)
2012-06-12 09:55 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2012-06-15 11:46 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joshua Bell 2012-06-08 11:55:52 PDT
IndexedDB: Add new type (and chromium webkit API) for metadata snapshot
Comment 1 Joshua Bell 2012-06-08 11:57:07 PDT
Created attachment 146621 [details]
Patch
Comment 2 Joshua Bell 2012-06-08 11:58:34 PDT
Needs an xcode project update to add Modules/indexeddb/IDBMetadata.h
Comment 3 WebKit Review Bot 2012-06-08 12:02:47 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 Darin Fisher (:fishd, Google) 2012-06-10 23:05:32 PDT
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.
Comment 5 Joshua Bell 2012-06-12 09:19:05 PDT
(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.
Comment 6 Joshua Bell 2012-06-12 09:55:21 PDT
Created attachment 147103 [details]
Patch
Comment 7 Joshua Bell 2012-06-12 09:58:57 PDT
(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.
Comment 8 Joshua Bell 2012-06-12 11:52:24 PDT
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.
Comment 9 Alec Flett 2012-06-12 11:57:01 PDT
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?)
Comment 10 Joshua Bell 2012-06-12 12:00:33 PDT
(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.
Comment 11 Darin Fisher (:fishd, Google) 2012-06-14 22:56:38 PDT
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
Comment 12 Joshua Bell 2012-06-15 11:35:14 PDT
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.
Comment 13 Joshua Bell 2012-06-15 11:46:55 PDT
Created attachment 147866 [details]
Patch
Comment 14 Joshua Bell 2012-06-18 13:35:14 PDT
fishd@ or anyone - r?
Comment 15 WebKit Review Bot 2012-06-19 14:19:58 PDT
Comment on attachment 147866 [details]
Patch

Clearing flags on attachment: 147866

Committed r120753: <http://trac.webkit.org/changeset/120753>
Comment 16 WebKit Review Bot 2012-06-19 14:20:08 PDT
All reviewed patches have been landed.  Closing bug.