Last Comment Bug 888598 - Move IDBTransaction to WebIDL
: Move IDBTransaction to WebIDL
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla25
Assigned To: Andrea Marchesini [:baku]
:
Mentors:
Depends on: 899584
Blocks: 888591 idbwebidl
  Show dependency treegraph
 
Reported: 2013-06-29 02:38 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2013-08-30 06:30 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (53.54 KB, patch)
2013-07-09 15:13 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
transaction.patch (53.68 KB, patch)
2013-07-25 08:36 PDT, Andrea Marchesini [:baku]
jvarga: review+
bent.mozilla: feedback+
Details | Diff | Splinter Review
patch (53.72 KB, patch)
2013-07-26 15:05 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review
patch (53.45 KB, patch)
2013-07-29 10:07 PDT, Andrea Marchesini [:baku]
no flags Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2013-06-29 02:38:39 PDT

    
Comment 1 Andrea Marchesini [:baku] 2013-07-09 15:13:42 PDT
Created attachment 772924 [details] [diff] [review]
patch
Comment 2 Jan Varga [:janv] 2013-07-25 05:30:09 PDT
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 (⌚ UTC+1/+2) 2013-07-25 05:39:01 PDT
(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] 2013-07-25 06:56:28 PDT
(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] 2013-07-25 08:36:25 PDT
Created attachment 781008 [details] [diff] [review]
transaction.patch
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2013-07-25 08:46:52 PDT
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] 2013-07-25 23:06:18 PDT
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] 2013-07-26 15:05:10 PDT
Created attachment 781961 [details] [diff] [review]
patch

https://tbpl.mozilla.org/?tree=Try&rev=c99aed729935
Comment 9 Jan Varga [:janv] 2013-07-26 22:27:22 PDT
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] 2013-07-29 01:30:06 PDT
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] 2013-07-29 09:58:25 PDT
https://tbpl.mozilla.org/?tree=Try&rev=59e10ae84f7a
Comment 12 Andrea Marchesini [:baku] 2013-07-29 10:07:01 PDT
Created attachment 782637 [details] [diff] [review]
patch
Comment 13 Andrea Marchesini [:baku] 2013-07-29 10:13:07 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/268f7f146100
Comment 14 [github robot] 2013-07-29 13:28:36 PDT
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] 2013-07-30 00:08:13 PDT
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!) 2013-07-30 09:04:20 PDT
(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 :Ehsan Akhgari 2013-07-30 09:04:55 PDT
(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] 2013-07-30 09:36:39 PDT
> <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 :Ehsan Akhgari 2013-07-30 10:30:13 PDT
(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] 2013-07-30 10:32:35 PDT
https://hg.mozilla.org/mozilla-central/rev/268f7f146100

Note You need to log in before you can comment on or make changes to this bug.