735094 - IndexedDB: Use new fangled string constants

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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 (he/him; ⌚ UTC+1/+2)]

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]]

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) No longer reading bugmail consistently]

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) No longer reading bugmail consistently]

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 (he/him; ⌚ UTC+1/+2)]

(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) No longer reading bugmail consistently]

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

Comment 8

[Chris Mills [:cmills]]

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]]

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 [:cmills]]

(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