Bug 164097 – IndexedDB 2.0: Support IDBObjectStore getAll/getAllKeys

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

IndexedDB 2.0: Support IDBObjectStore getAll/getAllKeys

[Brady Eidson]

<rdar://problem/28806934>

[Brady Eidson]

Created attachment 293085 [details]
WIP patch

Attaching WIP patch.

Not for style check, EWS, or review, but rather just so I can get to it later from outside the office.

[Brady Eidson]

This patch builds and gets *parts* of W3C tests passing, like imported/w3c/web-platform-tests/IndexedDB/idbobjectstore_getAllKeys.html

[Brady Eidson]

Created attachment 293239 [details]
New WIP

This is almost complete. It works great in DRT. Still exploring a final WKTR issue.

Not for review/EWS - Just so I can get to it from a different machine later.

[Brady Eidson]

Created attachment 293334 [details]
Patch

[Brady Eidson]

Created attachment 293349 [details]
Patch

[Brady Eidson]

GTK/EFL still failing build with:
DerivedSources/WebKit2/WebIDBConnectionToClientMessageReceiver.cpp:37:42: fatal error: WebCore/IDBGetAllRecordsData.h: No such file or directory

But...  I don't know why. That header exists in a path that's already included. I'm not sure what addition steps are required to get it to be picked up?

[Brady Eidson]

