888598 - Move IDBTransaction to WebIDL

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Jan Varga [:janv]]

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

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

It looks good overall, requesting feedback from bent on the style in IDB webidl files.

::: dom/indexedDB/IDBTransaction.cpp
@@ +586,5 @@
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>    NS_ASSERTION(aRequest, "This is undesirable.");
>  
> +  mozilla::ErrorResult rv;

this builds fine w/o the prefix on mac, is this needed only on windows ?

you could add a new "using" for that since it is needed more than once

@@ +688,5 @@
>    }
>  
>    uint32_t count = arrayOfNames->Length();
>    for (uint32_t index = 0; index < count; index++) {
> +    if (!list->Add(arrayOfNames->ElementAt(index))) {

add NS_WARNING() here ?

@@ +721,5 @@
>    }
>  
>    nsRefPtr<IDBObjectStore> objectStore =
>      GetOrCreateObjectStore(aName, info, false);
> +  if (!objectStore) {

add NS_WARNING() here ?

@@ +1282,5 @@
> +JSObject*
> +IDBTransaction::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope)
> +{
> +  return IDBTransactionBinding::Wrap(aCx, aScope, this);
> +}

Nit: I would move this before GetMode()

::: dom/indexedDB/IDBTransaction.h
@@ +29,1 @@
>  

Nit: move before mozilla/dom/indexedDB/IDBDatabase.h

@@ +214,5 @@
>    {
>      return mSerialNumber;
>    }
>  #endif
>  

I think it would be cleaner to use following order and comments:

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

// WebIDL
nsPIDOMWindow*
GetParentObject() const
{
  return GetOwner();
}

IDBTransactionMode
GetMode(ErrorResult& aRv) const;

...

@@ +216,5 @@
>    }
>  #endif
>  
> +  // WebIDL
> +  nsPIDOMWindow* GetParentObject() const;

you can inline this

@@ +221,5 @@
> +
> +  virtual JSObject*
> +  WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  IDBTransactionMode GetMode(ErrorResult& aRv) const;

return value on separate line, dtto for other methods, I know there are some methods in this file which don't follow it, but we are slowly converging to the new style :)

@@ +223,5 @@
> +  WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
> +
> +  IDBTransactionMode GetMode(ErrorResult& aRv) const;
> +
> +  nsIIDBDatabase* Db() const;

you can inline this too (including the main thread assertion)
this method isn't virtual anymore, so it makes sense to inline it here

@@ +230,5 @@
> +
> +  already_AddRefed<nsIIDBObjectStore>
> +  ObjectStore(const nsAString& aName, ErrorResult& aRv);
> +
> +  void Abort(ErrorResult& aRv);

inline this one too

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +812,5 @@
>  
> +      ErrorResult rv;
> +      nsCOMPtr<nsIIDBObjectStore> store = mTransaction->ObjectStore(name, rv);
> +      if (rv.Failed()) {
> +        return false;

You should add a warning here (NS_WARNING)

::: dom/webidl/IDBTransaction.webidl
@@ +14,5 @@
> +    "readonly",
> +    "readwrite",
> +    "versionchange"
> +};
> +

I'm not sure what style we want to use for IDB webidl interfaces.
The style in this patch is obviously the style used in HTML*.webidl and the IndexedDB spec uses it too.
nsIIDB*.idl used a different style and IDBFactory.webidl uses it too.
Personally, I'd prefer the nsIIDB*.idl style, but I'll let bent to be the judge.
Also, all the interface comments got removed in this patch, some of them could stay I think.
For example these (slightly modified):
// Don't commit the transaction.

// Event handler that fires when the transaction is completed successfully.

// Event handler that fires when the transaction is aborted.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +43,1 @@
>  

Nit: what about moving this before IDBVersionChangeEventBinding.h

@@ +484,5 @@
>      // Init WebIDL binding constructors wanted on all XPConnect globals.
>      if (!IDBVersionChangeEventBinding::GetConstructorObject(aJSContext, global) ||
>          !TextDecoderBinding::GetConstructorObject(aJSContext, global) ||
>          !TextEncoderBinding::GetConstructorObject(aJSContext, global) ||
> +        !IDBTransactionBinding::GetConstructorObject(aJSContext, global) ||

Nit: move before IDBVersionChangeEventBinding.h too ?

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Jan Varga [:janv] from comment #2)
> ::: dom/webidl/IDBTransaction.webidl
> @@ +14,5 @@
> > +    "readonly",
> > +    "readwrite",
> > +    "versionchange"
> > +};
> > +
> 
> I'm not sure what style we want to use for IDB webidl interfaces.

