Bug 41252 – Support for keys and in-memory storage for IndexedDB

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

Support for keys and in-memory storage for IndexedDB

[Jeremy Orlow]

Created attachment 59840 [details]
Patch

[Jeremy Orlow]

Just the IDBKey/IDBKeyTree parts of the other patch.  This takes pretty much all of the meat out of the other.

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/v8/custom/V8IDBKeyCustom.cpp:31:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/custom/V8IDBKeyCustom.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/custom/V8IDBKeyCustom.cpp:34:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/storage/IDBKeyTree.h:70:  get_less is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:71:  set_less is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:72:  get_greater is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:73:  set_greater is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:74:  get_balance_factor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:75:  set_balance_factor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:79:  compare_key_key is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:80:  compare_key_node is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:81:  compare_node_node is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:106:  IDBKeyTree::AVLTreeAbstractor::compare_key_key is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:176:  converted_key is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 14 in 27 files


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

[WebKit Review Bot]

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

[Marcus Bulach]

(In reply to comment #2)
> Just the IDBKey/IDBKeyTree parts of the other patch.  This takes pretty much all of the meat out of the other.


apparently as a non-reviewer I can't comment directly on the patch, let me see how it goes from here.. let me know if you need more context:


 76 private:
 77     IDBKey();
 78     explicit IDBKey(int32_t);
 79     explicit IDBKey(const String&);
 80 
 81     Type m_type;
 82     String m_string;
 83     int32_t m_number;
note:
js/SerializedScriptValue.h stores as a double.
on the embedder side, DictionaryValue explicitly differentiates as "real" and "integer".

having as "number" seems a bit misleading.



 46     // Pointer is into the active tree, so don't use it after mutating the tree.
 47     typedef Vector<RefPtr<ValueType>, 1> ValueVector;
 48     ValueVector* get(PassRefPtr<IDBKey> key);
hmm.. does this need to be a *? does the caller need to modify it? it seems it'd be safer to just copy the vector and return a passrefptr?
or is this needed to check for null, i.e., a key without value?



 106 int IDBKeyTree<ValueType>::AVLTreeAbstractor::compare_key_key(key va, key vb)
 107 {
 108     if (va->type() != va->type())
 109         return va->type() - vb->type();
is a left and b right? wouldn't it be vb - va?


 114     case IDBKey::NumberType:
 115         return va->number() - va->number();
either va - vb, or vb - va..


 124 template <typename ValueType>
 125 typename IDBKeyTree<ValueType>::ValueVector* IDBKeyTree<ValueType>::get(PassRefPtr<IDBKey> key)
this non-const get seems dangerous..


 135 void IDBKeyTree<ValueType>::insert(PassRefPtr<IDBKey> prpKey, PassRefPtr<ValueType> value)
 136 {
 137     RefPtr<IDBKey> key = prpKey;
 138     TreeNode* node = m_tree.search(key.get());
 139     if (!node) {
 140         node = new TreeNode();
does this zero-initialize node? if not, perhaps it should at least for "less, greater and balanceFactor" members.. either here or even better as an explicit ctor for TreeNode..

[Jeremy Orlow]

(In reply to comment #5)
> (In reply to comment #2)
> > Just the IDBKey/IDBKeyTree parts of the other patch.  This takes pretty much all of the meat out of the other.
> 
> 
> apparently as a non-reviewer I can't comment directly on the patch, let me see how it goes from here.. let me know if you need more context:
> 
> 
>  76 private:
>  77     IDBKey();
>  78     explicit IDBKey(int32_t);
>  79     explicit IDBKey(const String&);
>  80 
>  81     Type m_type;
>  82     String m_string;
>  83     int32_t m_number;
> note:
> js/SerializedScriptValue.h stores as a double.
> on the embedder side, DictionaryValue explicitly differentiates as "real" and "integer".
> 
> having as "number" seems a bit misleading.

Well, it's a "number" from the point of view of IndexedDB which actually specs it to be a long long (so I guess I should have included a fixme that int32 is not good enough long term).  I guess I could change it to m_longLong or m_integer, but I'm not sure we gain much.  But I don't really care, so tell me what you think it should be named.


>  46     // Pointer is into the active tree, so don't use it after mutating the tree.
>  47     typedef Vector<RefPtr<ValueType>, 1> ValueVector;
>  48     ValueVector* get(PassRefPtr<IDBKey> key);
> hmm.. does this need to be a *? does the caller need to modify it? it seems it'd be safer to just copy the vector and return a passrefptr?
> or is this needed to check for null, i.e., a key without value?

Sure, it'd be safer, but it'd be _much_ slower.  Not only would it copy, but we'd cycle through a ref increment and decrement for each one.  And we'd do that any time we're getting or setting a value.

Instead of getting bogged down on this, I just removed all the code for handling non-unique keys since it's not used yet and the code was only half written anyway.


>  106 int IDBKeyTree<ValueType>::AVLTreeAbstractor::compare_key_key(key va, key vb)
>  107 {
>  108     if (va->type() != va->type())
>  109         return va->type() - vb->type();
> is a left and b right? wouldn't it be vb - va?

Hm.  I just copied what AVLTreeAbstractorForFreeList had.  Without digging through the code, I'm not sure which is right.  But it also won't matter until we have cursors working.  When we do, we can write a layout test to verify it's doing the right thing in terms of sorting.  I've changed it to vb - va for now.


>  114     case IDBKey::NumberType:
>  115         return va->number() - va->number();
> either va - vb, or vb - va..

Ha...yeah...good call.


>  124 template <typename ValueType>
>  125 typename IDBKeyTree<ValueType>::ValueVector* IDBKeyTree<ValueType>::get(PassRefPtr<IDBKey> key)
> this non-const get seems dangerous..

Changed.


>  135 void IDBKeyTree<ValueType>::insert(PassRefPtr<IDBKey> prpKey, PassRefPtr<ValueType> value)
>  136 {
>  137     RefPtr<IDBKey> key = prpKey;
>  138     TreeNode* node = m_tree.search(key.get());
>  139     if (!node) {
>  140         node = new TreeNode();
> does this zero-initialize node? if not, perhaps it should at least for "less, greater and balanceFactor" members.. either here or even better as an explicit ctor for TreeNode..

Those are all completely owned by AVLTree and it zeros them out first thing when we add the node.

[Jeremy Orlow]

Created attachment 59988 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/v8/custom/V8IDBKeyCustom.cpp:31:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/custom/V8IDBKeyCustom.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/custom/V8IDBKeyCustom.cpp:34:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/storage/IDBKeyTree.h:65:  get_less is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:66:  set_less is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:67:  get_greater is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:68:  set_greater is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:69:  get_balance_factor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:70:  set_balance_factor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:74:  compare_key_key is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:75:  compare_key_node is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:76:  compare_node_node is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:101:  IDBKeyTree::AVLTreeAbstractor::compare_key_key is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:186:  converted_key is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 14 in 27 files


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

[WebKit Review Bot]

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

[Andrei Popescu]

looks good, some comments:

- we should set on one license text to use, right now we have various flavors (see js/IDBBindingUtilities.cpp vs s/JSIDBKeyCustom.cpp)


> storage/IDBKey.h
> // In order of lest to more precident.

Did you really mean "lest"? And what does "precident" mean?

> storage/IDBKeyTree.h
> class IDBKeyTree 

Is 'add()' missing? Add a FIXME?

> get(PassRefPtr<IDBKey> prpKey)

Why is this taking a PassRefPtr? We don't transfer ownership and all we do with it is to transder it to a RefPtr and call get() on it. Can't we just take the raw pointer instead? Same question for the return type.

[Jeremy Orlow]

(In reply to comment #10)
> looks good, some comments:
> 
> - we should set on one license text to use, right now we have various flavors (see js/IDBBindingUtilities.cpp vs s/JSIDBKeyCustom.cpp)
> 
> 
> > storage/IDBKey.h
> > // In order of lest to more precident.
> 
> Did you really mean "lest"? And what does "precident" mean?

Will clean up.

> > storage/IDBKeyTree.h
> > class IDBKeyTree 
> 
> Is 'add()' missing? Add a FIXME?

Will do.

> > get(PassRefPtr<IDBKey> prpKey)
> 
> Why is this taking a PassRefPtr? We don't transfer ownership and all we do with it is to transder it to a RefPtr and call get() on it. Can't we just take the raw pointer instead? Same question for the return type.

I'll try to do that.

[Nate Chapin]

Comment on attachment 59988 [details]
Patch

Couple of very minor nits, but otherwise the bindings look good.


> +
> +#ifndef IDBBindingUtilities_h
> +#define IDBBindingUtilities_h
> +
> +#include "ScriptValue.h"
> +#include <wtf/PassRefPtr.h>

I don't see any reason why ScriptValue.h is needed here?

> +
> +#if ENABLE(INDEXED_DATABASE)
> +
> +namespace WebCore {
> +
> +class IDBKey;
> +
> +PassRefPtr<IDBKey> createIDBKeyFromValue(JSC::ExecState* exec, JSC::JSValue value);

Omit 'exec' and 'value'.


> +#include "PlatformString.h"
> +#include <v8.h>
> +#include <wtf/PassRefPtr.h>

Is PlatformString.h needed?

[Jeremy Orlow]

> (In reply to comment #10)
> > > storage/IDBKeyTree.h
> > > class IDBKeyTree 
> > 
> > Is 'add()' missing? Add a FIXME?

Er...no.  Distinguishing between the spec's add/put is done at a higher level.

[Jeremy Orlow]

(In reply to comment #12)
> (From update of attachment 59988 [details])
> Couple of very minor nits, but otherwise the bindings look good.
> 
> 
> > +
> > +#ifndef IDBBindingUtilities_h
> > +#define IDBBindingUtilities_h
> > +
> > +#include "ScriptValue.h"
> > +#include <wtf/PassRefPtr.h>
> 
> I don't see any reason why ScriptValue.h is needed here?

I need something for the JSC:: stuff below.  What's better?

[Jeremy Orlow]

Created attachment 60207 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/v8/custom/V8IDBKeyCustom.cpp:31:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/bindings/v8/custom/V8IDBKeyCustom.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/bindings/v8/custom/V8IDBKeyCustom.cpp:34:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebCore/storage/IDBKeyTree.h:65:  get_less is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:66:  set_less is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:67:  get_greater is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:68:  set_greater is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:69:  get_balance_factor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:70:  set_balance_factor is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:74:  compare_key_key is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:75:  compare_key_node is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:76:  compare_node_node is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/storage/IDBKeyTree.h:101:  IDBKeyTree::AVLTreeAbstractor::compare_key_key is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:186:  converted_key is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 14 in 27 files


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

[Andrei Popescu]

(In reply to comment #11)
> (In reply to comment #10)
> > looks good, some comments:
> > 
> > - we should set on one license text to use, right now we have various flavors (see js/IDBBindingUtilities.cpp vs s/JSIDBKeyCustom.cpp)
> > 
> > 
> > > storage/IDBKey.h
> > > // In order of lest to more precident.
> > 
> > Did you really mean "lest"? And what does "precident" mean?
> 

I still think that "precident" is not an English word :) Did you mean precedence?  And should the attribute be "highest" instead of "most"?

> 
> > > get(PassRefPtr<IDBKey> prpKey)
> > 
> > Why is this taking a PassRefPtr? We don't transfer ownership and all we do with it is to transder it to a RefPtr and call get() on it. Can't we just take the raw pointer instead? Same question for the return type.
> 
> I'll try to do that.

I looked at the latest patch but the above comment wasn't addressed. Any reason why not?

[Andrei Popescu]

(In reply to comment #17)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > looks good, some comments:
> > > 
> > > - we should set on one license text to use, right now we have various flavors (see js/IDBBindingUtilities.cpp vs s/JSIDBKeyCustom.cpp)
> > > 
> > > 
> > > > storage/IDBKey.h
> > > > // In order of lest to more precident.
> > > 
> > > Did you really mean "lest"? And what does "precident" mean?
> > 
> 
> I still think that "precident" is not an English word :) Did you mean precedence?  And should the attribute be "highest" instead of "most"?
> 
> > 
> > > > get(PassRefPtr<IDBKey> prpKey)
> > > 
> > > Why is this taking a PassRefPtr? We don't transfer ownership and all we do with it is to transder it to a RefPtr and call get() on it. Can't we just take the raw pointer instead? Same question for the return type.
> > 
> > I'll try to do that.
> 
> I looked at the latest patch but the above comment wasn't addressed. Any reason why not?

Ah, I noticed in the other patch that you said 

> "It's probably possible, but probably best for a second patch (this one is already so big).". 

Fair enough, but I still think you should at least change the Tree class interface to use raw pointers. That's a small change and can be done in this patch.

[Jeremy Orlow]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #11)
> > > (In reply to comment #10)
> > > > looks good, some comments:
> > > > 
> > > > - we should set on one license text to use, right now we have various flavors (see js/IDBBindingUtilities.cpp vs s/JSIDBKeyCustom.cpp)
> > > > 
> > > > 
> > > > > storage/IDBKey.h
> > > > > // In order of lest to more precident.
> > > > 
> > > > Did you really mean "lest"? And what does "precident" mean?
> > > 
> > 
> > I still think that "precident" is not an English word :) Did you mean precedence?  And should the attribute be "highest" instead of "most"?

I should have spell checked, but this is what I meant: http://en.wikipedia.org/wiki/Precedent  I don't really care though.  I'll make it highest precedence.

> > 
> > > 
> > > > > get(PassRefPtr<IDBKey> prpKey)
> > > > 
> > > > Why is this taking a PassRefPtr? We don't transfer ownership and all we do with it is to transder it to a RefPtr and call get() on it. Can't we just take the raw pointer instead? Same question for the return type.
> > > 
> > > I'll try to do that.
> > 
> > I looked at the latest patch but the above comment wasn't addressed. Any reason why not?
> 
> Ah, I noticed in the other patch that you said 
> 
> > "It's probably possible, but probably best for a second patch (this one is already so big).". 
> 
> Fair enough, but I still think you should at least change the Tree class interface to use raw pointers. That's a small change and can be done in this patch.

k

[Jeremy Orlow]

(In reply to comment #19)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (In reply to comment #11)
> > > > (In reply to comment #10)
> > > > > looks good, some comments:
> > > > > 
> > > > > - we should set on one license text to use, right now we have various flavors (see js/IDBBindingUtilities.cpp vs s/JSIDBKeyCustom.cpp)
> > > > > 
> > > > > 
> > > > > > storage/IDBKey.h
> > > > > > // In order of lest to more precident.
> > > > > 
> > > > > Did you really mean "lest"? And what does "precident" mean?
> > > > 
> > > 
> > > I still think that "precident" is not an English word :) Did you mean precedence?  And should the attribute be "highest" instead of "most"?
> 
> I should have spell checked, but this is what I meant: http://en.wikipedia.org/wiki/Precedent  I don't really care though.  I'll make it highest precedence.
> 
> > > 
> > > > 
> > > > > > get(PassRefPtr<IDBKey> prpKey)
> > > > > 
> > > > > Why is this taking a PassRefPtr? We don't transfer ownership and all we do with it is to transder it to a RefPtr and call get() on it. Can't we just take the raw pointer instead? Same question for the return type.
> > > > 
> > > > I'll try to do that.
> > > 
> > > I looked at the latest patch but the above comment wasn't addressed. Any reason why not?
> > 
> > Ah, I noticed in the other patch that you said 
> > 
> > > "It's probably possible, but probably best for a second patch (this one is already so big).". 
> > 
> > Fair enough, but I still think you should at least change the Tree class interface to use raw pointers. That's a small change and can be done in this patch.
> 
> k

I've spent some time trying to do the full PassRefPtr -> * change but it has a large cascading effect and it looks like the change will be massive as I predicted.  I will change the IDBKeyTree.h to use *'s and fix that comment upon landing though (or on the next cycle).

Can someone please review 41252 and 41250 today though?  And if it's close, can you give me a r+ so I can land?  I promise I'll do any follow up patches necessary, but most of what's coming up at this point are really just nits and every time I get an r- it means that I have to wait another day before the next round.

[Jeremy Orlow]

Adding another potential reviewer.

[WebKit Review Bot]

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

[Dumitru Daniliuc]

Comment on attachment 60207 [details]
Patch

WebCore/bindings/js/IDBBindingUtilities.cpp:29
 +  #include "IDBKey.h"
should probably be inside #if ENABLE(INDEXED_DATABASE)

WebCore/bindings/js/IDBBindingUtilities.h:30
 +  #include <wtf/PassRefPtr.h>
move inside #if ENABLE(INDEXED_DATABASE). also, i think it's better to use #include <wtf/Forward.h> for PassRefPtr, RefPtr, OwnPtr, etc. in header files.

WebCore/bindings/v8/IDBBindingUtilities.cpp:30
 +  #include "V8Binding.h"
move these 2 #includes inside #if ENABLE(INDEXED_DATABASE)

WebCore/bindings/v8/IDBBindingUtilities.h:30
 +  #include <wtf/PassRefPtr.h>
move inside ENABLE(INDEXED_DATABASE) + <wtf/Forward.h>

WebCore/storage/IDBKey.cpp:29
 +  #include "SerializedScriptValue.h"
move inside ENABLE(INDEXED_DB)

WebCore/storage/IDBKey.cpp:40
 +  IDBKey::IDBKey(int32_t number)
are types like int32_t allowed in webcore? maybe use 'unsigned long' instead?

WebCore/storage/IDBKey.h:32
 +  #include <wtf/RefPtr.h>
move inside ENABLE(INDEXED_DB)
WebCore/storage/IDBKey.h:83
 +      int32_t m_number;
int32_t --> unsigned long?

WebCore/storage/IDBKey.idl:32
 +          // This space is intentionally left blank.
maybe a short explanation why it's blank? or FIXME if you'll add something here later?

WebCore/storage/IDBKeyTree.h:31
 +  #include <wtf/Vector.h>
move inside ENABLE(INDEXED_DB)

WebCore/storage/IDBKeyTree.h:46
 +      PassRefPtr<ValueType> get(PassRefPtr<IDBKey> key);
do we really want PassRefPtr<> as an argument here? since it's a getter, it seems like the caller might want to hold on to 'key'.

WebCore/storage/IDBKeyTree.h:63
 +          typedef IDBKey* key;
s/handle/Handle/, s/size/Size/, s/key/Key/ ?

WebCore/storage/IDBKeyTree.h:96
 +          delete *iter;
should m_tree rather be a tree of RefPtr<> or some other "smart" pointer type that would automatically destroy the object? then your destructor could look have one single statement: m_tree.purge().

WebCore/storage/IDBKeyTree.h:130
 +  
remove one empty line.

WebCore/storage/IDBKeyTree.h:150
 +          delete node;
can the manual deletion be avoided?

[Dumitru Daniliuc]

Comment on attachment 60207 [details]
Patch

r=me. it's new code that isn't used by anybody yet and there's probably no need to keep you blocked on minor comments. however, please address as many comments as possible before landing, and please upload a follow up patch if some things are left unaddressed.

[Jeremy Orlow]

I believe everything except the comment update and changing some pass ref ptr's to * in IDBKeyTree.h are in.  And that I plan to fix on landing.

Thanks!

[Jeremy Orlow]

(In reply to comment #23)
> int32_t --> unsigned long?

The bindings give us a 32 bit int, so I think it's probably best to just leave it as is.

> WebCore/storage/IDBKey.idl:32
>  +          // This space is intentionally left blank.
> maybe a short explanation why it's blank? or FIXME if you'll add something here later?

The implementation is complete.  And it doesn't have any methods.  I guess I could put a comment, but I don't see what good it'd do because if someone wanted to modify this IDL they'd really need to understand all of the code and it'd be really easy for the comment to go stale.

> WebCore/storage/IDBKeyTree.h:46
>  +      PassRefPtr<ValueType> get(PassRefPtr<IDBKey> key);
> do we really want PassRefPtr<> as an argument here? since it's a getter, it seems like the caller might want to hold on to 'key'.

This is what Andrei wanted me to do as well.  :-)

> WebCore/storage/IDBKeyTree.h:63
>  +          typedef IDBKey* key;
> s/handle/Handle/, s/size/Size/, s/key/Key/ ?

This is defined by the AVLTree class which, for some reason, doesn't follow WebKit's style.

> WebCore/storage/IDBKeyTree.h:96
>  +          delete *iter;
> should m_tree rather be a tree of RefPtr<> or some other "smart" pointer type that would automatically destroy the object? then your destructor could look have one single statement: m_tree.purge().

Maybe we can try to do this in the future but it definitely wouldn't be easy and/or would cause a lot of RefPtr thrashing.  My feeling is that the allocation and deallocation logic is pretty clearly paired and the objects being allocated aren't passed outside of the AVLTree, so it should be OK.  (Normally I'm against any sort of manual memory allocation.)

[Jeremy Orlow]

this was landed...if we need to do anything more, lets do it in 41250