888596 - Move IDBDatabase to WebIDL

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Jan Varga [:janv]]

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);

Comment 3

[Andrea Marchesini [:baku]]

> @@ +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.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: database.patch

Comment 5

[Jan Varga [:janv]]

(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 6

[Andrea Marchesini [:baku]]

Attachment: database.patch

Comment 7

[Jan Varga [:janv]]

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

Comment 8

[Andrea Marchesini [:baku]]

Attachment: database.patch

Ready to land

Comment 9

[Jan Varga [:janv]]

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

r=me

Comment 10

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f4550642535

Comment 11

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=da1e64bed527

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9f4550642535