Comment on attachment 605207 [details] [diff] [review] Patch to fix Review of attachment 605207 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDOMClassInfo.cpp @@ +5709,5 @@ > +const char* IDBConstant::IDBCursor = "IDBCursor"; > +const char* IDBConstant::IDBRequest = "IDBRequest"; > +const char* IDBConstant::IDBTransaction = "IDBTransaction"; > + > +IDBConstant sIDBConstants[] = { Should be static const. @@ +5721,5 @@ > + { IDBConstant::IDBTransaction, "READ_WRITE", "readwrite" }, > + { IDBConstant::IDBTransaction, "VERSION_CHANGE", "versionchange" }, > +}; > + > +JSBool IDBConstantGetter(JSContext *cx, JSObject *obj, jsid id, jsval* vp) Should be static. @@ +5729,5 @@ > + int8_t index = JSID_TO_INT(id); > + > + MOZ_ASSERT((uint8_t)index < mozilla::ArrayLength(sIDBConstants)); > + > + IDBConstant& c = sIDBConstants[index]; const. @@ +5734,5 @@ > + > + // Put a warning on the console > + nsString warnText = > + NS_LITERAL_STRING("The constant ") + > + NS_ConvertASCIItoUTF16(nsDependentCString(c.interface)) + No need for the nsDependentCString, NS_ConvertASCIItoUTF16 can handle char*. @@ +5747,5 @@ > + if (context) { > + nsCOMPtr<nsPIDOMWindow> window = > + do_QueryInterface(context->GetGlobalObject()); > + if (window) { > + window = window->GetCurrentInnerWindow(); Nit: NS_WARNING if this fails? @@ +5755,5 @@ > + } > + } > + > + nsCOMPtr<nsIScriptError> errorObject = > + do_CreateInstance(NS_SCRIPTERROR_CONTRACTID); And here. @@ +5762,5 @@ > + nsnull, // file name > + nsnull, // source line > + 0, 0, // Line/col number > + nsIScriptError::warningFlag, > + "DOM Core", windowID); And here. @@ +5773,5 @@ > + } > + } > + > + // Redefine property to remove getter > + NS_ConvertASCIItoUTF16 valStr(nsDependentCString(c.value)); Lose nsDependentCString. @@ +5778,5 @@ > + jsval value; > + if (!xpc::StringToJsval(cx, valStr, &value)) { > + return JS_FALSE; > + } > + if (!::JS_DefineProperty(cx, obj, c.name, value, nsnull, nsnull, Nit: You really don't need the :: here. @@ +5789,5 @@ > + return JS_TRUE; > +} > + > +static nsresult > +DefinedIDBInterfaceConstants(JSContext *cx, JSObject *obj, const nsIID *aIID) Nit: "Defined" is not what you want, you want "Define", right? ::: dom/indexedDB/IDBCursor.cpp @@ +353,5 @@ > mContinueToKey = aKey; > > #ifdef DEBUG > { > + nsAutoString readyState; Just nsString. ::: dom/indexedDB/IDBDatabase.cpp @@ +547,5 @@ > if (mRunningVersionChange) { > return NS_ERROR_DOM_INDEXEDDB_NOT_ALLOWED_ERR; > } > > + IDBTransaction::TransMode transMode = IDBTransaction::READ_ONLY; Nit, "mode" maybe? "transMode" doesn't help imo. ::: dom/indexedDB/IDBTransaction.h @@ +92,5 @@ > NS_DECL_NSITHREADOBSERVER > > NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(IDBTransaction, IDBWrapperCache) > > + enum TransMode Nit: Mode! And then you can change Mode() to GetMode(). ::: dom/indexedDB/nsIIDBDatabase.idl @@ +77,5 @@ > > [optional_argc, implicit_jscontext] > nsIIDBTransaction > transaction(in jsval storeNames, // js array of strings > + [optional /* "readonly" */] in DOMString mode); I think this needs a comment now since there's no place to find acceptable values for the enum ::: dom/indexedDB/nsIIDBIndex.idl @@ +83,5 @@ > > [implicit_jscontext, optional_argc] > nsIIDBRequest > openCursor([optional /* null */] in jsval key, > + [optional /* "next" */] in DOMString direction); Same here. ::: dom/indexedDB/nsIIDBObjectStore.idl @@ +107,5 @@ > // no match. > [implicit_jscontext, optional_argc] > nsIIDBRequest > openCursor([optional /* null */] in jsval range, > + [optional /* "next" */] in DOMString direction); And here.

