Last Comment Bug 730161 - Remove DOMFileException and IDBDatabaseException
: Remove DOMFileException and IDBDatabaseException
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
: 530212 693061 (view as bug list)
Depends on: 744910
Blocks: 743574
  Show dependency treegraph
 
Reported: 2012-02-23 16:45 PST by Masatoshi Kimura [:emk]
Modified: 2013-09-08 21:49 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement IDBRequest.error (9.00 KB, patch)
2012-04-07 20:18 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Replace IDBDatabaseException and FileException with DOMException (50.36 KB, patch)
2012-04-07 20:20 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Tests fix (38.24 KB, patch)
2012-04-07 20:20 PDT, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Implement IDBRequest.error, v2 (9.02 KB, patch)
2012-04-08 10:44 PDT, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Splinter Review
Replace IDBDatabaseException and FileException with DOMException, v2 (53.24 KB, patch)
2012-04-08 10:46 PDT, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Splinter Review
Tests fix, v2 (39.25 KB, patch)
2012-04-08 10:47 PDT, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Splinter Review
Implement IDBRequest.error. r=sicking (9.00 KB, patch)
2012-04-11 08:05 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Replace IDBDatabaseException and FileException with DOMException. r=sicking (54.43 KB, patch)
2012-04-11 08:06 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Tests fix. r=sicking (39.26 KB, patch)
2012-04-11 08:06 PDT, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2012-02-23 16:45:51 PST
These exception types are no longer defined in the latest spec.
Comment 1 Masatoshi Kimura [:emk] 2012-04-07 20:18:59 PDT
Created attachment 613153 [details] [diff] [review]
Implement IDBRequest.error

This is required because code is useless for indexedDB errors.
Comment 2 Masatoshi Kimura [:emk] 2012-04-07 20:20:13 PDT
Created attachment 613154 [details] [diff] [review]
Replace IDBDatabaseException and FileException with DOMException
Comment 3 Masatoshi Kimura [:emk] 2012-04-07 20:20:51 PDT
Created attachment 613155 [details] [diff] [review]
Tests fix
Comment 4 Masatoshi Kimura [:emk] 2012-04-07 20:34:16 PDT
https://tbpl.mozilla.org/?tree=Try&rev=befe95e6c20c
Comment 5 :Ms2ger (⌚ UTC+1/+2) 2012-04-08 08:49:06 PDT
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 6 :Ms2ger (⌚ UTC+1/+2) 2012-04-08 08:55:44 PDT
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 7 :Ms2ger (⌚ UTC+1/+2) 2012-04-08 08:57:44 PDT
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.
Comment 8 Masatoshi Kimura [:emk] 2012-04-08 09:01:23 PDT
(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.
Comment 9 Masatoshi Kimura [:emk] 2012-04-08 10:44:32 PDT
Created attachment 613187 [details] [diff] [review]
Implement IDBRequest.error, v2

Fixed nits
Comment 10 Masatoshi Kimura [:emk] 2012-04-08 10:46:22 PDT
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.
Comment 11 Masatoshi Kimura [:emk] 2012-04-08 10:47:16 PDT
Created attachment 613189 [details] [diff] [review]
Tests fix, v2

Added .code checks
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-09 14:25:04 PDT
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
Comment 13 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-09 14:31:34 PDT
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));
Comment 14 Jonas Sicking (:sicking) PTO Until July 5th 2012-04-09 14:47:10 PDT
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.
Comment 15 Masatoshi Kimura [:emk] 2012-04-09 18:10:23 PDT
Files bug 743887 bug 743888 and for followups.
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2012-04-10 01:31:03 PDT
(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.
Comment 17 Masatoshi Kimura [:emk] 2012-04-11 08:05:39 PDT
Created attachment 614001 [details] [diff] [review]
Implement IDBRequest.error. r=sicking
Comment 18 Masatoshi Kimura [:emk] 2012-04-11 08:06:14 PDT
Created attachment 614002 [details] [diff] [review]
Replace IDBDatabaseException and FileException with DOMException. r=sicking
Comment 19 Masatoshi Kimura [:emk] 2012-04-11 08:06:48 PDT
Created attachment 614003 [details] [diff] [review]
Tests fix. r=sicking
Comment 21 Richard Newman [:rnewman] 2012-04-11 15:38:29 PDT
Backed out whole merge for bustage per Yoric (Bug 743574):

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e42fb8e321
Comment 22 Richard Newman [:rnewman] 2012-04-11 17:55:51 PDT
Disregard that; PEBKAC. Did not get backed out. I misread TBPL.
Comment 24 Matthew N. [:MattN] 2012-08-04 13:46:08 PDT
*** Bug 693061 has been marked as a duplicate of this bug. ***
Comment 25 Matthew N. [:MattN] 2012-08-04 13:50:58 PDT
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.
Comment 26 Masatoshi Kimura [:emk] 2013-09-08 21:49:02 PDT
*** Bug 530212 has been marked as a duplicate of this bug. ***

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