[webkit-reviews] review denied: [Bug 62284] IndexedDB: implement compound (array) key support : [Attachment 113551] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Joshua Bell <jsbell at chromium.org>'s
request for review:
Bug 62284: IndexedDB: implement compound (array) key support
https://bugs.webkit.org/show_bug.cgi?id=62284

Attachment 113551: Patch
https://bugs.webkit.org/attachment.cgi?id=113551&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113551&action=review


> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:59
> +    PassRefPtr<IDBKey> createInternal(v8::Handle<v8::Value> value)
> +    {
> +	   if (value->IsNumber() && !isnan(value->NumberValue()))

What's the benefit of having a KeyCreator class?  Couldn't it just be a static
function?

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:74
> +	       for (uint32_t i = 0; i < length; i++) {

Nit: ++i

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:91
> +    Vector<v8::Handle<v8::Array> > m_stack;

Why is this a member variable?

> Source/WebCore/bindings/v8/custom/V8IDBKeyCustom.cpp:63
> +    case IDBKey::ArrayType:
> +	   array = v8::Array::New(key->array().size());
> +	   for (size_t i = 0; i < key->array().size(); ++i)
> +	       array->Set(i, toV8(key->array()[i].get()));
> +	   return array;

Nit: I would declare array in the case block.  You'll need to use {} to scope
the variable declartion.

> Source/WebCore/storage/IDBLevelDBCoding.cpp:305
> +int compareEncodedStringsWithLength(const char*& p, const char* limitP,
const char*& q, const char* limitQ)

The pass by reference here seems unnecessary.  It looks like the changed values
are only used in the unit test.  Can you instead ASSERT in this function that p
+ lenP*2 == limitP?

> Source/WebCore/storage/IDBLevelDBCoding.cpp:365
> +    size_t prevSize = into.size();
> +    size_t len;

Nit: Avoid abbreviations for variable names.  prevSize -> previousSize and len
-> length.

> Source/WebCore/storage/IDBLevelDBCoding.cpp:379
> +    case IDBKey::ArrayType:
> +	   into.append(encodeByte(kIDBKeyArrayTypeByte));
> +	   len = key.array().size();
> +	   into.append(encodeVarInt(len));
> +	   for (size_t i = 0; i < len; ++i)
> +	       encodeIDBKey(*key.array()[i], into);
> +	   ASSERT_UNUSED(prevSize, into.size() > prevSize);
> +	   return;

I would also put this in {} and scope the declaration of length.

> Source/WebCore/storage/IDBLevelDBCoding.cpp:410
> +    int64_t len;

Nit: len -> length

> Source/WebCore/storage/IDBLevelDBCoding.cpp:420
> +    case kIDBKeyArrayTypeByte:

{} and scoped variables

> Source/WebCore/storage/IDBLevelDBCoding.cpp:421
> +	   // Array.

I would remove this comment.

> Source/WebCore/storage/IDBLevelDBCoding.cpp:471
> +    int64_t len;

len -> length

> Source/WebCore/storage/IDBLevelDBCoding.cpp:480
> +    case kIDBKeyArrayTypeByte:
> +	   // Array.

Same comments as above.

> Source/WebCore/storage/IDBLevelDBCoding.cpp:498
>      case kIDBKeyStringTypeByte:
>	   // String.
> -	   p = decodeVarInt(p, limit, stringLen);
> +	   p = decodeVarInt(p, limit, len);

I would scope stringLength and get rid of the comment.

> Source/WebCore/storage/IDBLevelDBCoding.cpp:534
> +    default:

No default: on switches so the compiler will give an error if you forget a
case.  You'll need to move the ASSERT_NOT_REACHED and return InvalidType
outside the switch to make some versions of gcc happy.

> Source/WebCore/storage/IDBLevelDBCoding.cpp:547
> +    int64_t lenA, lenB;

lenA -> lengthA, lenB -> lengthB.

> Source/WebCore/storage/IDBLevelDBCoding.cpp:558
> +    case kIDBKeyArrayTypeByte:
> +	   // Array type.

Scoped local variables, no comment.

> Source/WebCore/storage/IDBLevelDBCoding.cpp:566
> +	       if (int cmp = compareEncodedIDBKeys(p, limitA, q, limitB))

Is it possible to nest arrays in arrays?  If so, is there a limit?  Otherwise I
think you can overflow the stack (too much recursion) if you nest lots of
arrays.

> Source/WebKit/chromium/src/WebIDBKey.cpp:116
> +	   default:
> +	       ASSERT_NOT_REACHED();
> +	       break;

No default:

> Source/WebKit/chromium/src/WebIDBKey.cpp:144
> +	   default:
> +	       ASSERT_NOT_REACHED();
> +	       break;

No default: