Closed Bug 730161 Opened 12 years ago Closed 12 years ago

Remove DOMFileException and IDBDatabaseException

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: emk, Assigned: emk)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 6 obsolete files)

These exception types are no longer defined in the latest spec.
Attached patch Implement IDBRequest.error (obsolete) — Splinter Review
This is required because code is useless for indexedDB errors.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #613153 - Flags: review?(bent.mozilla)
Attached patch Tests fix (obsolete) — Splinter Review
Attachment #613155 - Flags: review?(bent.mozilla)
Keywords: dev-doc-needed
Attachment #613155 - Attachment is patch: true
Comment on attachment 613153 [details] [diff] [review]
Implement IDBRequest.error

Review of attachment 613153 [details] [diff] [review]:
-----------------------------------------------------------------

A few nits:

::: dom/base/DOMError.cpp
@@ +24,5 @@
>  } // anonymous namespace
>  
>  // static
>  already_AddRefed<nsIDOMDOMError>
> +DOMError::CreateForNSResult(nsresult rv)

aRv

::: dom/indexedDB/IDBRequest.cpp
@@ +61,5 @@
>  USING_INDEXEDDB_NAMESPACE
>  
>  IDBRequest::IDBRequest()
>  : mResultVal(JSVAL_VOID),
> +  mError(nsnull),

No need; smart pointers are initialized to null.

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +2200,5 @@
>      NS_ERROR("Failed to create event!");
>      return;
>    }
>  
> +  nsCOMPtr<nsIDOMDOMError> error = 0;

s/= 0//
Comment on attachment 613154 [details] [diff] [review]
Replace IDBDatabaseException and FileException with DOMException

Review of attachment 613154 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/domerr.msg
@@ +83,5 @@
> +DOM4_MSG_DEF(ReadOnlyError, "A mutation operation was attempted in a READ_ONLY transaction.", NS_ERROR_DOM_INDEXEDDB_READ_ONLY_ERR)
> +DOM4_MSG_DEF(VersionError, "The operation failed because the stored database is a higher version than the version requested.", NS_ERROR_DOM_INDEXEDDB_VERSION_ERR)
> +
> +DOM4_MSG_DEF(NotFoundError, "The operation failed because the requested database object could not be found. For example, an object store did not exist but was being opened.", NS_ERROR_DOM_INDEXEDDB_NOT_FOUND_ERR)
> +DOM4_MSG_DEF(InvalidStateError, "A mutation operation was attempted on a database that did not allow mutations.", NS_ERROR_DOM_INDEXEDDB_NOT_ALLOWED_ERR)

These should use the NS_ERROR_DOM_* ones
Comment on attachment 613155 [details] [diff] [review]
Tests fix

Review of attachment 613155 [details] [diff] [review]:
-----------------------------------------------------------------

I'd love to see tests for the fact that IDB exceptions that are defined in DOM4 also have a correct .code.
(In reply to Ms2ger from comment #6)
> These should use the NS_ERROR_DOM_* ones
I want to define different error messages for different modules. For example, I don't think "Node was not found" is an appropriate message for File and IndexedDB.
Attached patch Implement IDBRequest.error, v2 (obsolete) — Splinter Review
Fixed nits
Attachment #613153 - Attachment is obsolete: true
Attachment #613187 - Flags: review?(bent.mozilla)
Attachment #613153 - Flags: review?(bent.mozilla)
Fixed ToString(). NS_ERROR_GET_CODE(nsresult) will not get a corrent error code anymore.
Attachment #613154 - Attachment is obsolete: true
Attachment #613188 - Flags: review?(jonas)
Attachment #613154 - Flags: review?(jonas)
Attached patch Tests fix, v2 (obsolete) — Splinter Review
Added .code checks
Attachment #613155 - Attachment is obsolete: true
Attachment #613189 - Flags: review?(bent.mozilla)
Attachment #613155 - Flags: review?(bent.mozilla)
Blocks: 743574
Comment on attachment 613188 [details] [diff] [review]
Replace IDBDatabaseException and FileException with DOMException, v2

Review of attachment 613188 [details] [diff] [review]:
-----------------------------------------------------------------

First off, as always, this is awesome stuff!

However, there are a few things that aren't entirely correct.

In several places we should per spec actually be throwing a JS-style TypeError. I.e. not a DOMException but rather a plain old TypeError. I don't think that XPConnect is able to do that for us, so we need to either do that as a followup bug, or fix that here (followup might be better, but up to you).

But this means that we should return some consistent nsresult in all places where we want to return a TypeError, so that XPConnect can detect this nsresult value and create a TypeError. In the IndexedDB code we have tried to use NS_ERROR_DOM_INDEXEDDB_NON_TRANSIENT_ERR as a "we should throw a TypeError here" value, though I think we haven't been 100% consistent. Since you are removing NS_ERROR_DOM_INDEXEDDB_NON_TRANSIENT_ERR we could maybe use create a new NS_ERROR_TYPE_ERR instead. If you do do this, can you change IDBCursor::Advance to return NS_ERROR_TYPE_ERROR

Let me know if this makes sense?

So r+=me if you create a new code so that we don't loose track of where we should be throwing a TypeError. Even if actually making us throw TypeError is done in a separate bug.

::: dom/base/nsDOMException.cpp
@@ +200,5 @@
>  
>    return NS_OK;
>  }
>  
> +IMPL_INTERNAL_DOM_EXCEPTION_HEAD(SVGException)

