Remove DOMFileException and IDBDatabaseException

RESOLVED FIXED in mozilla14

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

({dev-doc-complete})

Trunk
mozilla14
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
These exception types are no longer defined in the latest spec.
(Assignee)

Comment 1

5 years ago
Created attachment 613153 [details] [diff] [review]
Implement IDBRequest.error

This is required because code is useless for indexedDB errors.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #613153 - Flags: review?(bent.mozilla)
(Assignee)

Comment 2

5 years ago
Created attachment 613154 [details] [diff] [review]
Replace IDBDatabaseException and FileException with DOMException
Attachment #613154 - Flags: review?(jonas)
(Assignee)

Comment 3

5 years ago
Created attachment 613155 [details] [diff] [review]
Tests fix
Attachment #613155 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

5 years ago
Attachment #613155 - Attachment is patch: true
(Assignee)

Comment 4

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=befe95e6c20c
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.
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
Created attachment 613187 [details] [diff] [review]
Implement IDBRequest.error, v2

Fixed nits
Attachment #613153 - Attachment is obsolete: true
Attachment #613187 - Flags: review?(bent.mozilla)
Attachment #613153 - Flags: review?(bent.mozilla)
(Assignee)

Comment 10

5 years ago
Created attachment 613188 [details] [diff] [review]
Replace IDBDatabaseException and FileException with DOMException, v2

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)
(Assignee)

Comment 11

5 years ago
Created attachment 613189 [details] [diff] [review]
Tests fix, v2

Added .code checks
Attachment #613155 - Attachment is obsolete: true
Attachment #613189 - Flags: review?(bent.mozilla)
Attachment #613155 - Flags: review?(bent.mozilla)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 15

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Created attachment 614001 [details] [diff] [review]
Implement IDBRequest.error. r=sicking
Attachment #613187 - Attachment is obsolete: true
Attachment #614001 - Flags: review+
(Assignee)

Comment 18

5 years ago
Created attachment 614002 [details] [diff] [review]
Replace IDBDatabaseException and FileException with DOMException. r=sicking
Attachment #613188 - Attachment is obsolete: true
Attachment #614002 - Flags: review+
(Assignee)

Comment 19

5 years ago
Created attachment 614003 [details] [diff] [review]
Tests fix. r=sicking
Attachment #613189 - Attachment is obsolete: true
Attachment #614003 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/37e9c8655cfd
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b27921b99eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/56262a68406b
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Backed out whole merge for bustage per Yoric (Bug 743574):

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e42fb8e321
Target Milestone: mozilla14 → ---
Keywords: checkin-needed
Disregard that; PEBKAC. Did not get backed out. I misread TBPL.
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/37e9c8655cfd
https://hg.mozilla.org/mozilla-central/rev/3b27921b99eb
https://hg.mozilla.org/mozilla-central/rev/56262a68406b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 744910
Duplicate of this bug: 693061
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.
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

4 years ago
Duplicate of this bug: 530212
You need to log in before you can comment on or make changes to this bug.