Bug 39850

Summary: Add IndexedDB's IDBIndex
Product: WebKit Reporter: Jeremy Orlow <jorlow>
Component: New BugsAssignee: Jeremy Orlow <jorlow>
Status: RESOLVED FIXED    
Severity: Normal CC: andreip, fishd, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch steveblock: review+

Description Jeremy Orlow 2010-05-27 11:03:13 PDT
Add IndexedDB's IDBIndex
Comment 1 Jeremy Orlow 2010-05-27 11:04:27 PDT
+ fishd for WebKit layer
+ andreip for informal review
Comment 2 Jeremy Orlow 2010-05-27 11:07:43 PDT
Created attachment 57260 [details]
Patch
Comment 3 Darin Fisher (:fishd, Google) 2010-05-27 11:15:55 PDT
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
Comment 4 Jeremy Orlow 2010-05-27 12:12:10 PDT
Created attachment 57266 [details]
Patch
Comment 5 Jeremy Orlow 2010-05-27 12:12:37 PDT
Oops...forgot build files.  Also took care of Darin's review comments.
Comment 6 Steve Block 2010-05-28 06:34:47 PDT
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.
Comment 7 Jeremy Orlow 2010-05-28 06:40:47 PDT
Oops....I thought Andrei (who hopefully does) had already done an informal review (for that reason).
Comment 8 Jeremy Orlow 2010-05-28 06:46:47 PDT
(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.  :-/
Comment 9 Andrei Popescu 2010-05-28 07:10:59 PDT
LGTM with two nits:

Android make files have indent problems on lines 717 and below.
Same with GNUMake on lines 2501 and below
Comment 10 Jeremy Orlow 2010-05-28 07:17:25 PDT
Created attachment 57327 [details]
Patch
Comment 11 Jeremy Orlow 2010-05-28 07:17:41 PDT
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?
Comment 12 Jeremy Orlow 2010-05-28 07:31:01 PDT
Committed r60357: <http://trac.webkit.org/changeset/60357>