Bug 55443 – Split IDBCursor.value into IDBCursor.primaryKey and IDBCursor.value

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

Split IDBCursor.value into IDBCursor.primaryKey and IDBCursor.value

[Jeremy Orlow]

Created attachment 84181 [details]
Patch

[Jeremy Orlow]

The bulk of the change is just to layout test expectations, don't worry.

This needs to be in for m11...i.e. EOW...and it has a Chromium half too.  So please look at it ASAP.

[Hans Wennborg]

Comment on attachment 84181 [details]
Patch

Very glad we're fixing this. Makes the whole cursor thing easier to thing about.

View in context: https://bugs.webkit.org/attachment.cgi?id=84181&action=review

> Source/WebCore/WebCore.gypi:4067
> +            'storage/IDBCursorWithValue.h',

nit: alpha order

> Source/WebCore/storage/IDBCursorBackendImpl.cpp:171
> +    m_currentPrimaryKey = IDBKey::fromQuery(*m_query, 5);

How does this work for an object store cursor? Is there even a fifth column in the query in that case?

> Source/WebCore/storage/IDBCursorWithValue.idl:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

nit: might as well make it 2011

[Jeremy Orlow]

(In reply to comment #3)
> (From update of attachment 84181 [details])
> Very glad we're fixing this. Makes the whole cursor thing easier to thing about.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=84181&action=review
> 
> > Source/WebCore/WebCore.gypi:4067
> > +            'storage/IDBCursorWithValue.h',
> 
> nit: alpha order

Will do.

> > Source/WebCore/storage/IDBCursorBackendImpl.cpp:171
> > +    m_currentPrimaryKey = IDBKey::fromQuery(*m_query, 5);
> 
> How does this work for an object store cursor? Is there even a fifth column in the query in that case?

Yes, see the changes to Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp
 
> > Source/WebCore/storage/IDBCursorWithValue.idl:2
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
> 
> nit: might as well make it 2011

Will do.

[Hans Wennborg]

(In reply to comment #4)
> > > Source/WebCore/storage/IDBCursorBackendImpl.cpp:171
> > > +    m_currentPrimaryKey = IDBKey::fromQuery(*m_query, 5);
> > 
> > How does this work for an object store cursor? Is there even a fifth column in the query in that case?
> 
> Yes, see the changes to Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp

Ah, I missed that; it all makes sense now :)

lgtm

[Jeremy Orlow]

Thanks Tony!

[Jeremy Orlow]

Created attachment 84277 [details]
Patch

[Jeremy Orlow]

Rebased the patch on top of the 2 I just committed.  Nothing materially changed though.

[WebKit Review Bot]

Attachment 84277 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8072750

[Jeremy Orlow]

Created attachment 84283 [details]
Patch

[James Robinson]

Comment on attachment 84283 [details]
Patch

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?

[Jeremy Orlow]

(In reply to comment #11)
> (From update of attachment 84283 [details])
> 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.

Will add.

> > 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.

In some cases means it's really complicated in what conditions, but if it is used, the call is benign.

Yes, it's not a parameter, but there's no "UNUSED_LOCAL_VARIABLE" macro that I'm aware of.  And it doesn't seem worth adding it.  Do you disagree?

> This also really needs a test.

Will do.
 
> > 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.

removed

> > 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).

removed

> > Source/WebCore/storage/IDBCursorWithValue.h:40
> > +    // value() is defined in parent.
> 
> What does this mean?

value is in the IDL for IDBCursorWithValue.  It's subtle that we actually implement it in the parent, even though it's not exposed in IDL.

> 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

done

> > 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?

It is.  'value' in someType will tell you.

[James Robinson]

(In reply to comment #12)
> > > 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.
> 
> In some cases means it's really complicated in what conditions, but if it is used, the call is benign.
> 
> Yes, it's not a parameter, but there's no "UNUSED_LOCAL_VARIABLE" macro that I'm aware of.  And it doesn't seem worth adding it.  Do you disagree?
> 

If you can tell in the generator that a local variable won't be used, then just don't generate code to produce the local.

[Jeremy Orlow]

Created attachment 84334 [details]
Patch

[Jeremy Orlow]

Fixed all issues.  Removed the bindings work that was in the last one (and did it in 55530).

[James Robinson]

Comment on attachment 84334 [details]
Patch

Cool, R=me

[WebKit Review Bot]

Attachment 84283 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8073792

[Jeremy Orlow]

Committed r80171: <http://trac.webkit.org/changeset/80171>