Closed Bug 888598 Opened 11 years ago Closed 11 years ago

Move IDBTransaction to WebIDL

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: Ms2ger, Assigned: baku)

References

Details

(Keywords: addon-compat, dev-doc-complete, site-compat)

Attachments

(1 file, 3 obsolete files)

      No description provided.
Assignee: nobody → amarchesini
Attached patch patch (obsolete) — Splinter Review
Attachment #772924 - Flags: review?(Jan.Varga)
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 ?
Attachment #772924 - Flags: review?(Jan.Varga) → feedback?(bent.mozilla)
(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.
(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"
Attached patch transaction.patch (obsolete) — Splinter Review
Attachment #772924 - Attachment is obsolete: true
Attachment #772924 - Flags: feedback?(bent.mozilla)
Attachment #781008 - Flags: feedback?(bent.mozilla)
Attachment #781008 - Flags: review?(Jan.Varga)
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.
Attachment #781008 - Flags: feedback?(bent.mozilla) → feedback+
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
Attachment #781008 - Flags: review?(Jan.Varga) → review+
Attached patch patch (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=c99aed729935
Attachment #781008 - Attachment is obsolete: true
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 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
Attached patch patchSplinter Review
Attachment #781961 - Attachment is obsolete: true
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
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?
Keywords: dev-doc-needed
(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.
(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>
> <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?
(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.
https://hg.mozilla.org/mozilla-central/rev/268f7f146100
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.