We should get rid of SVGException, but that's definitely controversial enough that it needs a separate bug.

@@ +213,5 @@
>  
>    return NS_OK;
>  }
>  
> +IMPL_INTERNAL_DOM_EXCEPTION_HEAD(XPathException)

We should really get rid of XPathException. Mind filing a followup?

For NS_ERROR_DOM_INVALID_EXPRESSION_ERR we should just use SyntaxError and for NS_ERROR_DOM_TYPE_ERR we should use a built-in TypeError (i.e. something like a new NS_ERROR_TYPE_ERR)

::: dom/indexedDB/IDBCursor.cpp
@@ +258,5 @@
>    else if (aDirection.EqualsLiteral("prevunique")) {
>      *aResult = PREV_UNIQUE;
>    }
>    else {
> +    return NS_ERROR_DOM_SYNTAX_ERR;

This should use the new NS_ERROR_TYPE_ERROR

::: dom/indexedDB/IDBDatabase.cpp
@@ +552,5 @@
>      if (aMode.EqualsLiteral("readwrite")) {
>        transactionMode = IDBTransaction::READ_WRITE;
>      }
>      else if (!aMode.EqualsLiteral("readonly")) {
> +      return NS_ERROR_DOM_SYNTAX_ERR;

This should use the new NS_ERROR_TYPE_ERROR

::: dom/indexedDB/IDBFactory.cpp
@@ +511,5 @@
>                   PRUint8 aArgc,
>                   nsIIDBOpenDBRequest** _retval)
>  {
>    if (aVersion < 1 && aArgc) {
> +    return NS_ERROR_XPC_NOT_ENOUGH_ARGS;

This should use the new NS_ERROR_TYPE_ERROR
Attachment #613188 - Flags: review?(jonas) → review+
Comment on attachment 613187 [details] [diff] [review]
Implement IDBRequest.error, v2

Review of attachment 613187 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMError.cpp
@@ +34,5 @@
> +    return nsnull;
> +  }
> +  nsString nameStr;
> +  nameStr.AssignASCII(name);
> +  return CreateWithName(nameStr);

I think you can just do:

return CreateWithName(NS_ConvertASCIItoUTF16(name));
Attachment #613187 - Flags: review?(bent.mozilla) → review+
Comment on attachment 613189 [details] [diff] [review]
Tests fix, v2

Review of attachment 613189 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/test/helpers.js
@@ +82,5 @@
>  ExpectError.prototype = {
>    handleEvent: function(event)
>    {
>      is(event.type, "error", "Got an error event");
> +    is(this._name, event.target.error.name, "Expected error was thrown.");

Would you mind switching the first two arguments around. The first argument should be the received value, and the second argument should be the expected value.
Attachment #613189 - Flags: review?(bent.mozilla) → review+
Files bug 743887 bug 743888 and for followups.
(In reply to Jonas Sicking (:sicking) from comment #12)
> TypeError

FWIW, I think most of those should be fixed in the new DOM bindings, so it's not particularly useful to put a lot of work into it now.
Attachment #613187 - Attachment is obsolete: true
Attachment #614001 - Flags: review+
Attachment #613189 - Attachment is obsolete: true
Attachment #614003 - Flags: review+
Keywords: checkin-needed
Backed out whole merge for bustage per Yoric (Bug 743574):

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e42fb8e321
Target Milestone: mozilla14 → ---
Disregard that; PEBKAC. Did not get backed out. I misread TBPL.
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Depends on: 744910
https://developer.mozilla.org/en-US/docs/IndexedDB/IDBDatabaseException
Added obsolete banner and updated compatibility table.
DOMFileException wasn't documented in the first place according to search.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.