[webkit-reviews] review denied: [Bug 83526] [V8] IndexedDB: Cursor value modifications should be preserved until cursor iterates : [Attachment 146974] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Alec Flett
<alecflett at chromium.org>'s request for review:
Bug 83526: [V8] IndexedDB: Cursor value modifications should be preserved until
cursor iterates
https://bugs.webkit.org/show_bug.cgi?id=83526

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

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=146974&action=review


> this isn't for performance, this is for spec compliance

I am sorry! I should have read your ChangeLog carefully...

For now let me mark r- due to missing necessary tests to confirm the change.

> Source/WebCore/ChangeLog:9
> +	   determined by IDBCursor.cpp, to follow spec behavior of keeping a

Link to the spec please.

> Source/WebCore/ChangeLog:12
> +	   Test: storage/indexeddb/cursor-value.html

Would you add the following test cases?

(Case 1)
cursor.value.foo = "bar";
shouldBe('cursor.value.foo', ...);
cursor.advance();
shouldBe('cursor.value.foo', ...);

(Case 2)
cursor.value.foo = "bar";
shouldBe('cursor.value.foo', ...);
gc();
shouldBe('cursor.value.foo', ...);

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:2
> + * Copyright (C) 2011, Google Inc. All rights reserved.

2011 => 2012

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY GOOGLE INC. AND ITS CONTRIBUTORS ``AS IS''
AND

Please do not change the line manually. Please just copy the whole copyright
from bindings/v8/custom/V8TrackEventCustom.cpp, and just change "2011" to
"2012".

> Source/WebCore/bindings/v8/IDBCustomBindings.cpp:50
> +    IDBCursorWithValue* imp = V8IDBCursorWithValue::toNative(info.Holder());

> +    v8::Handle<v8::String> propertyName = v8::String::NewSymbol("value");
> +    if (!imp->valueIsDirty()) {
> +	   v8::Handle<v8::Value> value =
info.Holder()->GetHiddenValue(propertyName);
> +	   if (!value.IsEmpty())
> +	       return value;
> +    }
> +
> +    RefPtr<IDBAny> result = imp->value();
> +    v8::Handle<v8::Value> wrapper = toV8(result.get(), info.GetIsolate());
> +    info.Holder()->SetHiddenValue(propertyName, wrapper);
> +    return wrapper;

Confirmation: The semantics you need is:

- If the value is not yet invalidated, return the same wrapper object keeping
JavaScript properties.
- If the value is already invalidated, create a new wrapper object.

Is my understanding correct? If so, the code looks correct.