Bug 41888 – Initial bindings and plumbing for IDBCursor.

• Component: New Bugs
• Status: RESOLVED FIXED

[Marcus Bulach]

Initial bindings and plumbing for IDBCursor.

[Marcus Bulach]

Created attachment 60928 [details]
Patch

[WebKit Review Bot]

Attachment 60928 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/IDBCursorRequest.cpp:71:  IDBCursorRequest::continue_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBCursorRequest.h:53:  continue_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Marcus Bulach]

Hi jorlow, andreip,

Couple of notes:
1. The patch is not yet entirely finished. I didn't add the new files to most of the projects (specially xcode) in order to keep the meat slightly more visible.

2. This patch also includes https://bugs.webkit.org/show_bug.cgi?id=41613. Please, skip anything related to the code generator here, and focus on the bindings / proxies.

3. There's also a slightly unrelated change for avoiding the cyclic dependency between IDBAny and IDBDatabaseRequest and a few adoptRef fixes due to http://trac.webkit.org/changeset/62213. If you guys prefer I'm happy to split on a change of its own.

4. The layout test exercises the cursor end-to-end when opening the browser (yay!), but it depends on jorlow's change to actually do anything useful, i.e., iterate the cursor, rather than just opening it.

Thanks!
Marcus

(In reply to comment #1)
> Created an attachment (id=60928) [details]
> Patch

[Andrei Popescu]

Looks good, some comments:

> WebCore/GNUmakefile.am

I think you have some tabs there or the indentation is wrong.

> WebCore/storage/IDBCursorRequest.idl

We don't have that in the spec anymore. I am not sure anymore what we're implementing, though...My suggestion would be to remove this altogether and only add a single IDL file, as per the spec.

// class IDBCursor
// ...
// PassRefPtr<IDBKeyRange> m_keyRange;

You don't want to store a PassRefPtr...make it a RefPtr?

>> PassRefPtr<IDBRequest> request() const;
>>     PassRefPtr<IDBAny> key() const;
 >>     PassRefPtr<IDBAny> value() const;

Why do these return PassRefPtr? Do we transfer ownership? If not, can they be raw pointers?

> class WebIDBKeyRange {
> WebIDBKeyRange(const WebIDBKeyRange& d) { m_private = d.m_private; }

I think you're supposed to use meaningful variable names instead of "d".

[Jeremy Orlow]

Comment on attachment 60928 [details]
Patch

WebKit/chromium/src/WebIDBObjectStoreImpl.cpp:88
 +  
remove extra newline

WebKit/chromium/src/WebIDBKeyRange.cpp:39
 +  
get rid of this extra newline

WebKit/chromium/src/WebIDBKeyRange.cpp:42
 +  #if WEBKIT_IMPLEMENTATION
This is always defined in chromium/src

WebKit/chromium/src/WebIDBKeyRange.cpp:64
 +  void WebIDBKeyRange::reset()
Do you actually need this?

Where's the WebIDBKeyRange.h?  Why isn't WebIDBKeyRange designed like WebIDBDatabaseError (and such)...i.e. a thin wrapper around a WebCore type?  We only do this stuff with Impls and proxies when chromium needs to implement something as well.

WebKit/chromium/src/WebIDBCursorImpl.cpp:32
 +  #if ENABLE(INDEXED_DATABASE)
We don't use guards in WebKit code usually...we just assume it's on.

WebKit/chromium/src/WebIDBCallbacksImpl.cpp:67
 +  void WebIDBCallbacksImpl::onSuccess(WebKit::WebIDBCursor* webKitInstance)
This should probably follow the model of database errors...


WebCore/storage/IndexedDatabaseRequest.cpp:66
 +      RefPtr<IDBAny> db = IDBAny::create();
Should we add ::create() functions for each type so we can do this inline when creating the requests rather than needing 3 lines to do it?

WebCore/storage/IDBObjectStoreRequest.h:70
 +      PassRefPtr<IDBRequest> openCursor(ScriptExecutionContext*, PassRefPtr<IDBKeyRange> range = PassRefPtr<IDBKeyRange>(), unsigned short direction = IDBCursor::NEXT);
= 0 is the same thing

WebCore/storage/IDBObjectStoreRequest.cpp:106
 +      RefPtr<IDBAny> db = IDBAny::create();
Hm...these changes shoudl probably all be done in their own cl.

WebCore/storage/IDBObjectStoreImpl.h:53
 +      void openCursor(PassRefPtr<IDBCallbacks>, PassRefPtr<IDBKeyRange> range = PassRefPtr<IDBKeyRange>(), unsigned short direction = IDBCursor::NEXT);
don't need default params

WebCore/storage/IDBObjectStore.h:55
 +      virtual void openCursor(PassRefPtr<IDBCallbacks>, PassRefPtr<IDBKeyRange> range = PassRefPtr<IDBKeyRange>(), unsigned short direction = IDBCursor::NEXT) = 0;
don't need default params

WebCore/storage/IDBKeyRange.h:61
 +  protected:
Why protected?

WebCore/storage/IDBCursorRequest.idl:37
 +          void continue(in [Optional] IDBAny key);
I believe this should be an IDBKey

WebCore/storage/IDBCursorRequest.idl:36
 +          void update(in IDBAny value);
This should probably be a serializedScriptValue

IDBAny is only a return type...and perhaps not very well named?

WebCore/storage/IDBCursorRequest.cpp:79
 +      // FIXME: Implement.
ASSERT_NOT_IMPLEMENTED();

and above

WebCore/storage/IDBCursor.h:53
 +      virtual unsigned short direction() const { return m_direction; }
virtual?  If so, the ~ should be virtual as well probably.

WebCore/storage/IDBCursor.h:37
 +  // FIXME: Make this class pure virtual.
What's the hold up?



These comments are not exhaustive...shoudl get you started tho.

[Marcus Bulach]

Created attachment 61392 [details]
Patch

[WebKit Review Bot]

Attachment 61392 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/storage/IDBCursorRequest.cpp:78:  IDBCursorRequest::continue_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBCursorRequest.h:53:  continue_ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 2 in 55 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Marcus Bulach]

(In reply to comment #4)
> Looks good, some comments:
> 
> > WebCore/GNUmakefile.am
> 
> I think you have some tabs there or the indentation is wrong.
done (but I haven't added all the files to all makefiles yet anyway..)

> 
> > WebCore/storage/IDBCursorRequest.idl
> 
> We don't have that in the spec anymore. I am not sure anymore what we're implementing, though...My suggestion would be to remove this altogether and only add a single IDL file, as per the spec.

as we chatted, I'll do the big cleanup next.

> 
> // class IDBCursor
> // ...
> // PassRefPtr<IDBKeyRange> m_keyRange;
> 
> You don't want to store a PassRefPtr...make it a RefPtr?

done.

> 
> >> PassRefPtr<IDBRequest> request() const;
> >>     PassRefPtr<IDBAny> key() const;
>  >>     PassRefPtr<IDBAny> value() const;
> 
> Why do these return PassRefPtr? Do we transfer ownership? If not, can they be raw pointers?
done.

> 
> > class WebIDBKeyRange {
> > WebIDBKeyRange(const WebIDBKeyRange& d) { m_private = d.m_private; }
> 
> I think you're supposed to use meaningful variable names instead of "d".
done.

[Marcus Bulach]

(In reply to comment #5)
> (From update of attachment 60928 [details])
> WebKit/chromium/src/WebIDBObjectStoreImpl.cpp:88
>  +  
> remove extra newline
> 
> WebKit/chromium/src/WebIDBKeyRange.cpp:39
>  +  
> get rid of this extra newline
done

> 
> WebKit/chromium/src/WebIDBKeyRange.cpp:42
>  +  #if WEBKIT_IMPLEMENTATION
> This is always defined in chromium/src
makes sense. done.

> 
> WebKit/chromium/src/WebIDBKeyRange.cpp:64
>  +  void WebIDBKeyRange::reset()
> Do you actually need this?
removed.

> 
> Where's the WebIDBKeyRange.h?  Why isn't WebIDBKeyRange designed like WebIDBDatabaseError (and such)...i.e. a thin wrapper around a WebCore type?  We only do this stuff with Impls and proxies when chromium needs to implement something as well.

WebIDBKeyRange.h is (was?) in the patch. anyways, made a thin wrapper, removed the proxy. thanks for the clarification!

> 
> WebKit/chromium/src/WebIDBCursorImpl.cpp:32
>  +  #if ENABLE(INDEXED_DATABASE)
> We don't use guards in WebKit code usually...we just assume it's on.
done.

> 
> WebKit/chromium/src/WebIDBCallbacksImpl.cpp:67
>  +  void WebIDBCallbacksImpl::onSuccess(WebKit::WebIDBCursor* webKitInstance)
> This should probably follow the model of database errors...
not sure I understood this bit?

> WebCore/storage/IndexedDatabaseRequest.cpp:66
>  +      RefPtr<IDBAny> db = IDBAny::create();
> Should we add ::create() functions for each type so we can do this inline when creating the requests rather than needing 3 lines to do it?
done in:
 https://bugs.webkit.org/show_bug.cgi?id=42161
landed, merged in the latest patch.

> 
> WebCore/storage/IDBObjectStoreRequest.h:70
>  +      PassRefPtr<IDBRequest> openCursor(ScriptExecutionContext*, PassRefPtr<IDBKeyRange> range = PassRefPtr<IDBKeyRange>(), unsigned short direction = IDBCursor::NEXT);
> = 0 is the same thing
done.

> 
> WebCore/storage/IDBObjectStoreRequest.cpp:106
>  +      RefPtr<IDBAny> db = IDBAny::create();
> Hm...these changes shoudl probably all be done in their own cl.
done in:
 https://bugs.webkit.org/show_bug.cgi?id=42161

> 
> WebCore/storage/IDBObjectStoreImpl.h:53
>  +      void openCursor(PassRefPtr<IDBCallbacks>, PassRefPtr<IDBKeyRange> range = PassRefPtr<IDBKeyRange>(), unsigned short direction = IDBCursor::NEXT);
> don't need default params
removed.

> 
> WebCore/storage/IDBObjectStore.h:55
>  +      virtual void openCursor(PassRefPtr<IDBCallbacks>, PassRefPtr<IDBKeyRange> range = PassRefPtr<IDBKeyRange>(), unsigned short direction = IDBCursor::NEXT) = 0;
> don't need default params
removed.

> 
> WebCore/storage/IDBKeyRange.h:61
>  +  protected:
> Why protected?
reverted (this was needed for the proxy, now that it's a thin wrapper it's no longer necessary).

> 
> WebCore/storage/IDBCursorRequest.idl:37
>  +          void continue(in [Optional] IDBAny key);
> I believe this should be an IDBKey
done.

> 
> WebCore/storage/IDBCursorRequest.idl:36
>  +          void update(in IDBAny value);
> This should probably be a serializedScriptValue
done.

> 
> IDBAny is only a return type...and perhaps not very well named?
> 
> WebCore/storage/IDBCursorRequest.cpp:79
>  +      // FIXME: Implement.
> ASSERT_NOT_IMPLEMENTED();
done (nit: notImplemented())

> 
> and above
> 
> WebCore/storage/IDBCursor.h:53
>  +      virtual unsigned short direction() const { return m_direction; }
> virtual?  If so, the ~ should be virtual as well probably.
made it a pure virtual interface.

> 
> WebCore/storage/IDBCursor.h:37
>  +  // FIXME: Make this class pure virtual.
> What's the hold up?
I should've done before...

> 
> 
> 
> These comments are not exhaustive...shoudl get you started tho.
thanks! I uploaded another patch. I'd appreciate if you could please take another *quick* :) look at it, as it's not yet final.
(it depends on your add / serializedscriptvalue landing..)

[WebKit Review Bot]

Attachment 61392 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3486290

[Adam Barth]

Comment on attachment 61392 [details]
Patch

This patch is too large.  Please at least do the WebKit API changes in a separate patch.  Also, consider breaking this patch into even smaller pieces.  Drive-by comments below.

WebCore/GNUmakefile.am:2580
 +    WebCore/storage/IDBCursor.idl \
Bad indent.

WebCore/WebCore.vcproj/WebCore.vcproj:50055
 +        <File
Bad indent.

WebCore/bindings/scripts/CodeGenerator.pm:480
 +      return $name . "_" if exists $cppReservedKeyWords{$name};
Yuck

WebCore/storage/IDBCursor.h:39
 +      // Keep in sync with what's in the .idl file.
Can we assert this at compile time?

WebCore/storage/IDBCursorImpl.cpp:35
 +  class IDBCursorImpl : public IDBCursor {
What's the point of hiding the Impl class?

WebCore/storage/IDBObjectStoreRequest.cpp:115
 +      return request;
request.release()

Please read http://webkit.org/coding/RefPtr.html if you haven't already.

WebKit/chromium/public/WebIDBKeyRange.h:32
 +  namespace WebCore { class IDBKeyRange; }
Please put this on multiple lines so we can add more as needed.

[Jeremy Orlow]

(In reply to comment #11)
> (From update of attachment 61392 [details])
> This patch is too large.  Please at least do the WebKit API changes in a separate patch.  Also, consider breaking this patch into even smaller pieces.  Drive-by comments below.

It's mostly just boilerplate/build files.  And the only way to land this in chunks is either to land dead code (which we're normally against as a project) or to break chromium in the process (like splitting the WebKit parts into their own patch).

I'm happy to review this as is, so I don't think he should split it up.

> WebCore/bindings/scripts/CodeGenerator.pm:480
>  +      return $name . "_" if exists $cppReservedKeyWords{$name};
> Yuck

Really?  Most of the file looks like this.

> WebCore/storage/IDBCursor.h:39
>  +      // Keep in sync with what's in the .idl file.
> Can we assert this at compile time?

The only way I can think of is if we added some option for the code generator to add such asserts.  This actually does seem like a good idea.

> WebCore/storage/IDBCursorImpl.cpp:35
>  +  class IDBCursorImpl : public IDBCursor {
> What's the point of hiding the Impl class?

So that half can run in the Chrome browser process.  This is how all of IndexedDB, LocalStorage, and apparently now WebKit2 works.

> WebCore/storage/IDBObjectStoreRequest.cpp:115
>  +      return request;
> request.release()

> WebKit/chromium/public/WebIDBKeyRange.h:32
>  +  namespace WebCore { class IDBKeyRange; }
> Please put this on multiple lines so we can add more as needed.

This is done many places in the WebKit API last time I checked.  After thinking about it, it does seem slightly better but I'd be interested to hear what Darin thinks.

[Adam Barth]

> It's mostly just boilerplate/build files.  And the only way to land this in chunks is either to land dead code (which we're normally against as a project) or to break chromium in the process (like splitting the WebKit parts into their own patch).
>
> I'm happy to review this as is, so I don't think he should split it up.

Ok.  You'll need fishd to review as well since this touches the WebKit API.

> > WebCore/bindings/scripts/CodeGenerator.pm:480
> >  +      return $name . "_" if exists $cppReservedKeyWords{$name};
> > Yuck
> 
> Really?  Most of the file looks like this.

I mean that we're mangling the IDL names in this magical way.  Maybe a better design would be to add an IDL attribute to explain exactly what's going on?  [ImplementationName=continue_] ?

> > WebCore/storage/IDBCursorImpl.cpp:35
> >  +  class IDBCursorImpl : public IDBCursor {
> > What's the point of hiding the Impl class?
> 
> So that half can run in the Chrome browser process.  This is how all of IndexedDB, LocalStorage, and apparently now WebKit2 works.

Ok.  that wasn't clear to me from the patch, but I'm not an expert on this stuff.

[Marcus Bulach]

thanks, Adam! replies inline:

(In reply to comment #11)
> (From update of attachment 61392 [details])
> This patch is too large.  Please at least do the WebKit API changes in a separate patch.  Also, consider breaking this patch into even smaller pieces.  Drive-by comments below.
> 
> WebCore/GNUmakefile.am:2580
>  +    WebCore/storage/IDBCursor.idl \
> Bad indent.
> 
> WebCore/WebCore.vcproj/WebCore.vcproj:50055
>  +        <File
> Bad indent.
as mentioned above, this is not yet finished. please ignore any project files for the time being.

> 
> WebCore/bindings/scripts/CodeGenerator.pm:480
>  +      return $name . "_" if exists $cppReservedKeyWords{$name};
> Yuck
Yuck indeed! :)
This is being implemented as a separate patch, I added you to
https://bugs.webkit.org/show_bug.cgi?id=41613
Happy to hear any suggestion.

> 
> WebCore/storage/IDBCursor.h:39
>  +      // Keep in sync with what's in the .idl file.
> Can we assert this at compile time?
Good idea! We could probably improve the code generator to assert this.
I'll add a separate patch for it.

> WebCore/storage/IDBCursorImpl.cpp:35
>  +  class IDBCursorImpl : public IDBCursor {
> What's the point of hiding the Impl class?
as jorlow replied below.

> 
> WebCore/storage/IDBObjectStoreRequest.cpp:115
>  +      return request;
> request.release()
ugh. done, in several places in the file.

> 
> Please read http://webkit.org/coding/RefPtr.html if you haven't already.
thanks for the refresher.

> 
> WebKit/chromium/public/WebIDBKeyRange.h:32
>  +  namespace WebCore { class IDBKeyRange; }
> Please put this on multiple lines so we can add more as needed.
as jeremy said, this is fairly common.

[Darin Fisher (:fishd, Google)]

Comment on attachment 61392 [details]
Patch

WebKit/chromium/public/WebIDBKeyRange.h:48
 +  #if WEBKIT_IMPLEMENTATION
nit: the WEBKIT_IMPLEMENTATION section normally goes just before the private: label.

WebKit/chromium/src/IDBCursorProxy.h:52
 +  
nit: only one new line here

[Marcus Bulach]

Created attachment 61831 [details]
Patch

[Marcus Bulach]

(In reply to comment #16)
> Created an attachment (id=61831) [details]
> Patch

hi everyone, thanks all for the many comments!
this is NOT yet final, I'll still do a round of cleanup / address some of the comments.

however, my main goal with this patch is to to have the bare minimum implementation in order to have a layout test that successfully adds a key/value pair and retrieves it via cursor.

I'll upload a "tidy" patch soon but I'd appreciate any early comments, specially with regards to the architecture / layering, and/or ideas on how to split this patch without adding dead / untested code.

Thanks!

[Marcus Bulach]

(In reply to comment #17)
> (In reply to comment #16)
> > Created an attachment (id=61831) [details] [details]
> > Patch
> 
> hi everyone, thanks all for the many comments!
> this is NOT yet final, I'll still do a round of cleanup / address some of the comments.
> 
> however, my main goal with this patch is to to have the bare minimum implementation in order to have a layout test that successfully adds a key/value pair and retrieves it via cursor.
> 
> I'll upload a "tidy" patch soon but I'd appreciate any early comments, specially with regards to the architecture / layering, and/or ideas on how to split this patch without adding dead / untested code.
> 
> Thanks!

I split some independent bits of this patch into:
https://bugs.webkit.org/show_bug.cgi?id=42583, hopefully it'll help a bit with reviewing.

[Marcus Bulach]

Created attachment 62853 [details]
Patch

[Marcus Bulach]

(In reply to comment #19)
> Created an attachment (id=62853) [details]
> Patch

the other patches have landed, this is now ready for reaview. another look please?

[WebKit Review Bot]

Attachment 62853 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3576612

[Marcus Bulach]

Created attachment 62935 [details]
Patch

[Jeremy Orlow]

Comment on attachment 62935 [details]
Patch

Please redo the names in this patch to match the latest flavor of the week.

LayoutTests/storage/indexeddb/script-tests/open-cursor.js:29
 +  function openCursor(objectStore)
Maybe put above dataAddedSuccess so everything's in revert chronological order?

LayoutTests/storage/indexeddb/script-tests/open-cursor.js:16
 +  function dataAddedSuccess()
Not needed...just have the success call open cursor directly or pull that logic into this.

There's no inheritance of IDLs in the latest spec.  So please merge the 2 files.

WebCore/GNUmakefile.am:2854
 +    WebCore/storage/IDBCursor.cpp \
spacing issue

WebCore/storage/IDBCursor.h:43
 +      // Keep in sync with what's in the .idl file.
Remove comment and use your automated mechanism.

WebCore/storage/IDBCursor.h:41
 +  class IDBCursor : public RefCounted<IDBCursor> {
The interfaces we've been doing threadsafe ref counted since they could be used on a worker or the main thread once we add it to workers.  (If I were doing it again, I would have waited until we actually needed it to be threadsafe to do this, but it's better to be consistent at this point I think.)

WebCore/storage/IDBCursorImpl.cpp:29
 +  #include "IDBAny.h"
put inside guard...here an elsewhere...from now on

WebCore/storage/IDBCursorImpl.cpp:68
 +  long long IDBCursorImpl::count() const
Could just be inline...but doesn't matter much either way.

WebCore/storage/IDBCursorImpl.cpp:74
 +  {
ASSERT_NOT_REACHED()

+ FIXME

WebCore/storage/IDBCursorImpl.h:57
 +      unsigned short m_direction;
This should be an enum, right?

WebCore/storage/IDBCursorImpl.h:60
 +      int m_count;
long long?

WebCore/storage/IDBCursorImpl.h:43
 +      IDBCursorImpl(PassRefPtr<IDBObjectStoreImpl>, PassRefPtr<IDBKeyRange>, unsigned short direction, PassRefPtr<IDBKey>, PassRefPtr<SerializedScriptValue>, int count);
make private, add ::create method.

WebCore/storage/IDBCursorRequest.cpp:62
 +      return m_cursor->count();
Could be inline...but I don't care much.

WebCore/storage/IDBObjectStoreImpl.cpp:139
 +      int count = value ? 1 : 0;
we should move count all together since we're not doing sync cursors.

WebCore/storage/IDBObjectStoreImpl.cpp:141
 +        value = SerializedScriptValue::create();
Aren't we supposed to return a null cursor in this case?

WebCore/storage/IDBObjectStoreImpl.cpp:138
 +      RefPtr<SerializedScriptValue> value = m_tree->get(key.get());
This isn't right...what if the value doesn't exist but we can find something higher/lower (depending on cursor type) than that?  m_tree will need some new methods I guess.

WebCore/storage/IDBObjectStoreImpl.cpp:137
 +      RefPtr<IDBKey> key = range->left();
This doesn't handle direction.

WebCore/storage/IDBObjectStoreRequest.cpp:66
 +      return request.release();
Maybe do in another patch?  (And look at the other files too?)

WebKit/chromium/public/WebIDBCursor.h:44
 +          WEBKIT_ASSERT_NOT_REACHED();
Should be easy to implement in this CL.

WebKit/chromium/public/WebIDBKeyRange.h:43
 +      WEBKIT_API WebIDBKeyRange(const WebIDBKeyRange& keyRange) { m_private = keyRange.m_private; }
This won't work.  You need to make an assign method that does this and just call assign.

Similarly ~ should be inline and call reset().

All methods that aren't inline and aren't virtual should have a WEBKIT_API prefix.

WebKit/chromium/src/IDBCursorProxy.h:56
 +      PassOwnPtr<WebKit::WebIDBCursor> m_idbCursor;
RefPtr

[Marcus Bulach]

Created attachment 63462 [details]
Patch

[Marcus Bulach]

thanks for the detailed review, Jeremy!
replies inline:

(In reply to comment #23)
> (From update of attachment 62935 [details])
> Please redo the names in this patch to match the latest flavor of the week.
done.. this is now inline with the new spec, but everything has changed since the last patch. please, take a deep look at it.
andrei, could you also double check the naming conventions match the spec?

> 
> LayoutTests/storage/indexeddb/script-tests/open-cursor.js:29
>  +  function openCursor(objectStore)
> Maybe put above dataAddedSuccess so everything's in revert chronological order?
done.

> 
> LayoutTests/storage/indexeddb/script-tests/open-cursor.js:16
>  +  function dataAddedSuccess()
> Not needed...just have the success call open cursor directly or pull that logic into this.
done.

> 
> There's no inheritance of IDLs in the latest spec.  So please merge the 2 files.
done.

> 
> WebCore/GNUmakefile.am:2854
>  +    WebCore/storage/IDBCursor.cpp \
> spacing issue
done.

> 
> WebCore/storage/IDBCursor.h:43
>  +      // Keep in sync with what's in the .idl file.
> Remove comment and use your automated mechanism.
uh, the automated mechanism just adds compile-time asserts, it's still necessary to keep in sync..

> 
> WebCore/storage/IDBCursor.h:41
>  +  class IDBCursor : public RefCounted<IDBCursor> {
> The interfaces we've been doing threadsafe ref counted since they could be used on a worker or the main thread once we add it to workers.  (If I were doing it again, I would have waited until we actually needed it to be threadsafe to do this, but it's better to be consistent at this point I think.)

Done.

> 
> WebCore/storage/IDBCursorImpl.cpp:29
>  +  #include "IDBAny.h"
> put inside guard...here an elsewhere...from now on

Done.

> 
> WebCore/storage/IDBCursorImpl.cpp:68
>  +  long long IDBCursorImpl::count() const
> Could just be inline...but doesn't matter much either way.
count() has been removed.

> 
> WebCore/storage/IDBCursorImpl.cpp:74
>  +  {
> ASSERT_NOT_REACHED()
> 
> + FIXME
done.

> 
> WebCore/storage/IDBCursorImpl.h:57
>  +      unsigned short m_direction;
> This should be an enum, right?
yep, done.

> 
> WebCore/storage/IDBCursorImpl.h:60
>  +      int m_count;
> long long?
count has been removed.

> 
> WebCore/storage/IDBCursorImpl.h:43
>  +      IDBCursorImpl(PassRefPtr<IDBObjectStoreImpl>, PassRefPtr<IDBKeyRange>, unsigned short direction, PassRefPtr<IDBKey>, PassRefPtr<SerializedScriptValue>, int count);
> make private, add ::create method.
Done.

> 
> WebCore/storage/IDBCursorRequest.cpp:62
>  +      return m_cursor->count();
> Could be inline...but I don't care much.
count has been removed.

> 
> WebCore/storage/IDBObjectStoreImpl.cpp:139
>  +      int count = value ? 1 : 0;
> we should move count all together since we're not doing sync cursors.
agreed. removed.

> 
> WebCore/storage/IDBObjectStoreImpl.cpp:141
>  +        value = SerializedScriptValue::create();
> Aren't we supposed to return a null cursor in this case?
sounds good. done.

> 
> WebCore/storage/IDBObjectStoreImpl.cpp:138
>  +      RefPtr<SerializedScriptValue> value = m_tree->get(key.get());
> This isn't right...what if the value doesn't exist but we can find something higher/lower (depending on cursor type) than that?  m_tree will need some new methods I guess.
yeah, as we talked, this is the bare minimum necessary to exercise the API and the plumbing pipeline, so it works only with a single key.
continue, update, remove aren't implemented yet, I guess it's better to wait for the new actual storage before implementing them.


> 
> WebCore/storage/IDBObjectStoreImpl.cpp:137
>  +      RefPtr<IDBKey> key = range->left();
> This doesn't handle direction.
as above.

> 
> WebCore/storage/IDBObjectStoreRequest.cpp:66
>  +      return request.release();
> Maybe do in another patch?  (And look at the other files too?)
I'll split this in a separate patch soon.

> 
> WebKit/chromium/public/WebIDBCursor.h:44
>  +          WEBKIT_ASSERT_NOT_REACHED();
> Should be easy to implement in this CL.
as above, not yet implemented.

> 
> WebKit/chromium/public/WebIDBKeyRange.h:43
>  +      WEBKIT_API WebIDBKeyRange(const WebIDBKeyRange& keyRange) { m_private = keyRange.m_private; }
> This won't work.  You need to make an assign method that does this and just call assign.
> 
> Similarly ~ should be inline and call reset().
> 
> All methods that aren't inline and aren't virtual should have a WEBKIT_API prefix.
thanks for the explanation, fixed.


> 
> WebKit/chromium/src/IDBCursorProxy.h:56
>  +      PassOwnPtr<WebKit::WebIDBCursor> m_idbCursor;
> RefPtr
OwnPtr, actually :)
done.

[Marcus Bulach]

more renames went in, I'm merging this patch (again).. please wait for a new patch to review.

(In reply to comment #25)
> thanks for the detailed review, Jeremy!
> replies inline:
> 
> (In reply to comment #23)
> > (From update of attachment 62935 [details] [details])
> > Please redo the names in this patch to match the latest flavor of the week.
> done.. this is now inline with the new spec, but everything has changed since the last patch. please, take a deep look at it.
> andrei, could you also double check the naming conventions match the spec?
> 
> > 
> > LayoutTests/storage/indexeddb/script-tests/open-cursor.js:29
> >  +  function openCursor(objectStore)
> > Maybe put above dataAddedSuccess so everything's in revert chronological order?
> done.
> 
> > 
> > LayoutTests/storage/indexeddb/script-tests/open-cursor.js:16
> >  +  function dataAddedSuccess()
> > Not needed...just have the success call open cursor directly or pull that logic into this.
> done.
> 
> > 
> > There's no inheritance of IDLs in the latest spec.  So please merge the 2 files.
> done.
> 
> > 
> > WebCore/GNUmakefile.am:2854
> >  +    WebCore/storage/IDBCursor.cpp \
> > spacing issue
> done.
> 
> > 
> > WebCore/storage/IDBCursor.h:43
> >  +      // Keep in sync with what's in the .idl file.
> > Remove comment and use your automated mechanism.
> uh, the automated mechanism just adds compile-time asserts, it's still necessary to keep in sync..
> 
> > 
> > WebCore/storage/IDBCursor.h:41
> >  +  class IDBCursor : public RefCounted<IDBCursor> {
> > The interfaces we've been doing threadsafe ref counted since they could be used on a worker or the main thread once we add it to workers.  (If I were doing it again, I would have waited until we actually needed it to be threadsafe to do this, but it's better to be consistent at this point I think.)
> 
> Done.
> 
> > 
> > WebCore/storage/IDBCursorImpl.cpp:29
> >  +  #include "IDBAny.h"
> > put inside guard...here an elsewhere...from now on
> 
> Done.
> 
> > 
> > WebCore/storage/IDBCursorImpl.cpp:68
> >  +  long long IDBCursorImpl::count() const
> > Could just be inline...but doesn't matter much either way.
> count() has been removed.
> 
> > 
> > WebCore/storage/IDBCursorImpl.cpp:74
> >  +  {
> > ASSERT_NOT_REACHED()
> > 
> > + FIXME
> done.
> 
> > 
> > WebCore/storage/IDBCursorImpl.h:57
> >  +      unsigned short m_direction;
> > This should be an enum, right?
> yep, done.
> 
> > 
> > WebCore/storage/IDBCursorImpl.h:60
> >  +      int m_count;
> > long long?
> count has been removed.
> 
> > 
> > WebCore/storage/IDBCursorImpl.h:43
> >  +      IDBCursorImpl(PassRefPtr<IDBObjectStoreImpl>, PassRefPtr<IDBKeyRange>, unsigned short direction, PassRefPtr<IDBKey>, PassRefPtr<SerializedScriptValue>, int count);
> > make private, add ::create method.
> Done.
> 
> > 
> > WebCore/storage/IDBCursorRequest.cpp:62
> >  +      return m_cursor->count();
> > Could be inline...but I don't care much.
> count has been removed.
> 
> > 
> > WebCore/storage/IDBObjectStoreImpl.cpp:139
> >  +      int count = value ? 1 : 0;
> > we should move count all together since we're not doing sync cursors.
> agreed. removed.
> 
> > 
> > WebCore/storage/IDBObjectStoreImpl.cpp:141
> >  +        value = SerializedScriptValue::create();
> > Aren't we supposed to return a null cursor in this case?
> sounds good. done.
> 
> > 
> > WebCore/storage/IDBObjectStoreImpl.cpp:138
> >  +      RefPtr<SerializedScriptValue> value = m_tree->get(key.get());
> > This isn't right...what if the value doesn't exist but we can find something higher/lower (depending on cursor type) than that?  m_tree will need some new methods I guess.
> yeah, as we talked, this is the bare minimum necessary to exercise the API and the plumbing pipeline, so it works only with a single key.
> continue, update, remove aren't implemented yet, I guess it's better to wait for the new actual storage before implementing them.
> 
> 
> > 
> > WebCore/storage/IDBObjectStoreImpl.cpp:137
> >  +      RefPtr<IDBKey> key = range->left();
> > This doesn't handle direction.
> as above.
> 
> > 
> > WebCore/storage/IDBObjectStoreRequest.cpp:66
> >  +      return request.release();
> > Maybe do in another patch?  (And look at the other files too?)
> I'll split this in a separate patch soon.
> 
> > 
> > WebKit/chromium/public/WebIDBCursor.h:44
> >  +          WEBKIT_ASSERT_NOT_REACHED();
> > Should be easy to implement in this CL.
> as above, not yet implemented.
> 
> > 
> > WebKit/chromium/public/WebIDBKeyRange.h:43
> >  +      WEBKIT_API WebIDBKeyRange(const WebIDBKeyRange& keyRange) { m_private = keyRange.m_private; }
> > This won't work.  You need to make an assign method that does this and just call assign.
> > 
> > Similarly ~ should be inline and call reset().
> > 
> > All methods that aren't inline and aren't virtual should have a WEBKIT_API prefix.
> thanks for the explanation, fixed.
> 
> 
> > 
> > WebKit/chromium/src/IDBCursorProxy.h:56
> >  +      PassOwnPtr<WebKit::WebIDBCursor> m_idbCursor;
> > RefPtr
> OwnPtr, actually :)
> done.

[Marcus Bulach]

Created attachment 63574 [details]
Patch

[Marcus Bulach]

Comment on attachment 63462 [details]
Patch

merged with the latest changes, ready for review.

[Jeremy Orlow]

Comment on attachment 63462 [details]
Patch

WebCore/WebCore.vcproj/WebCore.vcproj:50244
 +          RelativePath="..\storage\IDBCursor.cpp"
intending is wrong

WebCore/storage/IDBCursor.h:48
 +      // Keep in sync with what's in the .idl file.
don't need this comment anymore, right?

WebCore/storage/IDBCursor.idl:31
 +          // Keep in sync with what's in the .h file.
ditto?

WebCore/storage/IDBObjectStoreImpl.cpp:142
 +      callbacks->onSuccess(cursor.release());
Please double check this is what the spec specifies for the case that the key doesn't exist.

WebCore/storage/IDBObjectStoreRequest.h:69
 +      PassRefPtr<IDBRequest> openCursor(ScriptExecutionContext*, PassRefPtr<IDBKeyRange> range = 0, unsigned short direction = IDBCursor::NEXT);
do you have to define the variable name in order to do a default param?  if not, delete "range"

WebKit/chromium/public/WebIDBKeyRange.h:55
 +      WebIDBKeyRange(const WTF::PassRefPtr<WebCore::IDBKeyRange>&);
if you do one, do all three

WebKit/chromium/src/WebIDBCursorImpl.cpp:71
 +  
extra new line


r=me

[Eric Seidel (no email)]

Comment on attachment 63462 [details]
Patch

Cleared Jeremy Orlow's review+ from obsolete attachment 63462 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Marcus Bulach]

Committed r64828: <http://trac.webkit.org/changeset/64828>