[webkit-reviews] review denied: [Bug 55443] Split IDBCursor.value into IDBCursor.primaryKey and IDBCursor.value : [Attachment 84283] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Jeremy Orlow
<jorlow at chromium.org>'s request for review:
Bug 55443: Split IDBCursor.value into IDBCursor.primaryKey and IDBCursor.value
https://bugs.webkit.org/show_bug.cgi?id=55443

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=84283&action=review

OK - mostly seems good, although a few bits didn't make sense.	I'd really like
the generator script changes to be tested, though, and preferably be done as a
separate bug+patch to keep things a more manageable size.  The generator
scripts have lots of tricky logic and without explicit tests it's impossible to
verify new behavior.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1833
> -    if (@$attributes) {
> +    if (@$attributes && (@$attributes > 1 ||
$$attributes[0]->signature->type ne "SerializedScriptValue")) {

These changes really need bindings tests - see
TestCallback/TestInterface/etc.idl in Source/WebCore/bindings/scripts/test/ and
add some coverage.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1977
> +    UNUSED_PARAM(defaultSignature); // In some cases, it will not be used.

What does "in some cases" mean?  defaultSignature looks more like a local
variable than a parameter unless I'm misreading the generator.

This also really needs a test.

> Source/WebCore/storage/IDBCursor.h:71
>      explicit IDBCursor(PassRefPtr<IDBCursorBackendInterface>, IDBRequest*,
IDBTransaction*);

nit: explicit is meaningless on constructors that take more than one parameter,
might as well remove it.

> Source/WebCore/storage/IDBCursorWithValue.h:38
> +    virtual ~IDBCursorWithValue();

Since this class has no members, it doesn't really need a destructor (unless
you intend to add members in the future and want to make sure that this isn't
forgotten).

> Source/WebCore/storage/IDBCursorWithValue.h:40
> +    // value() is defined in parent.

What does this mean?

I don't actually get what this class does at all - it seems like all the logic
is still in IDBCursor and this subclass isn't doing anything.  Will it do
something different in the future?

> Source/WebCore/storage/IDBCursorWithValue.h:43
> +    explicit IDBCursorWithValue(PassRefPtr<IDBCursorBackendInterface>,
IDBRequest*, IDBTransaction*);

nit: no explicit

> Source/WebCore/storage/IDBCursorWithValue.idl:31
> +	   readonly attribute SerializedScriptValue value;

I don't understand why this subinterface is needed.  The only difference
between IDBCursorWithValue and IDBCursor is the type of the 'value' attribute,
but that shouldn't be observable from javascript - should it?