Closed
Bug 891944
Opened 12 years ago
Closed 11 years ago
Move IDBCursor to WebIDL
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: baku, Assigned: baku)
References
Details
(Keywords: addon-compat, dev-doc-complete, site-compat)
Attachments
(1 file, 3 obsolete files)
37.03 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #776475 -
Flags: review?(Jan.Varga)
Comment 2•11 years ago
|
||
Comment on attachment 776475 [details] [diff] [review]
patch
Review of attachment 776475 [details] [diff] [review]:
-----------------------------------------------------------------
hm, it seems test-indexed-db.js needs to be updated too
you might need |using mozilla::ErrorResult| in IDBCursor.cpp too
::: dom/indexedDB/IDBCursor.cpp
@@ +358,5 @@
> mRooted(false),
> mContinueCalled(false),
> mHaveValue(true)
> {
> NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
Nit: I would add a new line here
@@ +444,5 @@
> NS_NOTREACHED("Unknown cursor type!");
> }
>
> nsresult rv = helper->DispatchToTransactionPool();
> + if (NS_FAILED(rv)) {
ENSURE_SUCCESS()
@@ +458,5 @@
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mRequest)
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTransaction)
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mObjectStore)
> NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIndex)
> + NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
redundant declaration?
@@ +473,5 @@
> NS_IMPL_CYCLE_COLLECTION_TRACE_JS_MEMBER_CALLBACK(mScriptOwner)
> NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mCachedKey)
> NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mCachedPrimaryKey)
> NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK(mCachedValue)
> + NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
Nit: I would place this first
@@ +480,5 @@
> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(IDBCursor)
> // Don't unlink mObjectStore, mIndex, or mTransaction!
> tmp->DropJSObjects();
> NS_IMPL_CYCLE_COLLECTION_UNLINK(mRequest)
> + NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
Nit: I would place this first
@@ +492,5 @@
> NS_IMPL_CYCLE_COLLECTING_ADDREF(IDBCursor)
> NS_IMPL_CYCLE_COLLECTING_RELEASE(IDBCursor)
>
> DOMCI_DATA(IDBCursor, IDBCursor)
> DOMCI_DATA(IDBCursorWithValue, IDBCursor)
hmm, this should be removed too, no?
@@ +501,5 @@
> NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>
> switch (mDirection) {
> case NEXT:
> + return IDBCursorDirection::Next;
fully qualified, here and below?
@@ +515,5 @@
>
> case DIRECTION_INVALID:
> default:
> + aRv.Throw(NS_ERROR_UNEXPECTED);
> + return IDBCursorDirection::Next;
just MOZ_CRASH() ?
@@ +528,5 @@
>
> + nsCOMPtr<nsISupports> source;
> + if (mType == OBJECTSTORE) {
> + source = do_QueryInterface(mObjectStore);
> + } else {
Nit: else on a new line
@@ +553,5 @@
> mRooted = true;
> }
>
> + aRv = mKey.ToJSVal(aCx, mCachedKey);
> + if (aRv.Failed()) {
ENSURE_SUCCESS
@@ +701,4 @@
> }
>
> +already_AddRefed<IDBRequest>
> +IDBCursor::Update(JSContext* aCx, JS::Handle<JS::Value> aValue, ErrorResult& aRv)
Nit: 80 chars max
@@ -874,4 @@
> {
> NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>
> - if (aCount < 1 || aCount > UINT32_MAX) {
are you sure you can remove this entirely ?
the method should throw when the count is zero and [EnforceRange] handles negative and "over the max" values only
you will need to call ThrowTypeError() and add an entry to dom/bindings/Errors.msg
::: dom/indexedDB/IDBCursor.h
@@ +17,2 @@
> #include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"
#include "mozilla/dom/indexedDB/IndexedDatabase.h"
#include "mozilla/Attributes.h"
#include "mozilla/dom/IDBCursorBinding.h"
#include "mozilla/ErrorResult.h"
#include "nsCycleCollectionParticipant.h"
#include "nsWrapperCache.h"
#include "mozilla/dom/indexedDB/IDBObjectStore.h"
#include "mozilla/dom/indexedDB/Key.h"
@@ +33,5 @@
> class IndexedDBCursorChild;
> class IndexedDBCursorParent;
>
> +class IDBCursor MOZ_FINAL : public nsISupports
> + , public nsWrapperCache
Nit: comma after (not before) and trailing whitespace
@@ +146,5 @@
> + void
> + ContinueInternal(const Key& aKey, int32_t aCount,
> + ErrorResult& aRv);
> +
> + // WebIDL
// nsWrapperCache
virtual JSObject*
WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope) MOZ_OVERRIDE;
// WebIDL
IDBTransaction*
GetParentObject() const
{
return mTransaction();
}
...
@@ +148,5 @@
> + ErrorResult& aRv);
> +
> + // WebIDL
> +
> + IDBTransaction* GetParentObject() const
Nit: return value on a separate line, here and elsewhere
@@ +166,5 @@
> + JS::Value GetPrimaryKey(JSContext* aCx, ErrorResult& aRv);
> +
> + already_AddRefed<IDBRequest>
> + Update(JSContext* aCx, JS::Handle<JS::Value> aValue,
> + ErrorResult& aRv);
Nit: one line for arguments is enough
::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +2094,5 @@
> AutoSetCurrentTransaction asct(mCursor->Transaction());
>
> + ErrorResult rv;
> + mCursor->ContinueInternal(aParams.key(), aParams.count(), rv);
> + if (rv.Failed()) {
ENSURE_SUCCESS()
Attachment #776475 -
Flags: review?(Jan.Varga)
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #776475 -
Attachment is obsolete: true
Attachment #783708 -
Flags: review?(Jan.Varga)
Comment 4•11 years ago
|
||
Comment on attachment 783708 [details] [diff] [review]
cursor.patch
Review of attachment 783708 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/bindings/Errors.msg
@@ +43,5 @@
> MSG_DEF(MSG_INVALID_VERSION, 0, "0 (Zero) is not a valid database version.")
> MSG_DEF(MSG_INVALID_BYTESTRING, 2, "Cannot convert string to ByteString because the character"
> " at index {0} has value {1} which is greater than 255.")
> MSG_DEF(MSG_NOT_DATE, 1, "{0} is not a date.")
> +MSG_DEF(MSG_NOT_ZERO, 1, "{0} can't be zero.")
this would probably translate into "0 can't be zero." which is not very useful
what about:
MSG_DEF(MSG_INVALID_ADVANCE_COUNT, 0, "0 (Zero) is not a valid advance count.")
but check with someone else, English is not my mother tongue :)
::: dom/indexedDB/IDBCursor.cpp
@@ +34,2 @@
> USING_INDEXEDDB_NAMESPACE
> +using mozilla::ErrorResult;
Nit: move after |using namespace mozilla::dom::indexedDB::ipc;|
@@ +441,5 @@
> NS_NOTREACHED("Unknown cursor type!");
> }
>
> nsresult rv = helper->DispatchToTransactionPool();
> + if (NS_FAILED(rv)) {
add a warning
@@ +508,5 @@
>
> case DIRECTION_INVALID:
> default:
> + MOZ_CRASH("Unknown direction!");
> + return mozilla::dom::IDBCursorDirection::Next;
It compiles even w/o |return mozilla::dom::IDBCursorDirection::Next;|
on mac at least
@@ +742,5 @@
> }
> else {
> JS::Rooted<JS::Value> keyVal(aCx);
> + aRv = objectKey.ToJSVal(aCx, &keyVal);
> + if (aRv.Failed()) {
ENSURE_SUCCESS
@@ +914,2 @@
> }
>
sorry, forgot to point out that WrapObject() could go before GetDirection()
::: dom/webidl/IDBCursor.webidl
@@ +17,5 @@
> +interface IDBCursor {
> + // This should be: readonly attribute (IDBObjectStore or IDBIndex) source;
> + readonly attribute nsISupports source;
> +
> + [Throws]
hm, I think this doesn't need [Throws] anymore
Attachment #783708 -
Flags: review?(Jan.Varga) → review+
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #783708 -
Attachment is obsolete: true
Attachment #783862 -
Flags: review?(Jan.Varga)
Comment 6•11 years ago
|
||
Comment on attachment 783862 [details] [diff] [review]
cursor.patch
Review of attachment 783862 [details] [diff] [review]:
-----------------------------------------------------------------
ok, I'm done :)
thanks!
::: dom/indexedDB/IDBCursor.cpp
@@ +443,5 @@
> }
>
> nsresult rv = helper->DispatchToTransactionPool();
> + if (NS_FAILED(rv)) {
> + NS_ERROR("Failed to dispatch!");
this should be NS_WARNING
Attachment #783862 -
Flags: review?(Jan.Varga) → review+
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #783862 -
Attachment is obsolete: true
Assignee | ||
Comment 8•11 years ago
|
||
Assignee | ||
Comment 9•11 years ago
|
||
With this patch IDBCursor.NEXT, IDBCursor.NEXT_NO_DUPLICATE, IDBCursor.PREV and IDBCursor.PREV_NO_DUPLICATE are removed completely. We need to update the documentation.
Keywords: dev-doc-needed
Comment 10•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 11•11 years ago
|
||
https://developer.mozilla.org/en-US/docs/Web/API/IDBCursor#Constants
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/25/Site_Compatibility
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•