IndexedDB: Replace IDL operation overloading with ScriptValue handling

Source/modules/indexeddb/IDBObjectStore.cpp

 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
@@ skipping 52 matching lines @@
 PassRefPtr<DOMStringList> IDBObjectStore::indexNames() const
 {
     IDB_TRACE("IDBObjectStore::indexNames");
     RefPtr<DOMStringList> indexNames = DOMStringList::create();
     for (IDBObjectStoreMetadata::IndexMap::const_iterator it = m_metadata.indexes.begin(); it != m_metadata.indexes.end(); ++it)
         indexNames->append(it->value.name);
     indexNames->sort();
     return indexNames.release();
 }
 
-PassRefPtr<IDBRequest> IDBObjectStore::get(ScriptExecutionContext* context, PassRefPtr<IDBKeyRange> keyRange, ExceptionState& es)
+PassRefPtr<IDBRequest> IDBObjectStore::get(ScriptExecutionContext* context, const ScriptValue& key, ExceptionState& es)
 {
     IDB_TRACE("IDBObjectStore::get");
     if (isDeleted()) {
         es.throwDOMException(InvalidStateError, IDBDatabase::objectStoreDeletedErrorMessage);
         return 0;
     }
-    if (!keyRange) {
-        es.throwDOMException(DataError, IDBDatabase::noKeyOrKeyRangeErrorMessage);
-        return 0;
-    }
     if (m_transaction->isFinished()) {
         es.throwDOMException(TransactionInactiveError, IDBDatabase::transactionFinishedErrorMessage);
         return 0;
     }
     if (!m_transaction->isActive()) {
         es.throwDOMException(TransactionInactiveError, IDBDatabase::transactionInactiveErrorMessage);
         return 0;
     }
+    RefPtr<IDBKeyRange> keyRange = IDBKeyRange::fromScriptValue(context, key, es);
+    if (es.hadException())
+        return 0;
+    if (!keyRange) {
+        es.throwDOMException(DataError, IDBDatabase::noKeyOrKeyRangeErrorMessage);
+        return 0;
+    }
+
     RefPtr<IDBRequest> request = IDBRequest::create(context, IDBAny::create(this), m_transaction.get());
     backendDB()->get(m_transaction->id(), id(), IDBIndexMetadata::InvalidId, keyRange, false, request);
     return request.release();
 }
 
-PassRefPtr<IDBRequest> IDBObjectStore::get(ScriptExecutionContext* context, const ScriptValue& key, ExceptionState& es)
-{
-    RefPtr<IDBKeyRange> keyRange = IDBKeyRange::only(context, key, es);
-    if (es.hadException())
-        return 0;
-    return get(context, keyRange.release(), es);
-}
-
 static void generateIndexKeysForValue(DOMRequestState* requestState, const IDBIndexMetadata& indexMetadata, const ScriptValue& objectValue, IDBObjectStore::IndexKeys* indexKeys)
 {
     ASSERT(indexKeys);
     RefPtr<IDBKey> indexKey = createIDBKeyFromScriptValueAndKeyPath(requestState, objectValue, indexMetadata.keyPath);
 
     if (!indexKey)
         return;
 
     if (!indexMetadata.multiEntry || indexKey->type() != IDBKey::ArrayType) {
         if (!indexKey->isValid())
@@ skipping 111 matching lines @@
     }
 
     RefPtr<IDBRequest> request = IDBRequest::create(context, source, m_transaction.get());
     Vector<char> wireBytes;
     serializedValue->toWireBytes(wireBytes);
     RefPtr<SharedBuffer> valueBuffer = SharedBuffer::adoptVector(wireBytes);
     backendDB()->put(m_transaction->id(), id(), valueBuffer, key.release(), static_cast<IDBDatabaseBackendInterface::PutMode>(putMode), request, indexIds, indexKeys);
     return request.release();
 }
 
-PassRefPtr<IDBRequest> IDBObjectStore::deleteFunction(ScriptExecutionContext* context, PassRefPtr<IDBKeyRange> keyRange, ExceptionState& es)
+PassRefPtr<IDBRequest> IDBObjectStore::deleteFunction(ScriptExecutionContext* context, const ScriptValue& key, ExceptionState& es)
 {
     IDB_TRACE("IDBObjectStore::delete");
     if (isDeleted()) {
         es.throwDOMException(InvalidStateError, IDBDatabase::objectStoreDeletedErrorMessage);
         return 0;
     }
     if (m_transaction->isFinished()) {
         es.throwDOMException(TransactionInactiveError, IDBDatabase::transactionFinishedErrorMessage);
         return 0;
     }
     if (!m_transaction->isActive()) {
         es.throwDOMException(TransactionInactiveError, IDBDatabase::transactionInactiveErrorMessage);
         return 0;
     }
     if (m_transaction->isReadOnly()) {
         es.throwDOMException(ReadOnlyError);
         return 0;
     }
+
+    RefPtr<IDBKeyRange> keyRange = IDBKeyRange::fromScriptValue(context, key, es);
+    if (es.hadException())
+        return 0;
     if (!keyRange) {
         es.throwDOMException(DataError, IDBDatabase::noKeyOrKeyRangeErrorMessage);
         return 0;
     }
 
     RefPtr<IDBRequest> request = IDBRequest::create(context, IDBAny::create(this), m_transaction.get());
     backendDB()->deleteRange(m_transaction->id(), id(), keyRange, request);
     return request.release();
 }
 