(In reply to comment #7)
> GTK/EFL still failing build with:
> DerivedSources/WebKit2/WebIDBConnectionToClientMessageReceiver.cpp:37:42:
> fatal error: WebCore/IDBGetAllRecordsData.h: No such file or directory
> 
> But...  I don't know why. That header exists in a path that's already
> included. I'm not sure what addition steps are required to get it to be
> picked up?

Regardless, patch is ready to review. I'll hope some linux folks take a look at the errors before I need to land.

[Alex Christensen]

Comment on attachment 293349 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBGetAllResult.cpp:63
> +    default:

Could we get rid of the defaults in these switch statements?  It seems unlikely that we'll ever add another GetAllType, but I think it's better to not have a default if you've covered all the cases.
Also, this is inconsistent in this patch; sometimes you use switch with unnecessary default, sometimes just one if statement.

> Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:79
> +    bool m_isValid { false };
> +    IndexedDB::GetAllType m_type;
> +    std::unique_ptr<Vector<IDBKeyData>> m_keyVector;
> +    std::unique_ptr<Vector<IDBValue>> m_valueVector;

Is there a particular reason you need to save a tiny bit of memory at the cost of dereferencing more pointers and making memory management more complicated?  I think you should use Vector<> instead of std::unique_ptr<Vector>.  Then you could just use the Vector copy constructor above.  !m_isValid and empty Vectors make sense.  We don't have to have nullptrs in the way.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h:72
> +typedef std::function<void(const IDBError&, const IDBGetAllResult&)> GetAllResultsCallback;

WTF::Function?

[Brady Eidson]

(In reply to comment #9)
> Comment on attachment 293349 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293349&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBGetAllResult.cpp:63
> > +    default:
> 
> Could we get rid of the defaults in these switch statements?  
Sure

> 
> > Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:79
> > +    bool m_isValid { false };
> > +    IndexedDB::GetAllType m_type;
> > +    std::unique_ptr<Vector<IDBKeyData>> m_keyVector;
> > +    std::unique_ptr<Vector<IDBValue>> m_valueVector;
> 
> Is there a particular reason you need to save a tiny bit of memory at the
> cost of dereferencing more pointers and making memory management more
> complicated?  

It's not about saving memory - It's paranoia programming, allowing verification that the appropriate GetAllType is matched up with the containing Vector.

It could also have been written, for example, as an Optional<Vector<IDBValue>>, etc.

Same pattern applied to IDBResultData, e.g.

> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h:72
> > +typedef std::function<void(const IDBError&, const IDBGetAllResult&)> GetAllResultsCallback;
> 
> WTF::Function?

Ideally, but probably not in this patch as it's matching existing style for the class.

Should be overhauled in one shot instead of adding more unrelated changes to this patch.

[Brady Eidson]

Created attachment 293424 [details]
Patch

[Darin Adler]

Comment on attachment 293424 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:43
> +    IDBGetAllResult(const IDBGetAllResult&);
> +    IDBGetAllResult& operator=(const IDBGetAllResult&);

Can we implement just move constructor and assignment operator instead of also having to implement copy construct.

> Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:56
> +        , m_type(type)
> +    {
> +        switch (m_type) {
> +        case IndexedDB::GetAllType::Keys:
> +            m_keyVector = std::make_unique<Vector<IDBKeyData>>();
> +            break;
> +        case IndexedDB::GetAllType::Values:
> +            m_valueVector = std::make_unique<Vector<IDBValue>>();
> +            break;
> +        }

This looks like a job for WTF::Variant, rather than three separate data members.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:344
> +    auto* exec = context->execState();

Lets use the word "state" rather than "exec".

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:351
> +    m_scriptResult = { context->vm(), toJS(exec, jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()), keyDatas) };

I don’t understand why this idiom is correct and safe. Why is it right to use the outermost execution state of the document in cases like this, rather than the current execution state?

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:746
> +    RELEASE_ASSERT(scriptExecutionContext());

What’s the point of this? Null dereferences will rapidly cause a crash, although not right at this spot. Is there something particularly sensitive here?

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:747
> +    ASSERT_UNUSED(execState, scriptExecutionContext() == scriptExecutionContextFromExecState(&execState));

I suggest just "state" as the name for this argument. The class name is still visible above in case there is some sense of ambiguity.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:749
> +    Ref<IDBRequest> request = IDBRequest::create(*scriptExecutionContext(), objectStore, *this);

I suggest auto for this rather than repeating the class name.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:752
> +    IDBGetAllRecordsData getAllRecordsData = { keyRangeData, getAllType, count, objectStore.info().identifier(), 0 };

Should omit the "=" sign here.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:754
> +    auto operation = IDBClient::createTransactionOperation(*this, request.get(), &IDBTransaction::didGetAllRecordsOnServer, &IDBTransaction::getAllRecordsOnServer, getAllRecordsData);
> +    scheduleOperation(WTFMove(operation));

I think this should be a one-liner. The line would just be a couple characters longer than the first line of code.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:194
> +    const IDBRequestData requestData(operation);

I suggest the { } syntax rather than the () syntax.

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:376
> +        return IDBError(IDBDatabaseException::UnknownError, ASCIILiteral("No backing store transaction found to get all records"));

I suggest the { } syntax rather than the () syntax.

> Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:380
> +        return IDBError(IDBDatabaseException::UnknownError, ASCIILiteral("No backing store object store found"));

Ditto.

> Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:411
> +void MemoryObjectStore::getAllRecords(const IDBKeyRangeData& keyRangeData, Optional<uint32_t> count, IndexedDB::GetAllType type, IDBGetAllResult& result) const

I find the use of uint32_t here puzzling. Throughout the project we use "unsigned" for this type, and "unsigned long" in IDL files. Should we really be starting to use uint32_t in certain cases? If so, what distinguishes those cases from the others?

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1897
> +    RefPtr<SharedBuffer> lowerBuffer = serializeIDBKeyData(key);

I suggest auto here.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1906
> +    RefPtr<SharedBuffer> upperBuffer = serializeIDBKeyData(key);

I suggest auto here.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:1948
> +    static NeverDestroyed<const ASCIILiteral> lowerOpenUpperOpenKey("SELECT key FROM Records WHERE objectStoreID = ? AND key > CAST(? AS TEXT) AND key < CAST(? AS TEXT) ORDER BY key;");
> +    static NeverDestroyed<const ASCIILiteral> lowerOpenUpperClosedKey("SELECT key FROM Records WHERE objectStoreID = ? AND key > CAST(? AS TEXT) AND key <= CAST(? AS TEXT) ORDER BY key;");
> +    static NeverDestroyed<const ASCIILiteral> lowerClosedUpperOpenKey("SELECT key FROM Records WHERE objectStoreID = ? AND key >= CAST(? AS TEXT) AND key < CAST(? AS TEXT) ORDER BY key;");
> +    static NeverDestroyed<const ASCIILiteral> lowerClosedUpperClosedKey("SELECT key FROM Records WHERE objectStoreID = ? AND key >= CAST(? AS TEXT) AND key <= CAST(? AS TEXT) ORDER BY key;");
> +    static NeverDestroyed<const ASCIILiteral> lowerOpenUpperOpenValue("SELECT value, ROWID FROM Records WHERE objectStoreID = ? AND key > CAST(? AS TEXT) AND key < CAST(? AS TEXT) ORDER BY key;");
> +    static NeverDestroyed<const ASCIILiteral> lowerOpenUpperClosedValue("SELECT value, ROWID FROM Records WHERE objectStoreID = ? AND key > CAST(? AS TEXT) AND key <= CAST(? AS TEXT) ORDER BY key;");
> +    static NeverDestroyed<const ASCIILiteral> lowerClosedUpperOpenValue("SELECT value, ROWID FROM Records WHERE objectStoreID = ? AND key >= CAST(? AS TEXT) AND key < CAST(? AS TEXT) ORDER BY key;");
> +    static NeverDestroyed<const ASCIILiteral> lowerClosedUpperClosedValue("SELECT value, ROWID FROM Records WHERE objectStoreID = ? AND key >= CAST(? AS TEXT) AND key <= CAST(? AS TEXT) ORDER BY key;");
> +
> +    const ASCIILiteral* query = nullptr;
> +
> +    if (getAllRecordsData.getAllType == IndexedDB::GetAllType::Keys) {
> +        if (getAllRecordsData.keyRangeData.lowerOpen) {
> +            if (getAllRecordsData.keyRangeData.upperOpen)
> +                query = &lowerOpenUpperOpenKey.get();
> +            else
> +                query = &lowerOpenUpperClosedKey.get();
> +        } else {
> +            if (getAllRecordsData.keyRangeData.upperOpen)
> +                query = &lowerClosedUpperOpenKey.get();
> +            else
> +                query = &lowerClosedUpperClosedKey.get();
> +        }
> +    } else {
> +        if (getAllRecordsData.keyRangeData.lowerOpen) {
> +            if (getAllRecordsData.keyRangeData.upperOpen)
> +                query = &lowerOpenUpperOpenValue.get();
> +            else
> +                query = &lowerOpenUpperClosedValue.get();
> +        } else {
> +            if (getAllRecordsData.keyRangeData.upperOpen)
> +                query = &lowerClosedUpperOpenValue.get();
> +            else
> +                query = &lowerClosedUpperClosedValue.get();
> +        }
> +    }
> +    ASSERT(query);

If we used a separate helper function for this, then we wouldn’t need the strange pointer type, nor the assertion. We could use return statements instead of assignment.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseTransaction.cpp:266
> +    RefPtr<UniqueIDBDatabaseTransaction> protectedThis(this);

This idiom should use Ref, not RefPtr.

> Source/WebCore/Modules/indexeddb/shared/IDBResultData.cpp:185
> +    IDBResultData result(IDBResultType::GetAllRecordsSuccess, requestIdentifier);

I suggest using the { } syntax rather than the () syntax.

> Source/WebCore/Modules/indexeddb/shared/IDBResultData.h:222
> +        std::unique_ptr<IDBGetAllResult> object = std::make_unique<IDBGetAllResult>();

I suggest using auto here.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:304
> +static JSValue deserializeIDBValueToJSValue(ExecState& exec, JSC::JSGlobalObject* globalObject, const IDBValue& value)

I suggest using the name state here instead of exec, and a reference for JSGlobalObject, which cannot be null.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:326
> +JSValue deserializeIDBValueToJSValue(ExecState& exec, const IDBValue& value)

I suggest using the name state here instead of exec.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:331
> +JSC::JSValue toJS(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const IDBValue& value)

I suggest using the name state here instead of exec.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:348
> +JSC::JSValue toJS(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, const IDBKeyData& keyData)

I suggest using the name state here instead of exec.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:354
> +    RefPtr<IDBKey> key = keyData.maybeCreateIDBKey();
> +    return toJS(*exec, *globalObject, key.get());

I suggest a one-liner for this instead of two lines.

> Source/WebCore/bindings/js/IDBBindingUtilities.h:33
> +#include <wtf/Vector.h>

I don’t understand why this had to be added. Nothing below is using Vector for the first time.

[Brady Eidson]

(In reply to comment #12)

Things I don't reply to I just took care of without further discussion.

As always, thanks for the thorough review!!!

> > Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:56
> > +        , m_type(type)
> > +    {
> > +        switch (m_type) {
> > +        case IndexedDB::GetAllType::Keys:
> > +            m_keyVector = std::make_unique<Vector<IDBKeyData>>();
> > +            break;
> > +        case IndexedDB::GetAllType::Values:
> > +            m_valueVector = std::make_unique<Vector<IDBValue>>();
> > +            break;
> > +        }
> 
> This looks like a job for WTF::Variant, rather than three separate data
> members.

Holy hell, yup.

Haven't had a chance to use Variant yet, it was the obvious call here.

> > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:351
> > +    m_scriptResult = { context->vm(), toJS(exec, jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()), keyDatas) };
> 
> I don’t understand why this idiom is correct and safe. Why is it right to
> use the outermost execution state of the document in cases like this, rather
> than the current execution state?

This is a pre-existing idiom, so I can't be making anything worse by perpetuating it in this patch.

As for why it exists; There *is* no current execution state. This is what is happening asynchronously as a result of a database operation completing at a future time.

The result has to be a script object, so we have to use some exec state. The exec state of the IDBRequest object does seem to make sense since it was the current exec state at the time the IDBRequest was created.

I think this is fine, but would love anybody to point out why it's not!

> > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:746
> > +    RELEASE_ASSERT(scriptExecutionContext());
> 
> What’s the point of this? Null dereferences will rapidly cause a crash,
> although not right at this spot. Is there something particularly sensitive
> here?

I'm going to leave this one here in this patch because it matches style with the rest of the file.

I'll do a followup later today that gets rid of them all at once.

> > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:749
> > +    Ref<IDBRequest> request = IDBRequest::create(*scriptExecutionContext(), objectStore, *this);
> 
> I suggest auto for this rather than repeating the class name.

Ditto.

> > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:754
> > +    auto operation = IDBClient::createTransactionOperation(*this, request.get(), &IDBTransaction::didGetAllRecordsOnServer, &IDBTransaction::getAllRecordsOnServer, getAllRecordsData);
> > +    scheduleOperation(WTFMove(operation));
> 
> I think this should be a one-liner. The line would just be a couple
> characters longer than the first line of code.

Ditto.

> > Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:194
> > +    const IDBRequestData requestData(operation);
> 
> I suggest the { } syntax rather than the () syntax.

Ditto.

> > Source/WebCore/Modules/indexeddb/server/MemoryIDBBackingStore.cpp:376
> > +        return IDBError(IDBDatabaseException::UnknownError, ASCIILiteral("No backing store transaction found to get all records"));
> 
> I suggest the { } syntax rather than the () syntax.

Ditto.

> > Source/WebCore/Modules/indexeddb/server/MemoryObjectStore.cpp:411
> > +void MemoryObjectStore::getAllRecords(const IDBKeyRangeData& keyRangeData, Optional<uint32_t> count, IndexedDB::GetAllType type, IDBGetAllResult& result) const
> 
> I find the use of uint32_t here puzzling. Throughout the project we use
> "unsigned" for this type, and "unsigned long" in IDL files. Should we really
> be starting to use uint32_t in certain cases? If so, what distinguishes
> those cases from the others?

I'm not one of the IDL warriors that have gotten intimately familiar with our IDL layers lately, so I'm not sure I have an authoritative answer to this.

What I do know is that:
- IDL specifies "unsigned long" as being an integer between [0, 4294967295]
- [0, 4294967295] is also the range of uint32_t, and also "unsigned long" on 32-bit.
- We build for both 32-bit and 64-bit
- In native code building for 64-bit "unsigned long" has a range of [0, 18446744073709551615]

That's why I've used this pattern in IDB code.


> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseTransaction.cpp:266
> > +    RefPtr<UniqueIDBDatabaseTransaction> protectedThis(this);
> 
> This idiom should use Ref, not RefPtr.

This change isn't possible to make yet because the functions saved off must be copyable (and capturing a Ref<> prevents that)

I will overhaul in a followup to allow that.

> > Source/WebCore/Modules/indexeddb/shared/IDBResultData.cpp:185
> > +    IDBResultData result(IDBResultType::GetAllRecordsSuccess, requestIdentifier);
> 
> I suggest using the { } syntax rather than the () syntax.

I'm going to leave this one here in this patch because it matches style with the rest of the file.

I'll do a followup later today that gets rid of them all at once.

> > Source/WebCore/Modules/indexeddb/shared/IDBResultData.h:222
> > +        std::unique_ptr<IDBGetAllResult> object = std::make_unique<IDBGetAllResult>();
> 
> I suggest using auto here.

I'm going to leave this one here in this patch because it matches style with the rest of the file.

I'll do a followup later today that gets rid of them all at once.

> > Source/WebCore/bindings/js/IDBBindingUtilities.h:33
> > +#include <wtf/Vector.h>
> 
> I don’t understand why this had to be added. Nothing below is using Vector
> for the first time.

Remnants from previous version of the patch that did require it.

[Brady Eidson]

(In reply to comment #13)
> (In reply to comment #12)
> 
> > > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:351
> > > +    m_scriptResult = { context->vm(), toJS(exec, jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()), keyDatas) };
> > 
> > I don’t understand why this idiom is correct and safe. Why is it right to
> > use the outermost execution state of the document in cases like this, rather
> > than the current execution state?
> 
> This is a pre-existing idiom, so I can't be making anything worse by
> perpetuating it in this patch.
> 
> As for why it exists; There *is* no current execution state. This is what is
> happening asynchronously as a result of a database operation completing at a
> future time.
> 
> The result has to be a script object, so we have to use some exec state. The
> exec state of the IDBRequest object does seem to make sense since it was the
> current exec state at the time the IDBRequest was created.

I also meant to add: It is the exec state that will be used when the success/error events fire at the IDBRequest object.

[Darin Adler]

(In reply to comment #13)
\> What I do know is that:
> - IDL specifies "unsigned long" as being an integer between [0, 4294967295]
> - [0, 4294967295] is also the range of uint32_t, and also "unsigned long" on
> 32-bit.
> - We build for both 32-bit and 64-bit
> - In native code building for 64-bit "unsigned long" has a range of [0,
> 18446744073709551615]

Yes, this is why we use "unsigned" and "int" in C++ when IDL files specify "unsigned long" and "long" in the WebKit project.

There’s nothing obvious wrong with using uint32_t and int32_t instead, except that it’s strange to start doing it with new IDB code and not do it for other new code, and there are possibly some small subtle downsides to using the sized types.

I would like us to all be moving in the same direction. If our future idea is to use uint32_t and int32_t in C++ when the IDL file says unsigned long and long, that’s OK with me, but I’d prefer to not have it be different for IDB and other parts of WebKit.

[Brady Eidson]

Created attachment 293451 [details]
Patch

[Darin Adler]

Comment on attachment 293451 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:42
> +    IDBGetAllResult()
> +    {
> +    }

This leaves m_type uninitialized.

> Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:44
> +    IDBGetAllResult(IndexedDB::GetAllType type)

Should be marked explicit.

> Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:75
> +    bool m_isValid { false };
> +    IndexedDB::GetAllType m_type;
> +    WTF::Variant<Vector<IDBKeyData>, Vector<IDBValue>> m_results;

I suggest we do this with a single data member:

    WTF::Optional<WTF::Variant<Vector<IDBKeyData>, Vector<IDBValue>> m_results;

Then the type() function would read like this:

    ASSERT(m_result);
    return holds_alternative<Vector<IDBKeyData>>(m_result.value()) ? IndexedDB::GetAllType::Keys : IndexedDB::GetAllType::Values;

[Darin Adler]

Comment on attachment 293451 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:75
>> +    WTF::Variant<Vector<IDBKeyData>, Vector<IDBValue>> m_results;
> 
> I suggest we do this with a single data member:
> 
>     WTF::Optional<WTF::Variant<Vector<IDBKeyData>, Vector<IDBValue>> m_results;
> 
> Then the type() function would read like this:
> 
>     ASSERT(m_result);
>     return holds_alternative<Vector<IDBKeyData>>(m_result.value()) ? IndexedDB::GetAllType::Keys : IndexedDB::GetAllType::Values;

Or alternatively a Variant where one of the three types is nullptr_t, which we could use for the invalid state.

[Brady Eidson]

(In reply to comment #17)
> Comment on attachment 293451 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=293451&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:42
> > +    IDBGetAllResult()
> > +    {
> > +    }
> 
> This leaves m_type uninitialized.
> 
> > Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:44
> > +    IDBGetAllResult(IndexedDB::GetAllType type)
> 
> Should be marked explicit.
> 
> > Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:75
> > +    bool m_isValid { false };
> > +    IndexedDB::GetAllType m_type;
> > +    WTF::Variant<Vector<IDBKeyData>, Vector<IDBValue>> m_results;
> 
> I suggest we do this with a single data member:
> 
>     WTF::Optional<WTF::Variant<Vector<IDBKeyData>, Vector<IDBValue>>
> m_results;
> 
> Then the type() function would read like this:
> 
>     ASSERT(m_result);
>     return holds_alternative<Vector<IDBKeyData>>(m_result.value()) ?
> IndexedDB::GetAllType::Keys : IndexedDB::GetAllType::Values;


Ooooo I like this.

[Brady Eidson]

(In reply to comment #19)
> (In reply to comment #17)
> > Comment on attachment 293451 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=293451&action=review
> > 
> > > Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:42
> > > +    IDBGetAllResult()
> > > +    {
> > > +    }
> > 
> > This leaves m_type uninitialized.
> > 
> > > Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:44
> > > +    IDBGetAllResult(IndexedDB::GetAllType type)
> > 
> > Should be marked explicit.
> > 
> > > Source/WebCore/Modules/indexeddb/IDBGetAllResult.h:75
> > > +    bool m_isValid { false };
> > > +    IndexedDB::GetAllType m_type;
> > > +    WTF::Variant<Vector<IDBKeyData>, Vector<IDBValue>> m_results;
> > 
> > I suggest we do this with a single data member:
> > 
> >     WTF::Optional<WTF::Variant<Vector<IDBKeyData>, Vector<IDBValue>>
> > m_results;
> > 
> > Then the type() function would read like this:
> > 
> >     ASSERT(m_result);
> >     return holds_alternative<Vector<IDBKeyData>>(m_result.value()) ?
> > IndexedDB::GetAllType::Keys : IndexedDB::GetAllType::Values;
> 
> 
> Ooooo I like this.

It was cleaner to go with the "including std::nullptr_t" variant.

New patch for landing on its way soon.

[Brady Eidson]

Created attachment 293477 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 293477 [details]:

The commit-queue is continuing to process your patch.

[Build Bot]

Comment on attachment 293477 [details]
Patch

Attachment 293477 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2415819

New failing tests:
svg/wicd/test-rightsizing-b.xhtml

[Build Bot]

Created attachment 293492 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Brady Eidson]

The test failures are not caused by this patch.

[Brady Eidson]

https://trac.webkit.org/changeset/208194