:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Description • 11 years ago

      No description provided.

	Andrea Marchesini [:baku] Assignee
	Comment 2 • 11 years ago

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

	Jan Varga [:janv]
	Comment 3 • 11 years ago

Comment on attachment 773961 [details] [diff] [review]
patch

Review of attachment 773961 [details] [diff] [review]:
-----------------------------------------------------------------

I wonder whether we need something like NS_ENSURE_SUCCESS/NS_ENSURE_TRUE that works with ErrorResult.
bent: What do you think? Should we just add warnings everywhere for now ?

::: dom/indexedDB/IDBCursor.cpp
@@ +317,5 @@
> +      *aResult = PREV_UNIQUE;
> +      break;
> +
> +    default:
> +      return NS_ERROR_TYPE_ERR;

The caller will assert while assigning this nsresult to an ErrorResult (NS_ERROR_TYPE_ERR can't be assigned directly).
Actually I don't think you need to return NS_ERROR_TYPE_ERR here, replace it with |MOZ_CRASH("Unknown direction!");| and change return value to void

::: dom/indexedDB/IDBCursor.h
@@ +8,5 @@
>  #define mozilla_dom_indexeddb_idbcursor_h__
>  
>  #include "mozilla/dom/indexedDB/IndexedDatabase.h"
>  #include "mozilla/dom/indexedDB/IDBObjectStore.h"
> +#include "mozilla/dom/IDBCursorBinding.h"

Nit: it would be nice to fix the ordering while you are here:

#include "mozilla/dom/indexedDB/IndexedDatabase.h"

#include "nsIIDBCursorWithValue.h"

#include "mozilla/dom/IDBCursorBinding.h"
#include "nsCycleCollectionParticipant.h"

#include "mozilla/dom/indexedDB/IDBObjectStore.h"
#include "mozilla/dom/indexedDB/Key.h"

@@ +116,5 @@
>  
>    static nsresult ParseDirection(const nsAString& aDirection,
>                                   Direction* aResult);
>  
> +  static nsresult ParseDirection(IDBCursorDirection aDirection,

Nit: return value on a separate line

::: dom/indexedDB/IDBObjectStore.cpp
@@ +526,5 @@
>      /*
>       * Not setting this will cause JS_CHECK_RECURSION to report false
>       * positives
>       */
> +    JS_SetNativeStackQuota(mRuntime, 128 * sizeof(size_t) * 1024);

AFAIK, we fix white space only when we touch that code

@@ +1063,5 @@
>      if (rv == NS_ERROR_STORAGE_CONSTRAINT && updateInfo.indexUnique) {
>        // If we're inserting multiple entries for the same unique index, then
>        // we might have failed to insert due to colliding with another entry for
>        // the same index in which case we should ignore it.
> +

revert this change (see the comment above)

@@ +1590,5 @@
>          }
>          NS_ConvertUTF16toUTF8 convName(name);
>          uint32_t convNameLength = SwapBytes(convName.Length());
>  
> +        if (!JS_WriteBytes(aWriter, &lastModifiedDate, sizeof(lastModifiedDate)) ||

revert this change

@@ +1631,5 @@
>  
>      nsresult rv;
>      int32_t id = token.ToInteger(&rv);
>      NS_ENSURE_SUCCESS(rv, rv);
> +

revert this change

@@ +1723,5 @@
>    mAutoIncrement(false),
>    mActorChild(nullptr),
>    mActorParent(nullptr)
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

Nit: add a new line here

@@ +1811,5 @@
>  
> +already_AddRefed<IDBRequest>
> +IDBObjectStore::AddOrPut(JSContext* aCx, JS::Handle<JS::Value> aValue,
> +                         const Optional<JS::Handle<JS::Value> >& aKey,
> +                         bool aOverwrite, mozilla::ErrorResult& aRv)

add |using mozilla::ErrorResult| and remove the namespace prefix here and elsewhere

@@ +1827,3 @@
>    }
>  
> +  JS::Rooted<JS::Value> keyval(aCx, aKey.WasPassed() ? aKey.Value() : JSVAL_VOID);

Nit: max 80 chars per line

@@ +1839,4 @@
>    }
>  
>    nsRefPtr<IDBRequest> request = GenerateRequest(this);
> +  if (!request) {

NS_ENSURE_TRUE() warns

add:
NS_WARNING("Failed to generate request!");

@@ +1848,5 @@
>      new AddHelper(mTransaction, request, this, cloneWriteInfo, key,
>                    aOverwrite, updateInfo);
>  
> +  nsresult rv = helper->DispatchToTransactionPool();
> +  if (NS_FAILED(rv)) {

NS_ENSURE_SUCCESS() warns

add:
NS_WARNING("Failed to dispatch!");

@@ +2001,4 @@
>    }
>  
>    nsRefPtr<IDBRequest> request = GenerateRequest(this);
> +  if (!request) {

add a warning here

@@ +2009,5 @@
>    nsRefPtr<GetHelper> helper =
>      new GetHelper(mTransaction, request, this, aKeyRange);
>  
>    nsresult rv = helper->DispatchToTransactionPool();
> +  if (NS_FAILED(rv)) {

add a warning here

@@ +2039,4 @@
>    }
>  
>    nsRefPtr<IDBRequest> request = GenerateRequest(this);
> +  if (!request) {

add a warning here

@@ +2047,5 @@
>    nsRefPtr<GetAllHelper> helper =
>      new GetAllHelper(mTransaction, request, this, aKeyRange, aLimit);
>  
>    nsresult rv = helper->DispatchToTransactionPool();
> +  if (NS_FAILED(rv)) {

add a warning here

@@ +2084,4 @@
>    }
>  
>    nsRefPtr<IDBRequest> request = GenerateRequest(this);
> +  if (!request) {

add a warning here

@@ +2092,5 @@
>    nsRefPtr<DeleteHelper> helper =
>      new DeleteHelper(mTransaction, request, this, aKeyRange);
>  
>    nsresult rv = helper->DispatchToTransactionPool();
> +  if (NS_FAILED(rv)) {

add a warning here

@@ +2125,4 @@
>    }
>  
>    nsRefPtr<IDBRequest> request = GenerateRequest(this);
> +  if (!request) {

add a warning here

@@ +2132,5 @@
>  
>    nsRefPtr<ClearHelper> helper(new ClearHelper(mTransaction, request, this));
>  
>    nsresult rv = helper->DispatchToTransactionPool();
> +  if (NS_FAILED(rv)) {

add a warning here

@@ +2161,4 @@
>    }
>  
>    nsRefPtr<IDBRequest> request = GenerateRequest(this);
> +  if (!request) {

add a warning here

please check other NS_ENSURE_SUCCESS/NS_ENSURE_TRUE that got converted to NS_FAILED

@@ +2370,5 @@
>  }
>  
>  NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(IDBObjectStore)
>    NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mCachedKeyPath)
> +  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER

Nit: I would put this one first

@@ +2382,5 @@
>      NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mCreatedIndexes[i]");
>      cb.NoteXPCOMChild(static_cast<nsIIDBIndex*>(tmp->mCreatedIndexes[i].get()));
>    }
> +
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS

this one is already there, no?

@@ +2396,5 @@
>    if (tmp->mRooted) {
>      NS_DROP_JS_OBJECTS(tmp, IDBObjectStore);
>      tmp->mRooted = false;
>    }
> +  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER

Nit: I would put this one first

@@ +2517,3 @@
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +  return AddOrPut(aCx, aValue, aKey, false, aRv);

you can inline this

@@ +2526,3 @@
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +  return AddOrPut(aCx, aValue, aKey, true, aRv);

dtto

@@ +2614,5 @@
> +                            const Sequence<nsString >& aKeyPath,
> +                            const IDBIndexParameters& aOptionalParameters,
> +                            mozilla::ErrorResult& aRv)
> +{
> +  NS_PRECONDITION(NS_IsMainThread(), "Wrong thread!");

NS_PRECONDITION -> NS_ASSERTION

@@ +2954,5 @@
>        union {
>          double d;
>          uint64_t u;
>        } pun;
> +

revert this change

@@ +4456,5 @@
> +  return mTransaction;
> +}
> +
> +JSObject*
> +IDBObjectStore::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope)

Nit: I would move this before GetKeyPath()

::: dom/indexedDB/IDBObjectStore.h
@@ +7,5 @@
>  #ifndef mozilla_dom_indexeddb_idbobjectstore_h__
>  #define mozilla_dom_indexeddb_idbobjectstore_h__
>  
>  #include "mozilla/dom/indexedDB/IndexedDatabase.h"
> +#include "mozilla/dom/indexedDB/IDBRequest.h"

Nit: this should go before |#include "mozilla/dom/indexedDB/IDBTransaction.h"|

@@ +15,5 @@
>  #include "mozilla/dom/indexedDB/IDBTransaction.h"
>  #include "mozilla/dom/indexedDB/KeyPath.h"
> +#include "mozilla/dom/IDBObjectStoreBinding.h"
> +#include "mozilla/dom/IDBCursorBinding.h"
> +#include "mozilla/dom/IDBIndexBinding.h"

Nit: these three should go before |#include "nsCycleCollectionParticipant.h"| in alphabetical order

@@ +245,5 @@
>    SetInfo(ObjectStoreInfo* aInfo);
>  
>    static JSClass sDummyPropJSClass;
>  
> +  // WebIDL

// nsWrapperCache
virtual JSObject*
WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;

// WebIDL
IDBTransaction*
GetParentObject() const
{
  return mTransaction;
}

void
GetName(nsString& aName) const
{
  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
  aName.Assign(mName);
}

...

@@ +247,5 @@
>    static JSClass sDummyPropJSClass;
>  
> +  // WebIDL
> +
> +  IDBTransaction* GetParentObject() const;

you can inline this

Nit: return value on a separate line here and elsewhere

@@ +253,5 @@
> +  virtual JSObject*
> +  WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  void GetName(nsString& aName) const
> +  {

add the main thread assertion

@@ +257,5 @@
> +  {
> +    aName.Assign(mName);
> +  }
> +
> +  JS::Value GetKeyPath(JSContext* aCx, ErrorResult& aRv);

Nit: return value on a separate line, here and elsewhere

@@ +261,5 @@
> +  JS::Value GetKeyPath(JSContext* aCx, ErrorResult& aRv);
> +
> +  already_AddRefed<nsIDOMDOMStringList> GetIndexNames(ErrorResult& aRv);
> +
> +  indexedDB::IDBTransaction* Transaction() const

is the namespace prefix really needed ?

@@ +276,5 @@
> +
> +  already_AddRefed<IDBRequest>
> +  Put(JSContext* aCx, JS::Handle<JS::Value> aValue,
> +      const Optional<JS::Handle<JS::Value> >& aKey,
> +      ErrorResult& aRv);

Nit: |ErrorResult& aRv| fits after |aKey, |

@@ +281,5 @@
> +
> +  already_AddRefed<IDBRequest>
> +  Add(JSContext* aCx, JS::Handle<JS::Value> aValue,
> +      const Optional<JS::Handle<JS::Value> >& aKey,
> +      ErrorResult& aRv);

dtto

@@ +285,5 @@
> +      ErrorResult& aRv);
> +
> +  already_AddRefed<IDBRequest>
> +  Delete(JSContext* aCx, JS::Handle<JS::Value> aKey,
> +         ErrorResult& aRv);

dtto

@@ +289,5 @@
> +         ErrorResult& aRv);
> +
> +  already_AddRefed<IDBRequest>
> +  Get(JSContext* aCx, JS::Handle<JS::Value> aKey,
> +      ErrorResult& aRv);

dtto

@@ +301,5 @@
> +
> +  already_AddRefed<nsIIDBIndex>
> +  CreateIndex(JSContext* aCx, const nsAString& aName, const nsAString& aKeyPath,
> +              const IDBIndexParameters& aOptionalParameters,
> +              ErrorResult& aRv);

dtto

@@ +304,5 @@
> +              const IDBIndexParameters& aOptionalParameters,
> +              ErrorResult& aRv);
> +
> +  already_AddRefed<nsIIDBIndex>
> +  CreateIndex(JSContext* aCx, const nsAString& aName, const Sequence<nsString >& aKeyPath,

Nit: 80 chars per line

@@ +311,5 @@
> +
> +  already_AddRefed<nsIIDBIndex>
> +  Index(const nsAString& aName, ErrorResult &aRv);
> +
> +  void DeleteIndex(const nsAString& aIndexName, ErrorResult& aRv);

Nit: return value on a separate line

@@ +335,5 @@
>  
> +  already_AddRefed<IDBRequest>
> +  AddOrPut(JSContext* aCx, JS::Handle<JS::Value> aValue,
> +           const Optional<JS::Handle<JS::Value> >& aKey,
> +           bool aOverwrite, ErrorResult& aRv);

Nit: |bool aOverWite| fits after |aKey,|

@@ +340,5 @@
> +
> +  already_AddRefed<nsIIDBIndex>
> +  CreateIndex(JSContext* aCx, const nsAString& aName, KeyPath& aKeyPath,
> +              const IDBIndexParameters& aOptionalParameters,
> +              ErrorResult& aRv);

Nit: you can join these two lines

::: dom/indexedDB/KeyPath.h
@@ +6,5 @@
>  
>  #ifndef mozilla_dom_indexeddb_keypath_h__
>  #define mozilla_dom_indexeddb_keypath_h__
>  
>  #include "mozilla/dom/indexedDB/IndexedDatabase.h"

Nit: add an empty line here

@@ +34,5 @@
>    {
>      MOZ_COUNT_CTOR(KeyPath);
>    }
>  
> +  KeyPath(JSContext* aCx, const nsAString& aString)

Why not:
static nsresult
Parse(JSContext* aCx, const nsAString& aString, KeyPath* aKeyPath);

setting type to STRING and then to NONEXISTENT, if something fails, looks a bit odd

@@ +44,5 @@
> +      SetType(NONEXISTENT);
> +    }
> +  }
> +
> +  KeyPath(JSContext* aCx, const Sequence<nsString >& aStrings)

