Bug 39850 – Add IndexedDB's IDBIndex

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

[Jeremy Orlow]

Add IndexedDB's IDBIndex

[Jeremy Orlow]

+ fishd for WebKit layer
+ andreip for informal review

[Jeremy Orlow]

Created attachment 57260 [details]
Patch

[Darin Fisher (:fishd, Google)]

Comment on attachment 57260 [details]
Patch

WebKit/chromium/public/WebIDBIndex.h:33
> +  // See comment in WebIndexedDatabase for a high level overview these classes.
nit: overview _of_ these classes

WebKit/chromium/public/WebIDBIndex.h:38
> +      virtual WebString name() { return WebString(); }
should these be const methods?

WebKit/chromium/src/WebIDBCallbacksImpl.h:44
> +      virtual void onError(const WebKit::WebIDBDatabaseError& error);
nit: the parameter names in this section seem redundant

WebKit/* bits look good otherwise

[Jeremy Orlow]

Created attachment 57266 [details]
Patch

[Jeremy Orlow]

Oops...forgot build files.  Also took care of Darin's review comments.

[Steve Block]

Comment on attachment 57266 [details]
Patch

This looks fine to me, but I don't have a good enough grasp of the spec to know where this is heading and to provide a complete review.

WebCore/storage/IDBDatabaseError.idl:43
 +          const unsigned short DEADLOCK_ERR = 33;
Are these constant values from the spec? Why have they moved?

WebCore/storage/IDBDatabaseError.h:47
 +      // Keey in sync with what's in the .idl file.
Keep

WebCore/storage/IDBAny.h:78
 +      // Only one of the following should ever be in use at any given time.
Maybe add a comment to say that set can only be called once.

[Jeremy Orlow]

Oops....I thought Andrei (who hopefully does) had already done an informal review (for that reason).

[Jeremy Orlow]

(In reply to comment #6)
> WebCore/storage/IDBDatabaseError.idl:43
>  +          const unsigned short DEADLOCK_ERR = 33;
> Are these constant values from the spec? Why have they moved?

It really doesn't make any sense for them to be in Exception.  It's not a normal interface (not sure if it's even legal for them to be there...this was brought up on the w3 bug tracker).  And it just felt right.  But now that you mention it, I guess we should probably get upstream consensus first.  :-/

[Andrei Popescu]

LGTM with two nits:

Android make files have indent problems on lines 717 and below.
Same with GNUMake on lines 2501 and below

[Jeremy Orlow]

Created attachment 57327 [details]
Patch

[Jeremy Orlow]

Addressed all comments.  Given darin's review of WebKit/chromium, Andrei's review of the domain specific stuff, I think you can easily give a final r+ Steve?

[Jeremy Orlow]

Committed r60357: <http://trac.webkit.org/changeset/60357>