Minor IndexedDB refactorings.

third_party/WebKit/Source/modules/indexeddb/IDBObjectStore.h

 /*
  * 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 41 matching lines @@
 class IDBObjectStore final : public GarbageCollectedFinalized<IDBObjectStore>, public ScriptWrappable {
     DEFINE_WRAPPERTYPEINFO();
 public:
     static IDBObjectStore* create(const IDBObjectStoreMetadata& metadata, IDBTransaction* transaction)
     {
         return new IDBObjectStore(metadata, transaction);
     }
     ~IDBObjectStore() { }
     DECLARE_TRACE();
 
+    const IDBObjectStoreMetadata& metadata() const { return m_metadata; }
+    const IDBKeyPath& idbKeyPath() const { return metadata().keyPath; }

[cmumford, 2016/09/19 23:26:48]

IDBObjectStore *is* an "idb" thing, so idbKeyPath is slightly redundant. How
about just "keyPath"?

[pwnall, 2016/09/20 09:11:14]

There's a keyPath() accessor in the WebIDL interface, which does a bit more (on
line 69). I'm slightly worried about having a method with one overload
implementing a method in the spec and the other overload used internally. If you
think there wouldn't be much confusion, I can make the switch.

[cmumford, 2016/09/23 22:21:11]

I'm fine with that - thx for pointing it out.

+
     // Implement the IDBObjectStore IDL
-    int64_t id() const { return m_metadata.id; }
-    const String& name() const { return m_metadata.name; }
+    int64_t id() const { return metadata().id; }
+    const String& name() const { return metadata().name; }
     void setName(const String& name, ExceptionState&);
     ScriptValue keyPath(ScriptState*) const;
     DOMStringList* indexNames() const;
     IDBTransaction* transaction() const { return m_transaction.get(); }
-    bool autoIncrement() const { return m_metadata.autoIncrement; }
+    bool autoIncrement() const { return metadata().autoIncrement; }

[cmumford, 2016/09/19 23:26:48]

What is the rationale for switching m_metadata to metadata()?

[pwnall, 2016/09/20 09:11:14]

The idea is that we can switch the metadata representation (e.g., to a
RefPtr<IDBObjectStoreMetadata>, which would share the underlying structure with
the database metadata) without causing as much code churn.

[cmumford, 2016/09/23 22:21:11]

So you'd be switching from a before that is one of:

  m_metadata.autoIncrement;
  metadata().autoIncrement;

to one of:

  m_metadata->autoIncrement;
  metadata()->autoIncrement;

To me it just seems just a bit more complicated, but I don't have strong
feelings on the subject. How about getting dmurph's opinion and pick one way and
go with it.

[pwnall, 2016/09/23 22:33:04]

Sorry I wasn't clear earlier. metadata() would always return const&, so the
calls to it wouldn't need to change.

 
     IDBRequest* openCursor(ScriptState*, const ScriptValue& range, const String& direction, ExceptionState&);
     IDBRequest* openKeyCursor(ScriptState*, const ScriptValue& range, const String& direction, ExceptionState&);
     IDBRequest* get(ScriptState*, const ScriptValue& key, ExceptionState&);
     IDBRequest* getKey(ScriptState*, const ScriptValue& key, ExceptionState&);
     IDBRequest* getAll(ScriptState*, const ScriptValue& range, unsigned long maxCount, ExceptionState&);
     IDBRequest* getAll(ScriptState*, const ScriptValue& range, ExceptionState&);
     IDBRequest* getAllKeys(ScriptState*, const ScriptValue& range, unsigned long maxCount, ExceptionState&);
     IDBRequest* getAllKeys(ScriptState*, const ScriptValue& range, ExceptionState&);
     IDBRequest* add(ScriptState*, const ScriptValue&, const ScriptValue& key, ExceptionState&);
     IDBRequest* put(ScriptState*, const ScriptValue&, const ScriptValue& key, ExceptionState&);
     IDBRequest* deleteFunction(ScriptState*, const ScriptValue& key, ExceptionState&);
     IDBRequest* clear(ScriptState*, ExceptionState&);
 
     IDBIndex* createIndex(ScriptState* scriptState, const String& name, const StringOrStringSequence& keyPath, const IDBIndexParameters& options, ExceptionState& exceptionState)
     {
         return createIndex(scriptState, name, IDBKeyPath(keyPath), options, exceptionState);
     }
     IDBIndex* index(const String& name, ExceptionState&);
     void deleteIndex(const String& name, ExceptionState&);
 
     IDBRequest* count(ScriptState*, const ScriptValue& range, ExceptionState&);
 
     // Used by IDBCursor::update():
     IDBRequest* put(ScriptState*, WebIDBPutMode, IDBAny* source, const ScriptValue&, IDBKey*, ExceptionState&);
 
     // Used internally and by InspectorIndexedDBAgent:
     IDBRequest* openCursor(ScriptState*, IDBKeyRange*, WebIDBCursorDirection, WebIDBTaskType = WebIDBTaskTypeNormal);
 
-    void markDeleted() { m_deleted = true; }
+    void markDeleted();
     bool isDeleted() const { return m_deleted; }
     void abort();
     void transactionFinished();
 
-    const IDBObjectStoreMetadata& metadata() const { return m_metadata; }
     void setMetadata(const IDBObjectStoreMetadata& metadata) { m_metadata = metadata; }
 
-    typedef HeapVector<Member<IDBKey>> IndexKeys;
-
     // Used by IDBIndex::setName:
     bool containsIndex(const String& name) const
     {
         return findIndexId(name) != IDBIndexMetadata::InvalidId;
     }
     void indexRenamed(int64_t indexId, const String& newName);
 
     WebIDBDatabase* backendDB() const;
 
 private:
     IDBObjectStore(const IDBObjectStoreMetadata&, IDBTransaction*);
 
     IDBIndex* createIndex(ScriptState*, const String& name, const IDBKeyPath&, const IDBIndexParameters&, ExceptionState&);
     IDBRequest* put(ScriptState*, WebIDBPutMode, IDBAny* source, const ScriptValue&, const ScriptValue& key, ExceptionState&);
 
     int64_t findIndexId(const String& name) const;
 
     IDBObjectStoreMetadata m_metadata;
     Member<IDBTransaction> m_transaction;
     bool m_deleted = false;
 
-    typedef HeapHashMap<String, Member<IDBIndex>> IDBIndexMap;
+    // Caches the IDBIndex instances returned by the index() method.
+    // The spec requires that an object store's index() returns the same
+    // IDBIndex instance for a specific index, so this cache is necessary
+    // for correctness.
+    //
+    // index() throws for completed/aborted transactions, so this is not used
+    // after a transaction is finished, and can be cleared.
+    using IDBIndexMap = HeapHashMap<String, Member<IDBIndex>>;
     IDBIndexMap m_indexMap;
 
     // Used to mark indexes created in an aborted upgrade transaction as
     // deleted.
     HeapHashSet<Member<IDBIndex>> m_createdIndexes;
 };
 
 } // namespace blink
 
 #endif // IDBObjectStore_h