-PassRefPtr<IDBRequest> IDBObjectStore::deleteFunction(ScriptExecutionContext* context, const ScriptValue& key, ExceptionState& es)
-{
-    RefPtr<IDBKeyRange> keyRange = IDBKeyRange::only(context, key, es);
-    if (es.hadException())
-        return 0;
-    return deleteFunction(context, keyRange.release(), es);
-}
-
 PassRefPtr<IDBRequest> IDBObjectStore::clear(ScriptExecutionContext* context, ExceptionState& es)
 {
     IDB_TRACE("IDBObjectStore::clear");
     if (isDeleted()) {
         es.throwDOMException(InvalidStateError, IDBDatabase::objectStoreDeletedErrorMessage);
         return 0;
     }
     if (m_transaction->isFinished()) {
         es.throwDOMException(TransactionInactiveError, IDBDatabase::transactionFinishedErrorMessage);
         return 0;
@@ skipping 134 matching lines @@
     backendDB()->createIndex(m_transaction->id(), id(), indexId, name, keyPath, unique, multiEntry);
 
     ++m_metadata.maxIndexId;
 
     IDBIndexMetadata metadata(name, indexId, keyPath, unique, multiEntry);
     RefPtr<IDBIndex> index = IDBIndex::create(metadata, this, m_transaction.get());
     m_indexMap.set(name, index);
     m_metadata.indexes.set(indexId, metadata);
     m_transaction->db()->indexCreated(id(), metadata);
 
     ASSERT(!es.hadException());

[jsbell, 2013/08/26 16:06:18]

Not relevant to this patch, but I have no idea what this ASSERT is doing here. I
don't see a path leading to this line where an exception may be thrown without
returning.

     if (es.hadException())
         return 0;
 
-    RefPtr<IDBRequest> indexRequest = openCursor(context, static_cast<IDBKeyRange*>(0), IDBCursor::directionNext(), IDBDatabaseBackendInterface::PreemptiveTask, es);
-    ASSERT(!es.hadException());
-    if (es.hadException())
-        return 0;

[haraken, 2013/08/25 23:51:06]

Why can you remove these checks?

[jsbell, 2013/08/26 16:06:18]

This change switches from calling a script-exposed method to an "internal"
method. The new "internal" openCursor() method is used from all call sites after
the checks, and doesn't take an ExceptionState.

The script-exposed openCursor() checks that the index was in a valid state for
operations (duplicating the checks at the top of this method) and that the
direction string is valid (but now the enum value is passed).

+    RefPtr<IDBRequest> indexRequest = openCursor(context, static_cast<IDBKeyRange*>(0), IndexedDB::CursorNext, IDBDatabaseBackendInterface::PreemptiveTask);
     indexRequest->preventPropagation();
 
     // This is kept alive by being the success handler of the request, which is in turn kept alive by the owning transaction.
     RefPtr<IndexPopulator> indexPopulator = IndexPopulator::create(backendDB(), m_transaction->id(), id(), metadata);
     indexRequest->setOnsuccess(indexPopulator);
 
     return index.release();
 }
 
 PassRefPtr<IDBIndex> IDBObjectStore::index(const String& name, ExceptionState& es)
@@ skipping 62 matching lines @@
 
     m_metadata.indexes.remove(indexId);
     m_transaction->db()->indexDeleted(id(), indexId);
     IDBIndexMap::iterator it = m_indexMap.find(name);
     if (it != m_indexMap.end()) {
         it->value->markDeleted();
         m_indexMap.remove(name);
     }
 }
 
