Move IDBTransaction to WebIDL

RESOLVED FIXED in mozilla25

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Ms2ger, Assigned: baku)

Tracking

(Blocks: 1 bug, {addon-compat, dev-doc-complete, site-compat})

Trunk
mozilla25
addon-compat, dev-doc-complete, site-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

4 years ago
Assignee: nobody → amarchesini
(Assignee)

Comment 1

4 years ago
Created attachment 772924 [details] [diff] [review]
patch
Attachment #772924 - Flags: review?(Jan.Varga)

Comment 2

4 years ago
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)
(Reporter)

Comment 3

4 years ago
(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

4 years ago
(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"
(Assignee)

Comment 5

4 years ago
Created attachment 781008 [details] [diff] [review]
transaction.patch
Attachment #772924 - Attachment is obsolete: true
Attachment #772924 - Flags: feedback?(bent.mozilla)
Attachment #781008 - Flags: feedback?(bent.mozilla)
(Assignee)

Updated

4 years ago
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 7

4 years ago
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+
(Assignee)

Comment 8

4 years ago
Created attachment 781961 [details] [diff] [review]
patch

https://tbpl.mozilla.org/?tree=Try&rev=c99aed729935
Attachment #781008 - Attachment is obsolete: true

Comment 9

4 years ago
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

4 years ago
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
(Assignee)

Comment 11

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=59e10ae84f7a
(Assignee)

Comment 12

4 years ago
Created attachment 782637 [details] [diff] [review]
patch
Attachment #781961 - Attachment is obsolete: true
(Assignee)

Comment 13

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

Comment 14

4 years ago
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
(Assignee)

Comment 15

4 years ago
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
Depends on: 899584
(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>
(Assignee)

Comment 18

4 years ago
> <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
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
https://developer.mozilla.org/en-US/docs/Web/API/IDBTransaction#Constants
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25/Site_Compatibility
Keywords: dev-doc-needed → addon-compat, dev-doc-complete, site-compat
You need to log in before you can comment on or make changes to this bug.