Split IDBCursor.value into IDBCursor.primaryKey and IDBCursor.value
Created attachment 84181 [details] Patch
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 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
(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.
(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
Thanks Tony!
Created attachment 84277 [details] Patch
Rebased the patch on top of the 2 I just committed. Nothing materially changed though.
Attachment 84277 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8072750
Created attachment 84283 [details] Patch
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?
(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.
(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.
Created attachment 84334 [details] Patch
Fixed all issues. Removed the bindings work that was in the last one (and did it in 55530).
Comment on attachment 84334 [details] Patch Cool, R=me
Attachment 84283 [details] did not build on chromium: Build output: http://queues.webkit.org/results/8073792
Committed r80171: <http://trac.webkit.org/changeset/80171>