Last Comment Bug 735094 - IndexedDB: Use new fangled string constants
: IndexedDB: Use new fangled string constants
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Jonas Sicking (:sicking)
:
Mentors:
Depends on:
Blocks: 735357 735536 736376 777191
  Show dependency treegraph
 
Reported: 2012-03-12 16:15 PDT by Jonas Sicking (:sicking)
Modified: 2015-12-14 06:13 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to fix (127.27 KB, patch)
2012-03-12 16:15 PDT, Jonas Sicking (:sicking)
bent.mozilla: review+
Details | Diff | Review

Description Jonas Sicking (:sicking) 2012-03-12 16:15:09 PDT
Created attachment 605207 [details] [diff] [review]
Patch to fix
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-03-12 18:15:15 PDT
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 2 :Ms2ger 2012-03-13 01:24:10 PDT
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
Comment 3 Jan Varga [:janv] 2012-03-13 02:58:38 PDT
Looks like you forgot to update tests in dom/indexedDB/test.
Fortunately, they don't fail due to the temporary code in nsDOMClassInfo.cpp
Comment 4 Jonas Sicking (:sicking) 2012-03-13 08:14:33 PDT
That was intentional, I wanted to have tests that checks that the constants work.

https://hg.mozilla.org/mozilla-central/rev/5a7686f1119b
Comment 5 Jonas Sicking (:sicking) 2012-03-13 08:26:19 PDT
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.
Comment 6 :Ms2ger 2013-04-05 05:05:03 PDT
(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?
Comment 7 Jonas Sicking (:sicking) 2013-05-07 15:39:25 PDT
Ms2ger: I'll file a new bug if you attach a patch :)
Comment 8 Chris Mills (Mozilla, MDN editor) [:cmills] 2015-12-01 05:20:30 PST
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?
Comment 9 Jan Varga [:janv] 2015-12-01 05:31:20 PST
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.
Comment 10 Chris Mills (Mozilla, MDN editor) [:cmills] 2015-12-14 06:13:56 PST
(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

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