-PassRefPtr<IDBRequest> IDBObjectStore::openCursor(ScriptExecutionContext* context, PassRefPtr<IDBKeyRange> range, const String& directionString, IDBDatabaseBackendInterface::TaskType taskType, ExceptionState& es)
+PassRefPtr<IDBRequest> IDBObjectStore::openCursor(ScriptExecutionContext* context, const ScriptValue& range, const String& directionString, ExceptionState& es)
 {
     IDB_TRACE("IDBObjectStore::openCursor");
     if (isDeleted()) {
         es.throwDOMException(InvalidStateError, IDBDatabase::objectStoreDeletedErrorMessage);
         return 0;
     }
     if (m_transaction->isFinished()) {
         es.throwDOMException(TransactionInactiveError, IDBDatabase::transactionFinishedErrorMessage);
         return 0;
     }
     if (!m_transaction->isActive()) {
         es.throwDOMException(TransactionInactiveError, IDBDatabase::transactionInactiveErrorMessage);
         return 0;
     }
+
     IndexedDB::CursorDirection direction = IDBCursor::stringToDirection(directionString, es);
     if (es.hadException())
         return 0;
 
+    RefPtr<IDBKeyRange> keyRange = IDBKeyRange::fromScriptValue(context, range, es);
+    if (es.hadException())
+        return 0;
+

[haraken, 2013/08/25 23:51:06]

Don't you need to check if(!keyRange) ?

[jsbell, 2013/08/26 16:06:18]

openCursor() can take a null range.

+    return openCursor(context, keyRange, direction, IDBDatabaseBackendInterface::NormalTask);
+}
+
+PassRefPtr<IDBRequest> IDBObjectStore::openCursor(ScriptExecutionContext* context, PassRefPtr<IDBKeyRange> range, IndexedDB::CursorDirection direction, IDBDatabaseBackendInterface::TaskType taskType)
+{
     RefPtr<IDBRequest> request = IDBRequest::create(context, IDBAny::create(this), m_transaction.get());
     request->setCursorDetails(IndexedDB::CursorKeyAndValue, direction);
 
-    backendDB()->openCursor(m_transaction->id(), id(), IDBIndexMetadata::InvalidId, range, direction, false, static_cast<IDBDatabaseBackendInterface::TaskType>(taskType), request);
+    backendDB()->openCursor(m_transaction->id(), id(), IDBIndexMetadata::InvalidId, range, direction, false, taskType, request);
     return request.release();
 }
 
-PassRefPtr<IDBRequest> IDBObjectStore::openCursor(ScriptExecutionContext* context, const ScriptValue& key, const String& direction, ExceptionState& es)
-{
-    RefPtr<IDBKeyRange> keyRange = IDBKeyRange::only(context, key, es);
-    if (es.hadException())
-        return 0;
-    return openCursor(context, keyRange.release(), direction, es);
-}
-
-PassRefPtr<IDBRequest> IDBObjectStore::count(ScriptExecutionContext* context, PassRefPtr<IDBKeyRange> range, ExceptionState& es)
+PassRefPtr<IDBRequest> IDBObjectStore::count(ScriptExecutionContext* context, const ScriptValue& range, ExceptionState& es)
 {
     IDB_TRACE("IDBObjectStore::count");
     if (isDeleted()) {
         es.throwDOMException(InvalidStateError, IDBDatabase::objectStoreDeletedErrorMessage);
         return 0;
     }
     if (m_transaction->isFinished()) {
         es.throwDOMException(TransactionInactiveError, IDBDatabase::transactionFinishedErrorMessage);
         return 0;
     }
     if (!m_transaction->isActive()) {
         es.throwDOMException(TransactionInactiveError, IDBDatabase::transactionInactiveErrorMessage);
         return 0;
     }
+
+    RefPtr<IDBKeyRange> keyRange = IDBKeyRange::fromScriptValue(context, range, es);
+    if (es.hadException())
+        return 0;
+

[haraken, 2013/08/25 23:51:06]

Shall we add ASSERT(keyRange) ?

[jsbell, 2013/08/26 16:06:18]

count() can take a null range

     RefPtr<IDBRequest> request = IDBRequest::create(context, IDBAny::create(this), m_transaction.get());
-    backendDB()->count(m_transaction->id(), id(), IDBIndexMetadata::InvalidId, range, request);
+    backendDB()->count(m_transaction->id(), id(), IDBIndexMetadata::InvalidId, keyRange, request);
     return request.release();
 }
 
-PassRefPtr<IDBRequest> IDBObjectStore::count(ScriptExecutionContext* context, const ScriptValue& key, ExceptionState& es)
-{
-    RefPtr<IDBKeyRange> keyRange = IDBKeyRange::only(context, key, es);
-    if (es.hadException())
-        return 0;
-    return count(context, keyRange.release(), es);
-}
-
 void IDBObjectStore::transactionFinished()
 {
     ASSERT(m_transaction->isFinished());
 
     // Break reference cycles.
     m_indexMap.clear();
 }
 
 int64_t IDBObjectStore::findIndexId(const String& name) const
 {
     for (IDBObjectStoreMetadata::IndexMap::const_iterator it = m_metadata.indexes.begin(); it != m_metadata.indexes.end(); ++it) {
         if (it->value.name == name) {
             ASSERT(it->key != IDBIndexMetadata::InvalidId);
             return it->key;
         }
     }
     return IDBIndexMetadata::InvalidId;
 }
 
 IDBDatabaseBackendInterface* IDBObjectStore::backendDB() const
 {
     return m_transaction->backendDB();
 }
 
 } // namespace WebCore