Why not:
static nsresult
Parse(JSContext* aCx, const Sequence<nsString>& aStrings, KeyPath* aKeyPath);

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +1279,3 @@
>    }
>  
> +  return !rv.Failed();

NS_ENSURE_SUCCESS() warns

add:
NS_WARNING("Failed to delete index!");

@@ +1322,3 @@
>      }
>  
> +    if (rv.Failed()) {

NS_ENSURE_SUCCESS() warns

add:
NS_WARNING("Failed to create index!");

@@ +1532,5 @@
>      AutoSetCurrentTransaction asct(mObjectStore->Transaction());
>  
> +    ErrorResult rv;
> +    request = mObjectStore->GetInternal(keyRange, rv);
> +    if (rv.Failed()) {

NS_ENSURE_SUCCESS() warns

add:
NS_WARNING("Failed to get value!");

@@ +1574,5 @@
>      AutoSetCurrentTransaction asct(mObjectStore->Transaction());
>  
> +    ErrorResult rv;
> +    request = mObjectStore->MozGetAllInternal(keyRange, aParams.limit(), rv);
> +    if (rv.Failed()) {

NS_ENSURE_SUCCESS() warns

add:
NS_WARNING("Failed to get all values!");

and I would keep the old method name: GetAllInternal

@@ +1657,5 @@
>      AutoSetCurrentTransaction asct(mObjectStore->Transaction());
>  
> +    ErrorResult rv;
> +    request = mObjectStore->DeleteInternal(keyRange, rv);
> +    if (rv.Failed()) {

NS_ENSURE_SUCCESS() warns

add:
NS_WARNING("Failed to delete value!");

@@ +1680,5 @@
>      AutoSetCurrentTransaction asct(mObjectStore->Transaction());
>  
> +    ErrorResult rv;
> +    request = mObjectStore->Clear(rv);
> +    if (rv.Failed()) {

NS_ENSURE_SUCCESS() warns

add:
NS_WARNING("Failed to clear object store!");

@@ +1721,5 @@
>      AutoSetCurrentTransaction asct(mObjectStore->Transaction());
>  
> +    ErrorResult rv;
> +    request = mObjectStore->CountInternal(keyRange, rv);
> +    if (rv.Failed()) {

NS_ENSURE_SUCCESS() warns

add:
NS_WARNING("Failed to count values!");

@@ +1764,5 @@
>      AutoSetCurrentTransaction asct(mObjectStore->Transaction());
>  
> +    ErrorResult rv;
> +    request = mObjectStore->OpenCursorInternal(keyRange, direction, rv);
> +    if (rv.Failed()) {

NS_ENSURE_SUCCESS() warns

add:
NS_WARNING("Failed to open cursor!");

::: dom/webidl/WebIDL.mk
@@ +160,5 @@
>    HTMLTimeElement.webidl \
>    HTMLTitleElement.webidl \
>    HTMLUListElement.webidl \
>    HTMLVideoElement.webidl \
> +  IDBIndex.webidl \

Nit: Move before IDBObjectStore.weibidl ?

::: js/xpconnect/src/nsXPConnect.cpp
@@ +38,5 @@
>  #include "mozilla/dom/IDBVersionChangeEventBinding.h"
>  #include "mozilla/dom/TextDecoderBinding.h"
>  #include "mozilla/dom/TextEncoderBinding.h"
>  #include "mozilla/dom/DOMErrorBinding.h"
> +#include "mozilla/dom/IDBObjectStoreBinding.h"

Nit: Don't forget to keep alphabetical order when you rebase this patch.

@@ +486,5 @@
>      if (!IDBVersionChangeEventBinding::GetConstructorObject(aJSContext, global) ||
>          !TextDecoderBinding::GetConstructorObject(aJSContext, global) ||
>          !TextEncoderBinding::GetConstructorObject(aJSContext, global) ||
>          !IDBTransactionBinding::GetConstructorObject(aJSContext, global) ||
> +        !IDBObjectStoreBinding::GetConstructorObject(aJSContext, global) ||

dtto

	Jan Varga [:janv]
	Comment 4 • 11 years ago

Actually, all those internal methods (e.g. GetInternal) could return nsresult (as they used to).
I think it makes sense to use nsresult internally and ErrorResult for WebIDL interface
If you do that, you don't have to add warnings (in place of original NS_ENSURE_SUCCESS) to internal methods nor to IndexedDBParent.cpp

	Jan Varga [:janv]
	Comment 5 • 11 years ago

Comment on attachment 773961 [details] [diff] [review]
patch

Review of attachment 773961 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/IDBObjectStore.webidl
@@ +60,5 @@
> +
> +partial interface IDBObjectStore {
> +    // Success fires IDBTransactionEvent, result == array of values for given keys
> +    [Throws]
> +    IDBRequest mozGetAll(optional any key, optional unsigned long limit);

Nit: |IDBRequest mozGetAll (optional any key, optional unsigned long limit);|

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 6 • 11 years ago

Comment on attachment 773961 [details] [diff] [review]
patch

Review of attachment 773961 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/IDBCursor.h
@@ +13,5 @@
>  #include "mozilla/dom/indexedDB/Key.h"
>  
>  #include "nsIIDBCursorWithValue.h"
>  
>  #include "nsCycleCollectionParticipant.h"

This should just be one sorted list without empty lines:

#include "mozilla/dom/IDBCursorBinding.h"
#include "mozilla/dom/indexedDB/IDBObjectStore.h"
#include "mozilla/dom/indexedDB/IndexedDatabase.h"
#include "mozilla/dom/indexedDB/Key.h"
#include "nsCycleCollectionParticipant.h"		
#include "nsIIDBCursorWithValue.h"

	Jan Varga [:janv]
	Comment 7 • 11 years ago

(In reply to :Ms2ger from comment #6)
> This should just be one sorted list without empty lines:

oh, I see now why you stopped working on the LockedFile conversion

	Jan Varga [:janv]
	Comment 9 • 11 years ago

Comment on attachment 783102 [details] [diff] [review]
patch

Review of attachment 783102 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/IDBCursor.cpp
@@ +318,5 @@
> +      break;
> +
> +    default:
> +      MOZ_CRASH("Unknown direction!");
> +      break;

no need for "break"

::: dom/indexedDB/IDBCursor.h
@@ +6,5 @@
>  
>  #ifndef mozilla_dom_indexeddb_idbcursor_h__
>  #define mozilla_dom_indexeddb_idbcursor_h__
>  
>  #include "mozilla/dom/indexedDB/IndexedDatabase.h"

add an empty line here

@@ +7,5 @@
>  #ifndef mozilla_dom_indexeddb_idbcursor_h__
>  #define mozilla_dom_indexeddb_idbcursor_h__
>  
>  #include "mozilla/dom/indexedDB/IndexedDatabase.h"
> +#include "nsIIDBCursorWithValue.h"

add an empty line here

@@ +9,5 @@
>  
>  #include "mozilla/dom/indexedDB/IndexedDatabase.h"
> +#include "nsIIDBCursorWithValue.h"
> +#include "mozilla/dom/IDBCursorBinding.h"
> +#include "nsCycleCollectionParticipant.h"

add an empty line here

::: dom/indexedDB/IDBObjectStore.cpp
@@ +50,5 @@
>  
>  USING_INDEXEDDB_NAMESPACE
>  using namespace mozilla::dom;
>  using namespace mozilla::dom::indexedDB::ipc;
> +using mozilla::ErrorResult;

this should go after |using mozilla::dom::quota::FileOutputStream;|

@@ +1834,4 @@
>    }
>  
> +  JS::Rooted<JS::Value> keyval(aCx, aKey.WasPassed()
> +                                 ? aKey.Value() : JSVAL_VOID);

? aKey.Value()
: JSVAL_VOID);

would be nicer (":" just below "?")

@@ +2511,5 @@
>  
> +already_AddRefed<IDBRequest>
> +IDBObjectStore::MozGetAll(JSContext* aCx,
> +                          const Optional<JS::Handle<JS::Value> >& aKey,
> +                          const Optional<uint32_t >& aLimit, ErrorResult& aRv)

no need for space before ">"

@@ +2592,5 @@
> +      return nullptr;
> +    }
> +  }
> +
> +  IDBCursor::ParseDirection(aDirection, &direction);

sorry I didn't notice this earlier, this should be:

IDBCursor::Direction direction = IDBCursor::ConvertDirection(aDirection);

::: dom/indexedDB/IDBObjectStore.h
@@ +10,5 @@
>  #include "mozilla/dom/indexedDB/IndexedDatabase.h"
>  
> +#include "mozilla/dom/IDBObjectStoreBinding.h"
> +#include "mozilla/dom/IDBIndexBinding.h"
> +#include "mozilla/dom/IDBCursorBinding.h"

alphabetical order ?

#include "mozilla/dom/IDBCursorBinding.h"
#include "mozilla/dom/IDBIndexBinding.h"
#include "mozilla/dom/IDBObjectStoreBinding.h"

@@ +15,1 @@
>  

remove this empty line

@@ +246,5 @@
>    SetInfo(ObjectStoreInfo* aInfo);
>  
>    static JSClass sDummyPropJSClass;
>  
> +  // WebIDL

"// nsWrapperCache" goes here

@@ +247,5 @@
>  
>    static JSClass sDummyPropJSClass;
>  
> +  // WebIDL
> +

remove this empty line

@@ +250,5 @@
> +  // WebIDL
> +
> +  virtual JSObject*
> +  WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +

"// WebIDL" goes here

@@ +350,5 @@
>  
> +  already_AddRefed<IDBRequest>
> +  AddOrPut(JSContext* aCx, JS::Handle<JS::Value> aValue,
> +           const Optional<JS::Handle<JS::Value> >& aKey,
> +           bool aOverwrite, ErrorResult& aRv);

Nit: |bool aOverWite| fits after |aKey,|

@@ +355,5 @@
> +
> +  already_AddRefed<nsIIDBIndex>
> +  CreateIndex(JSContext* aCx, const nsAString& aName, KeyPath& aKeyPath,
> +              const IDBIndexParameters& aOptionalParameters,
> +              ErrorResult& aRv);

Nit: you can join these two lines

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +1266,4 @@
>    }
>  
> +  if (rv.Failed()) {
> +    NS_WARNING("Failed to delete index!");

why not |return false| ?

::: dom/webidl/WebIDL.mk
@@ +163,5 @@
>    HTMLTitleElement.webidl \
>    HTMLTrackElement.webidl \
>    HTMLUListElement.webidl \
>    HTMLVideoElement.webidl \
> +  IDBIndex.webidl \

should this really go first ?

	Jan Varga [:janv]
	Comment 10 • 11 years ago

Ok, so you just got r+ for the ENSURE_SUCCESS macro patch. It would be highly appreciated if you reworked IDBObjectStore patch on top of it.

	Jan Varga [:janv]
	Comment 11 • 11 years ago

you can also add this to Bindings.conf

'IDBObjectStore': {
    ...

    'binaryNames': {
        'mozGetAll': 'getAll'
    }
}

	Andrea Marchesini [:baku] Assignee
	Comment 12 • 11 years ago

Ready to land once bent gives his feedback and when bug 899546 is fixed.

	Jan Varga [:janv]
	Comment 13 • 11 years ago

Comment on attachment 783593 [details] [diff] [review]
objectstore.patch

Review of attachment 783593 [details] [diff] [review]:
-----------------------------------------------------------------

I like these new macros, thanks for doing it!

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1834,4 @@
>    }
>  
> +  JS::Rooted<JS::Value> keyval(aCx, aKey.WasPassed()
> +                                 ? aKey.Value()

|? aKey.Value()| can go right after |aKey.WasPassed()|

":" should then be aligned below "?"

@@ +1842,5 @@
>    nsTArray<IndexUpdateInfo> updateInfo;
>  
>    JS::Rooted<JS::Value> value(aCx, aValue);
> +  aRv = GetAddInfo(aCx, value, keyval, cloneWriteInfo, key, updateInfo);
> +  ENSURE_SUCCESS(aRv, nullptr);

there was no NS_ENSURE_SUCCESS here, change it to:
if (aRv.Failed()) {
  return nullptr;
}

@@ +1856,5 @@
>      new AddHelper(mTransaction, request, this, cloneWriteInfo, key,
>                    aOverwrite, updateInfo);
>  
> +  nsresult rv = helper->DispatchToTransactionPool();
> +  if (NS_FAILED(rv)) {

this should be just ENSURE_SUCCESS()

@@ +2019,5 @@
>    nsRefPtr<GetHelper> helper =
>      new GetHelper(mTransaction, request, this, aKeyRange);
>  
>    nsresult rv = helper->DispatchToTransactionPool();
> +  if (NS_FAILED(rv)) {

this should be just ENSURE_SUCCESS()

@@ +2058,5 @@
>    nsRefPtr<GetAllHelper> helper =
>      new GetAllHelper(mTransaction, request, this, aKeyRange, aLimit);
>  
>    nsresult rv = helper->DispatchToTransactionPool();
> +  if (NS_FAILED(rv)) {

this should be just ENSURE_SUCCESS()

@@ +2105,5 @@
>    nsRefPtr<DeleteHelper> helper =
>      new DeleteHelper(mTransaction, request, this, aKeyRange);
>  
>    nsresult rv = helper->DispatchToTransactionPool();
> +  if (NS_FAILED(rv)) {

this should be just ENSURE_SUCCESS()

@@ +2147,5 @@
>  
>    nsRefPtr<ClearHelper> helper(new ClearHelper(mTransaction, request, this));
>  
>    nsresult rv = helper->DispatchToTransactionPool();
> +  if (NS_FAILED(rv)) {

this should be just ENSURE_SUCCESS()

@@ +2184,5 @@
>  
>    nsRefPtr<CountHelper> helper =
>      new CountHelper(mTransaction, request, this, aKeyRange);
>    nsresult rv = helper->DispatchToTransactionPool();
> +  if (NS_FAILED(rv)) {

this should be just ENSURE_SUCCESS()

@@ +2226,5 @@
>    nsRefPtr<OpenCursorHelper> helper =
>      new OpenCursorHelper(mTransaction, request, this, aKeyRange, direction);
>  
>    nsresult rv = helper->DispatchToTransactionPool();
> +  if (NS_FAILED(rv)) {

this should be just ENSURE_SUCCESS()

@@ +2316,5 @@
>      nsRefPtr<CreateIndexHelper> helper =
>        new CreateIndexHelper(mTransaction, index);
>  
>      nsresult rv = helper->DispatchToTransactionPool();
> +    if (NS_FAILED(rv)) {

this should be just ENSURE_SUCCESS()

@@ +2710,5 @@
>      nsRefPtr<DeleteIndexHelper> helper =
>        new DeleteIndexHelper(mTransaction, this, aName);
>  
> +    nsresult rv = helper->DispatchToTransactionPool();
> +    if (NS_FAILED(rv)) {

this should be just ENSURE_SUCCESS()

::: dom/indexedDB/KeyPath.cpp
@@ +237,5 @@
> +}
> +
> +//static
> +nsresult
> +KeyPath::Parse(JSContext* aCx, const Sequence<nsString>& aStrings, KeyPath* aKeyPath)

Nit: 80 chars max

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +1256,5 @@
>    {
>      AutoSetCurrentTransaction asct(mObjectStore->Transaction());
>  
> +    mObjectStore->DeleteIndex(aName, rv);
> +    ENSURE_SUCCESS(rv, false);

|ENSURE_SUCCESS(rv, false);| should go in place of original |NS_ENSURE_SUCCESS(rv, false);|

@@ +1261,3 @@
>    }
>  
> +  return !rv.Failed();

this should be reverted back to |return true|

@@ +1299,5 @@
>  
> +      ErrorResult rv;
> +      nsCOMPtr<nsIIDBIndex> obj = mObjectStore->CreateIndexInternal(info, rv);
> +      index = static_cast<IDBIndex*>(obj.get());
> +      ENSURE_SUCCESS(rv, false);

you're probably going to remove |index = static_cast<IDBIndex*>(obj.get());| in some following patch, but |ENSURE_SUCCESS(rv, false);| should be placed right after CreateIndexInternal() call

	Jan Varga [:janv]
	Comment 14 • 11 years ago

(In reply to Jan Varga [:janv] from comment #13)
> @@ +1856,5 @@
> >      new AddHelper(mTransaction, request, this, cloneWriteInfo, key,
> >                    aOverwrite, updateInfo);
> >  
> > +  nsresult rv = helper->DispatchToTransactionPool();
> > +  if (NS_FAILED(rv)) {
> 
> this should be just ENSURE_SUCCESS()

I take this back, we don't have a macro for this yet

	Andrea Marchesini [:baku] Assignee
	Comment 15 • 11 years ago

Ready to land.

	Jan Varga [:janv]
	Comment 16 • 11 years ago

Comment on attachment 783750 [details] [diff] [review]
objectstore.patch

r=me

	Andrea Marchesini [:baku] Assignee
	Comment 17 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/947b4570fae6

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/947b4570fae6