Comment on attachment 605207 [details] [diff] [review] Patch to fix Review of attachment 605207 [details] [diff] [review]: ----------------------------------------------------------------- s/JS_TRUE/true/g, s/JS_FALSE/false/g ::: dom/base/nsDOMClassInfo.cpp @@ +5800,5 @@ > + interface = IDBConstant::IDBRequest; > + } > + else if (aIID->Equals(NS_GET_IID(nsIIDBTransaction))) { > + interface = IDBConstant::IDBTransaction; > + } Assert that it's one of these interfaces, please. @@ +5802,5 @@ > + else if (aIID->Equals(NS_GET_IID(nsIIDBTransaction))) { > + interface = IDBConstant::IDBTransaction; > + } > + > + for (int8_t i = 0; i < (int8_t)mozilla::ArrayLength(sIDBConstants); ++i) { Mm, static assert that the length fits in an int8_t? @@ +6383,5 @@ > + primary_iid->Equals(NS_GET_IID(nsIIDBRequest)) || > + primary_iid->Equals(NS_GET_IID(nsIIDBTransaction))) { > + rv = DefinedIDBInterfaceConstants(cx, class_obj, primary_iid); > + NS_ENSURE_SUCCESS(rv, rv); > + } Fun that we have to do this stuff in two places; followup to refactor into one function? ::: dom/indexedDB/IDBCursor.cpp @@ +248,5 @@ > +{ > + if (aDirection.EqualsLiteral("next")) { > + *aResult = NEXT; > + } > + else if (aDirection.EqualsLiteral("nextunique")) { Cuddle @@ +469,4 @@ > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + switch(mDirection) { 'switch (' ::: dom/indexedDB/IDBTransaction.cpp @@ +533,4 @@ > { > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > + switch(mMode) { 'switch (' ::: dom/indexedDB/IDBTransaction.h @@ +152,5 @@ > { > return mAborted; > } > > + TransMode Mode() const

Looks like you forgot to update tests in dom/indexedDB/test. Fortunately, they don't fail due to the temporary code in nsDOMClassInfo.cpp

That was intentional, I wanted to have tests that checks that the constants work. https://hg.mozilla.org/mozilla-central/rev/5a7686f1119b

Didn't see Ms2ger's comments until after I'd checked in. Note that the code is going to be removed in a few months though so it's not big of a deal that it isn't perfect. It's not intended to be expanded on over time.

(In reply to Jonas Sicking (:sicking) from comment #5) > Didn't see Ms2ger's comments until after I'd checked in. > > Note that the code is going to be removed in a few months though so it's not > big of a deal that it isn't perfect. It's not intended to be expanded on > over time. Few months are over, I'd say. Who's removing?

Ms2ger: I'll file a new bug if you attach a patch :)

The old style constants are obsoleted, with pointers towards the new style constants, in appropriate places. For example: https://developer.mozilla.org/en-US/docs/Web/API/IDBCursor#Constants Is this ok?

The old style constants were removed, for example by bug 888598 which landed for FF 25, so I guess the term "obsolete since Gecko 25" is not 100% correct. The patch here landed for FF 13, so I would say it was deprecated in 13 and removed in 25.

(In reply to Jan Varga [:janv] from comment #9) > The old style constants were removed, for example by bug 888598 which landed > for FF 25, so I guess the term "obsolete since Gecko 25" is not 100% > correct. The patch here landed for FF 13, so I would say it was deprecated > in 13 and removed in 25. Ok, makes sense. I've updated all the sections as appropriate, e.g. https://developer.mozilla.org/en-US/docs/Web/API/IDBCursor#Constants