We want to match the spec.

Comment 4

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #2)
> ::: dom/indexedDB/IDBTransaction.h
> @@ +29,1 @@
> >  
> 
> Nit: move before mozilla/dom/indexedDB/IDBDatabase.h
> 

that was for:
+#include "mozilla/dom/IDBTransactionBinding.h"

Comment 5

[Andrea Marchesini [:baku]]

Attachment: transaction.patch

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 781008 [details] [diff] [review]
transaction.patch

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

I think it's fine to just straight copy-paste the webidl files from the spec.

Comment 7

[Jan Varga [:janv]]

Comment on attachment 781008 [details] [diff] [review]
transaction.patch

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

::: dom/indexedDB/IDBTransaction.cpp
@@ +35,5 @@
>  #include "ipc/IndexedDBChild.h"
>  
>  #define SAVEPOINT_NAME "savepoint"
>  
> +using namespace mozilla;

hrm, I meant "using mozilla::ErrorResult", not entire mozilla namespace
and put it after "using mozilla::dom::quota::QuotaManager;" please

@@ +689,5 @@
>  
>    uint32_t count = arrayOfNames->Length();
>    for (uint32_t index = 0; index < count; index++) {
> +    if (!list->Add(arrayOfNames->ElementAt(index))) {
> +      NS_WARNING("Error adding a new element to a nsDOMStringList");

Nit: What about just: "Failed to add element!"

@@ +723,5 @@
>  
>    nsRefPtr<IDBObjectStore> objectStore =
>      GetOrCreateObjectStore(aName, info, false);
> +  if (!objectStore) {
> +    NS_WARNING("objectStore doesn't exist");

Nit: What about: "Failed to get or create object store!"

::: dom/indexedDB/IDBTransaction.h
@@ +22,5 @@
>  #include "nsRefPtrHashtable.h"
>  
>  #include "mozilla/dom/indexedDB/IDBDatabase.h"
>  #include "mozilla/dom/indexedDB/IDBWrapperCache.h"
> +#include "mozilla/dom/indexedDB/IndexedDatabase.h"

please don't do this, see https://bugzilla.mozilla.org/show_bug.cgi?id=860731#c11

@@ +27,2 @@
>  #include "mozilla/dom/indexedDB/FileInfo.h"
> +#include "mozilla/dom/IDBTransactionBinding.h"

Nit: I meant to just move |#include "mozilla/dom/IDBTransactionBinding.h"| *before* |#include "mozilla/dom/indexedDB/IDBDatabase.h"|

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +797,5 @@
>  
> +      ErrorResult rv;
> +      nsCOMPtr<nsIIDBObjectStore> store = mTransaction->ObjectStore(name, rv);
> +      if (rv.Failed()) {
> +        NS_WARNING("Didn't get the object we expected!");

Nit: what about just: "Failed to get object store!"

::: js/xpconnect/src/nsXPConnect.cpp
@@ +35,5 @@
>  #include "XPCQuickStubs.h"
>  
>  #include "mozilla/dom/BindingUtils.h"
>  #include "mozilla/dom/IDBVersionChangeEventBinding.h"
> +#include "mozilla/dom/IDBTransactionBinding.h"

Nit: *before* IDBVersionChangeEventBinding.h

@@ +544,5 @@
>      MOZ_ASSERT(js::GetObjectClass(global)->flags & JSCLASS_DOM_GLOBAL);
>  
>      // Init WebIDL binding constructors wanted on all XPConnect globals.
>      if (!IDBVersionChangeEventBinding::GetConstructorObject(aJSContext, global) ||
> +        !IDBTransactionBinding::GetConstructorObject(aJSContext, global) ||

Nit: *before* IDBVersionChangeEventBinding.h

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch

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

Comment 9

[Jan Varga [:janv]]

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

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

::: dom/indexedDB/IDBTransaction.cpp
@@ +36,5 @@
>  
>  #define SAVEPOINT_NAME "savepoint"
>  
>  using namespace mozilla::dom;
> +using namespace mozilla::ErrorResult;

why *namespace* ?
if you fix it, move it after "using mozilla::dom::quota::QuotaManager;" please

@@ +723,5 @@
>  
>    nsRefPtr<IDBObjectStore> objectStore =
>      GetOrCreateObjectStore(aName, info, false);
> +  if (!objectStore) {
> +    NS_WARNING("Failed to get or create object store");

Nit: missing "!"

::: dom/indexedDB/IDBTransaction.h
@@ +21,5 @@
>  #include "nsInterfaceHashtable.h"
>  #include "nsRefPtrHashtable.h"
>  
> +#include "mozilla/dom/IDBTransactionBinding.h"
> +#include "mozilla/dom/indexedDB/IndexedDatabase.h"

Did you read the comment in the other bug?
Please keep |#include "mozilla/dom/indexedDB/IndexedDatabase.h"| at the original position/line.

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +797,5 @@
>  
> +      ErrorResult rv;
> +      nsCOMPtr<nsIIDBObjectStore> store = mTransaction->ObjectStore(name, rv);
> +      if (rv.Failed()) {
> +        NS_WARNING("Failed to get object store");

Nit: missing "!", see all other warnings in the module

Comment 10

[Jan Varga [:janv]]

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

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

::: dom/indexedDB/IDBTransaction.cpp
@@ +634,3 @@
>  }
>  
> +IDBTransactionMode

I think this should be fully qualified:
mozilla::dom::IDBTransactionMode
here and in each "case" below

Comment 11

[Andrea Marchesini [:baku]]

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

Comment 12

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 13

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/268f7f146100

Comment 14

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/224d10bf59b244df81752735a67dc35abd5f4a83
Sync Bug 888598's changes to Jetpack - Move IDBTransaction to WebIDL r=janv

Comment 15

[Andrea Marchesini [:baku]]

We need to update the documentation because IDBTransaction.READ_WRITE, IDBTransaction.READ_ONLY and IDBTransaction.VERSION_CHANGE have been removed.

Can we check if this breaks some addons?

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Andrea Marchesini (:baku) from comment #15)
> Can we check if this breaks some addons?

They were marked as deprecated a long time ago (and they cause Error Console warnings if you use them) so I personally think it's fine to proceed here.

Comment 17

[(no longer active)]

(In reply to Andrea Marchesini (:baku) from comment #15)
> We need to update the documentation because IDBTransaction.READ_WRITE,
> IDBTransaction.READ_ONLY and IDBTransaction.VERSION_CHANGE have been removed.
> 
> Can we check if this breaks some addons?

<https://mxr.mozilla.org/addons/search?string=IDBTransaction.VERSION_CHANGE> is empty.

<https://mxr.mozilla.org/addons/search?string=IDBTransaction.READ> shows one result which is not commented out: <https://mxr.mozilla.org/addons/source/438534/chrome/content/core/IDBWrapper.js#1>, which, I believe is related to this add-on: <http://www.cashola.com.br/lembrador>

Comment 18

[Andrea Marchesini [:baku]]

> <https://mxr.mozilla.org/addons/search?string=IDBTransaction.READ> shows one
> result which is not commented out:
> <https://mxr.mozilla.org/addons/source/438534/chrome/content/core/IDBWrapper.
> js#1>, which, I believe is related to this add-on:
> <http://www.cashola.com.br/lembrador>

what do we do usually in this case? Can we contact the developers?

Comment 19

[(no longer active)]

(In reply to comment #18)
> > <https://mxr.mozilla.org/addons/search?string=IDBTransaction.READ> shows one
> > result which is not commented out:
> > <https://mxr.mozilla.org/addons/source/438534/chrome/content/core/IDBWrapper.
> > js#1>, which, I believe is related to this add-on:
> > <http://www.cashola.com.br/lembrador>
> 
> what do we do usually in this case? Can we contact the developers?

It would be nice if you can find their contact info and contact them, for sure!  But this is only one add-on, I don't think we should be blocked on that.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/268f7f146100

Comment 21

[Kohei Yoshino]

https://developer.mozilla.org/en-US/docs/Web/API/IDBTransaction#Constants
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25/Site_Compatibility