Comment on attachment 774798 [details] [diff] [review] patch Review of attachment 774798 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBDatabase.cpp @@ +236,5 @@ > mRegistered(false), > mClosed(false), > mRunningVersionChange(false) > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); Nit: I would add an empty line here @@ +421,5 @@ > AutoRemoveObjectStore autoRemove(databaseInfo, newInfo->name); > > nsRefPtr<IDBObjectStore> objectStore = > aTransaction->GetOrCreateObjectStore(newInfo->name, newInfo, true); > + if (!objectStore) { NS_ENSURE_TRUE() warns use the new macro ENSURE_SUCCESS here and elsewhere @@ +474,4 @@ > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > + DatabaseInfo* info = Info(); > + return info->version; you can inline this @@ +493,5 @@ > > nsRefPtr<nsDOMStringList> list(new nsDOMStringList()); > uint32_t count = objectStoreNames.Length(); > for (uint32_t index = 0; index < count; index++) { > + if (!list->Add(objectStoreNames[index])) { NS_ENSURE_TRUE() warns add a warning (for now), here and elsewhere @@ +504,5 @@ > } > > +already_AddRefed<IDBObjectStore> > +IDBDatabase::CreateObjectStore(JSContext* aCx, const nsAString& aName, > + const IDBObjectStoreParameters& aOptionalParameters, Nit: 80 chars max, see how it's done elsewhere @@ +523,2 @@ > KeyPath keyPath(0); > + if (NS_FAILED(KeyPath::Parse(aCx, aOptionalParameters.mKeyPath, &keyPath))) { I wonder if we miss "|| !keyPath.IsValid()" here see original IDBObjectStore::CreateIndex(), before it got converted to webidl @@ +547,5 @@ > } > > +void > +IDBDatabase::DeleteObjectStore(const nsAString& aName, > + ErrorResult& aRv) Nit: one line should be enough @@ +599,5 @@ > + ErrorResult& aRv) > +{ > + Sequence<nsString> list; > + list.AppendElement(aStoreName); > + return Transaction(list, aMode, aRv); inline this? @@ +603,5 @@ > + return Transaction(list, aMode, aRv); > +} > + > +already_AddRefed<indexedDB::IDBTransaction> > +IDBDatabase::Transaction(const Sequence<nsString >& aStoreNames, IDBTransactionMode aMode, Nit: 80 chars max, put |IDBTransactionMode aMode,| on the same line as |ErrorResult& aRv| @@ +638,5 @@ > transactionMode = IDBTransaction::READ_WRITE; > + break; > + case IDBTransactionMode::Versionchange: > + transactionMode = IDBTransaction::VERSION_CHANGE; > + break; add |default:| with MOZ_CRASH("Unknown mode!") ? @@ +670,2 @@ > IDBDatabase::MozCreateFileHandle(const nsAString& aName, > + const Optional<nsAString >& aType, Nit: no need for space before ">" @@ +780,5 @@ > + return GetOwner(); > +} > + > +JSObject* > +IDBDatabase::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) Nit: I would move this before |IDBDatabase::GetObjectStoreNames| ::: dom/indexedDB/IDBDatabase.h @@ +19,5 @@ > #include "mozilla/dom/indexedDB/FileManager.h" > #include "mozilla/dom/indexedDB/IDBWrapperCache.h" > +#include "mozilla/dom/indexedDB/IDBRequest.h" > +#include "mozilla/dom/IDBObjectStoreBinding.h" > +#include "mozilla/dom/IDBTransactionBinding.h" Nit: Follow the "special" ordering scheme here: #include "mozilla/dom/indexedDB/IndexedDatabase.h" #include "nsIDocument.h" #include "nsIFileStorage.h" #include "nsIOfflineStorage.h" #include "mozilla/Attributes.h" #include "mozilla/dom/IDBObjectStoreBinding.h" #include "mozilla/dom/IDBTransactionBinding.h" #include "nsDOMEventTargetHelper.h" #include "mozilla/dom/indexedDB/FileManager.h" #include "mozilla/dom/indexedDB/IDBRequest.h" #include "mozilla/dom/indexedDB/IDBWrapperCache.h" @@ +160,5 @@ > CreateObjectStoreInternal(IDBTransaction* aTransaction, > const ObjectStoreInfoGuts& aInfo, > + ErrorResult& aRv); > + > + // WebIDL // nsWrapperCache virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE; // WebIDL nsPIDOMWindow* GetParentObject() const { return GetOwner(); } void GetName(nsString& aName) const { NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); aName.Assign(mName); } ... @@ +162,5 @@ > + ErrorResult& aRv); > + > + // WebIDL > + > + nsPIDOMWindow* GetParentObject() const; Nit: return value on a separate line here and elsewhere @@ +190,5 @@ > + Transaction(const nsAString& aStoreName, IDBTransactionMode aMode, > + ErrorResult& aRv); > + > + already_AddRefed<indexedDB::IDBTransaction> > + Transaction(const Sequence<nsString >& aStoreNames, IDBTransactionMode aMode, Nit: extra space is needed only when there are two ">" @@ +198,5 @@ > + IMPL_EVENT_HANDLER(error) > + IMPL_EVENT_HANDLER(versionchange) > + > + already_AddRefed<IDBRequest> > + MozCreateFileHandle(const nsAString& aName, const Optional<nsAString >& aType, Nit: extra space is needed only when there are two ">" @@ +202,5 @@ > + MozCreateFileHandle(const nsAString& aName, const Optional<nsAString >& aType, > + ErrorResult& aRv); > + > + // nsIOfflineStorage > + NS_IMETHOD Close(); this can be removed, then you also need to change NS_DECL_NSIOFFLINESTORAGE_NOCLOSE to NS_DECL_NSIOFFLINESTORAGE in this file and maybe remove the NS_DECL_NSIOFFLINESTORAGE_NOCLOSE completely (from nsIOfflineStorage.h) nsIIDBDatabase contained the same method, so we had to create NS_DECL_NSIOFFLINESTORAGE_NOCLOSE ::: dom/indexedDB/IDBFileHandle.h @@ +44,5 @@ > virtual already_AddRefed<nsIDOMFile> > CreateFileObject(mozilla::dom::file::LockedFile* aLockedFile, > uint32_t aFileSize); > > + IDBDatabase* Database(); Nit: put return value on a separate line while you are here ::: dom/indexedDB/ipc/IndexedDBParent.cpp @@ +30,5 @@ > #include "IDBKeyRange.h" > #include "IDBObjectStore.h" > #include "IDBTransaction.h" > > +#include "mozilla/dom/IDBDatabaseBinding.h" sigh, Ms2ger completely changed these includes recently personally, I would change it back, bent confirmed that we should follow the "special" style in this module so here's the list: #include "IndexedDBParent.h" #include "nsIDOMEvent.h" #include "nsIDOMFile.h" #include "nsIXPConnect.h" #include "mozilla/AppProcessChecker.h" #include "mozilla/Assertions.h" #include "mozilla/Attributes.h" #include "mozilla/dom/ContentParent.h" #include "mozilla/dom/IDBDatabaseBinding.h" #include "mozilla/dom/ipc/Blob.h" #include "mozilla/dom/TabParent.h" #include "mozilla/unused.h" #include "mozilla/Util.h" #include "nsContentUtils.h" #include "nsCxPusher.h" #include "AsyncConnectionHelper.h" #include "DatabaseInfo.h" #include "IDBDatabase.h" #include "IDBEvents.h" #include "IDBFactory.h" #include "IDBIndex.h" #include "IDBKeyRange.h" #include "IDBObjectStore.h" #include "IDBTransaction.h" @@ +393,5 @@ > > MOZ_ASSERT(!JSVAL_IS_PRIMITIVE(result)); > > + IDBDatabase *database; > + Nit: remove this empty line @@ +888,3 @@ > } > > + if (rv.Failed()) { NS_ENSURE_SUCCESS() warns @@ +940,3 @@ > } > > + if (rv.Failed()) { dtto ::: dom/webidl/IDBDatabase.webidl @@ +23,5 @@ > + [Throws] > + void deleteObjectStore (DOMString name); > + > + [Throws] > + IDBTransaction transaction (DOMString storeName, optional IDBTransactionMode mode = "readonly"); Don't you want to add a comment here? The spec uses a union (not overloaded methods) Did you actually try the union with our webidl parser and code generator? @@ +40,5 @@ > }; > > +partial interface IDBDatabase { > + [Throws] > + IDBRequest mozCreateFileHandle(DOMString name, optional DOMString type); Nit: |IDBRequest mozCreateFileHandle (DOMString name, optional DOMString type);

> @@ +474,4 @@ > > { > > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + DatabaseInfo* info = Info(); > > + return info->version; > > you can inline this If we inline this we have to expose DatabaseInfo and probably it's not what we want. > @@ +523,2 @@ > > KeyPath keyPath(0); > > + if (NS_FAILED(KeyPath::Parse(aCx, aOptionalParameters.mKeyPath, &keyPath))) { > > I wonder if we miss "|| !keyPath.IsValid()" here We don't have IsValid() in the original CreateIndex(). > Did you actually try the union with our webidl parser and code generator? yes. It's not supported yet.

(In reply to Andrea Marchesini (:baku) from comment #3) > > @@ +474,4 @@ > > > { > > > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > > + DatabaseInfo* info = Info(); > > > + return info->version; > > > > you can inline this > > If we inline this we have to expose DatabaseInfo and probably it's not what > we want. ah, you're right > > > @@ +523,2 @@ > > > KeyPath keyPath(0); > > > + if (NS_FAILED(KeyPath::Parse(aCx, aOptionalParameters.mKeyPath, &keyPath))) { > > > > I wonder if we miss "|| !keyPath.IsValid()" here > > > We don't have IsValid() in the original CreateIndex(). http://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/IDBObjectStore.cpp#2658 > > > Did you actually try the union with our webidl parser and code generator? > > yes. It's not supported yet. ok

Comment on attachment 783696 [details] [diff] [review] database.patch Review of attachment 783696 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBDatabase.cpp @@ +38,5 @@ > > #include "mozilla/dom/IDBDatabaseBinding.h" > > USING_INDEXEDDB_NAMESPACE > +using mozilla::ErrorResult; Nit: move after |using mozilla::dom::quota::QuotaManager;| @@ +589,5 @@ > nsRefPtr<DeleteObjectStoreHelper> helper = > new DeleteObjectStoreHelper(transaction, objectStoreInfo->id); > > + nsresult rv = helper->DispatchToTransactionPool(); > + if (NS_FAILED(rv)) { add a warning here @@ +663,5 @@ > } > > nsRefPtr<IDBTransaction> transaction = > + IDBTransaction::Create(this, aStoreNames, transactionMode, false); > + if (!transaction) { add a warning here @@ +709,5 @@ > QuotaManager* quotaManager = QuotaManager::Get(); > NS_ASSERTION(quotaManager, "We should definitely have a manager here"); > > nsresult rv = helper->Dispatch(quotaManager->IOThread()); > + if (NS_FAILED(rv)) { add a warning here

Ready to land

Comment on attachment 783753 [details] [diff] [review] database.patch r=me