Bug 55443 - Split IDBCursor.value into IDBCursor.primaryKey and IDBCursor.value
Summary: Split IDBCursor.value into IDBCursor.primaryKey and IDBCursor.value
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Jeremy Orlow
URL:
Keywords:
Depends on: 55530
Blocks: 55376
  Show dependency treegraph
 
Reported: 2011-02-28 19:59 PST by Jeremy Orlow
Modified: 2011-03-02 14:46 PST (History)
9 users (show)

See Also:


Attachments
Patch (173.70 KB, patch)
2011-02-28 20:02 PST, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (173.89 KB, patch)
2011-03-01 12:47 PST, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (173.75 KB, patch)
2011-03-01 13:14 PST, Jeremy Orlow
no flags Details | Formatted Diff | Diff
Patch (172.68 KB, patch)
2011-03-01 17:28 PST, Jeremy Orlow
jamesr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Orlow 2011-02-28 19:59:36 PST
Split IDBCursor.value into IDBCursor.primaryKey and IDBCursor.value
Comment 1 Jeremy Orlow 2011-02-28 20:02:23 PST
Created attachment 84181 [details]
Patch
Comment 2 Jeremy Orlow 2011-02-28 20:03:42 PST
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.
Comment 3 Hans Wennborg 2011-03-01 06:06:47 PST
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
Comment 4 Jeremy Orlow 2011-03-01 08:58:40 PST
(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.
Comment 5 Hans Wennborg 2011-03-01 09:01:49 PST
(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
Comment 6 Jeremy Orlow 2011-03-01 12:29:11 PST
Thanks Tony!
Comment 7 Jeremy Orlow 2011-03-01 12:47:48 PST
Created attachment 84277 [details]
Patch
Comment 8 Jeremy Orlow 2011-03-01 12:48:15 PST
Rebased the patch on top of the 2 I just committed.  Nothing materially changed though.
Comment 9 WebKit Review Bot 2011-03-01 13:14:23 PST
Attachment 84277 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8072750
Comment 10 Jeremy Orlow 2011-03-01 13:14:48 PST
Created attachment 84283 [details]
Patch
Comment 11 James Robinson 2011-03-01 16:34:17 PST
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?
Comment 12 Jeremy Orlow 2011-03-01 16:41:13 PST
(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.
Comment 13 James Robinson 2011-03-01 16:52:14 PST
(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.
Comment 14 Jeremy Orlow 2011-03-01 17:28:20 PST
Created attachment 84334 [details]
Patch
Comment 15 Jeremy Orlow 2011-03-01 17:28:58 PST
Fixed all issues.  Removed the bindings work that was in the last one (and did it in 55530).
Comment 16 James Robinson 2011-03-01 17:34:14 PST
Comment on attachment 84334 [details]
Patch

Cool, R=me
Comment 17 WebKit Review Bot 2011-03-01 18:48:50 PST
Attachment 84283 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8073792
Comment 18 Jeremy Orlow 2011-03-02 14:46:51 PST
Committed r80171: <http://trac.webkit.org/changeset/80171>