Summary: | Initial bindings and plumbing for IDBCursor. | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Marcus Bulach <bulach> | ||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, andreip, bulach, dglazkov, fishd, jorlow, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Other | ||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||
Bug Depends on: | 42583 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Marcus Bulach
2010-07-08 12:24:33 PDT
Created attachment 60928 [details]
Patch
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.
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 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". 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.
Created attachment 61392 [details]
Patch
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.
(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. (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..) Attachment 61392 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/3486290 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. (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. > 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. 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. 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
Created attachment 61831 [details]
Patch
(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! (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. Created attachment 62853 [details]
Patch
(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? Attachment 62853 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3576612 Created attachment 62935 [details]
Patch
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
Created attachment 63462 [details]
Patch
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. 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. Created attachment 63574 [details]
Patch
Comment on attachment 63462 [details]
Patch
merged with the latest changes, ready for review.
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
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. Committed r64828: <http://trac.webkit.org/changeset/64828> |