Add IndexedDB's IDBIndex
+ fishd for WebKit layer + andreip for informal review
Created attachment 57260 [details] Patch
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
Created attachment 57266 [details] Patch
Oops...forgot build files. Also took care of Darin's review comments.
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.
Oops....I thought Andrei (who hopefully does) had already done an informal review (for that reason).
(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. :-/
LGTM with two nits: Android make files have indent problems on lines 717 and below. Same with GNUMake on lines 2501 and below
Created attachment 57327 [details] Patch
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?
Committed r60357: <http://trac.webkit.org/changeset/60357>