Closed
Bug 765834
Opened 12 years ago
Closed 12 years ago
Rework and unify keypath handling
Categories
(Core :: Storage: IndexedDB, defect)
Core
Storage: IndexedDB
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: khuey, Assigned: khuey)
References
Details
Attachments
(2 files, 1 obsolete file)
69.96 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
Our keypath handling is a mess. This puts it all in one place.
Attachment #634105 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 1•12 years ago
|
||
Fixed an assertion that was firing in certain situations.
Attachment #634105 -
Attachment is obsolete: true
Attachment #634105 -
Flags: review?(bent.mozilla)
Attachment #634131 -
Flags: review?(bent.mozilla)
Comment on attachment 634131 [details] [diff] [review] Patch Review of attachment 634131 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/DatabaseInfo.h @@ +18,5 @@ > > BEGIN_INDEXEDDB_NAMESPACE > > class IndexedDBDatabaseChild; > +class KeyPath; Nit: nuke ::: dom/indexedDB/IDBCursor.cpp @@ +715,5 @@ > > return mObjectStore->Put(aValue, JSVAL_VOID, aCx, 0, _retval); > } > > + jsval keyVal = JSVAL_VOID; Revert. ::: dom/indexedDB/IDBDatabase.cpp @@ +483,5 @@ > } > > + // We need a default value here, which the XPIDL dictionary stuff doesn't > + // support. WebIDL shall save us all! > + JSBool hasProp = JS_FALSE; 'false' @@ +486,5 @@ > + // support. WebIDL shall save us all! > + JSBool hasProp = JS_FALSE; > + JSObject* obj = JSVAL_TO_OBJECT(aOptions); > + if (!JS_HasProperty(aCx, obj, "keyPath", &hasProp)) { > + // Wtf? No. ::: dom/indexedDB/IDBFactory.cpp @@ +236,5 @@ > > PRInt32 columnType; > nsresult rv = stmt->GetTypeOfIndex(2, &columnType); > NS_ENSURE_SUCCESS(rv, rv); > + // NB: We don't have to handle the NULL case, since that is the default Nit: extra line above please @@ +299,2 @@ > > + indexInfo->keyPath = KeyPath::DeserializeFromString(keyPathSerialization); Please assert that we have a non-empty keypath here. ::: dom/indexedDB/IDBObjectStore.cpp @@ +50,5 @@ > namespace { > > +inline > +bool > +IgnoreWhitespace(PRUnichar c) Let's rename to 'IgnoreNothing' @@ +563,1 @@ > JSClass gDummyPropClass = { Let's make this a static const member of IDBObjectStore. @@ +646,1 @@ > } Missing return here. (!!!) @@ +650,3 @@ > NS_ENSURE_SUCCESS(rv, rv); > > + if (aMultiEntry && !JSVAL_IS_PRIMITIVE(val) && aMultiEntry is unnecessary now @@ +1303,5 @@ > > +struct GetAddInfoClosure > +{ > + IDBObjectStore* mThis; > + StructuredCloneWriteInfo* mCloneWriteInfo; Ref would look better. @@ +1308,5 @@ > + jsval mValue; > +}; > + > +static nsresult > +GetAddInfoCallback(JSContext* aCx, void* aClosure) Let's move this to the anonymous namespace, then lose 'static'. Same for the struct. @@ +1311,5 @@ > +static nsresult > +GetAddInfoCallback(JSContext* aCx, void* aClosure) > +{ > + GetAddInfoClosure* data = > + static_cast<GetAddInfoClosure*>(aClosure); One line? @@ +1350,5 @@ > return rv; > } > } > else if (!mAutoIncrement) { > + nsresult rv = GetKeyPath().ExtractKey(aCx, aValue, aKey); Nit: Already have an rv somewhere above @@ +1352,5 @@ > } > else if (!mAutoIncrement) { > + nsresult rv = GetKeyPath().ExtractKey(aCx, aValue, aKey); > + if (NS_FAILED(rv)) { > + return rv; Nit: Some kind of indent problem @@ +1378,5 @@ > nsString targetObjectPropName; > JSObject* targetObject = nsnull; > > + GetAddInfoClosure data; > + data.mThis = this; If it compiles let's use { } notation? ::: dom/indexedDB/KeyPath.cpp @@ +72,5 @@ > +nsresult > +GetJSValFromKeyPathString(JSContext* aCx, > + const JS::Value& aValue, > + const nsAString& aKeyPathString, > + JS::Value& aKeyJSVal, Let's make this a jsval* @@ +125,5 @@ > + // Treat explicitly undefined as an error. > + if (intermediate == JSVAL_VOID) { > + return NS_ERROR_DOM_INDEXEDDB_DATA_ERR; > + } > + else if (tokenizer.hasMoreTokens()) { Nit: else after return @@ +138,5 @@ > + aKeyJSVal = intermediate; > + } > + } > + > + if (!hasProp) { Nit: else @@ +214,5 @@ > + NS_ENSURE_TRUE(JSVAL_TO_BOOLEAN(succeeded), > + NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > + } > + > + return NS_OK; Return rv? And warn if failure @@ +219,5 @@ > +} > + > +} // anonymous namespace > + > +/* static */ // @@ +221,5 @@ > +} // anonymous namespace > + > +/* static */ > +bool > +KeyPath::Parse(JSContext* aCx, const JS::Value& aValue, KeyPath& aKeyPath) Nit: each arg on its own line @@ +225,5 @@ > +KeyPath::Parse(JSContext* aCx, const JS::Value& aValue, KeyPath& aKeyPath) > +{ > + KeyPath keyPath; > + > + aKeyPath = keyPath; aKeyPath.SetType(Invalid) @@ +249,5 @@ > + jsval val; > + JSString* jsstr; > + nsDependentJSString str; > + if (!JS_GetElement(aCx, obj, index, &val) || > + !(jsstr = JS_ValueToString(aCx, val)) || Let's double check that this is right. @@ +260,5 @@ > + } > + } > + } > + // Otherwise convert it to a string. > + else if (aValue != JSVAL_NULL) { !JSVAL_IS_NULL. And what about undefined? Nit: Please move comment inside if block. @@ +283,5 @@ > +void > +KeyPath::SetType(KeyPathType aType) > +{ > + NS_ASSERTION(aType == String || aType == Array, "Invalid type!"); > + mType = aType; Make sure to clear mStrings @@ +295,5 @@ > + } > + > + if (IsString()) { > + NS_ASSERTION(mStrings.Length() == 0, "Too many strings!"); > + return !!mStrings.AppendElement(aString); Infallible now @@ +299,5 @@ > + return !!mStrings.AppendElement(aString); > + } > + > + if (IsArray()) { > + return !!mStrings.AppendElement(aString); And here @@ +318,5 @@ > + for (PRUint32 i = 0; i < len; ++i) { > + nsresult rv = GetJSValFromKeyPathString(aCx, aValue, mStrings[i], value, > + DoNotCreateProperties, nsnull, > + nsnull); > + NS_ENSURE_SUCCESS(rv, rv); I think we don't want the warning here. @@ +333,5 @@ > +} > + > +nsresult > +KeyPath::ExtractKeyAsJSVal(JSContext* aCx, const JS::Value& aValue, > + JS::Value& aOutVal) const Let's replace this with a call to ExtractKey() followed by ToJSVal(). @@ +378,5 @@ > + > + nsresult rv = GetJSValFromKeyPathString(aCx, aValue, mStrings[0], value, > + CreateProperties, aCallback, > + aClosure); > + NS_ENSURE_SUCCESS(rv, rv); No warning here. @@ +382,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + if (NS_FAILED(aKey.AppendItem(aCx, false, value))) { > + NS_ASSERTION(aKey.IsUnset(), "Should be unset"); > + return value == JSVAL_VOID ? NS_OK : NS_ERROR_DOM_INDEXEDDB_DATA_ERR; JSVAL_IS_VOID @@ +391,5 @@ > + return NS_OK; > +} > + > +bool > +KeyPath::operator==(const KeyPath& aOther) const inline in header, 'return mType == && mStrings ==' @@ +408,5 @@ > + return true; > +} > + > +nsString > +KeyPath::SerializeToString() const All our other string methods take outparams... Let's do that here. @@ +435,5 @@ > + NS_NOTREACHED("What?"); > + return EmptyString(); > +} > + > +/* static */ // @@ +442,5 @@ > +{ > + KeyPath keyPath; > + > + if (!aString.IsEmpty() && aString.First() == ',') { > + keyPath.mType = Array; Should be SetType() @@ +456,5 @@ > + > + return keyPath; > + } > + > + keyPath.mType = String; SetType() @@ +516,5 @@ > + return false; > + } > + > + // Neither are empty strings. > + if (IsString() && mStrings[0] == EmptyString()) { mStrings[0].IsEmpty() ::: dom/indexedDB/KeyPath.h @@ +23,5 @@ > +{ > + template<class T> friend struct IPC::ParamTraits; > + > + enum KeyPathType { > + Invalid, Nonexistant @@ +26,5 @@ > + enum KeyPathType { > + Invalid, > + String, > + Array, > + EndGuard Nit: all caps to match the rest @@ +31,5 @@ > + }; > + > + void SetType(KeyPathType aType); > + > + bool AppendStringWithValidation(JSContext* aCx, const nsAString& aString); NIt: comment about not throwing @@ +35,5 @@ > + bool AppendStringWithValidation(JSContext* aCx, const nsAString& aString); > + > +public: > + KeyPath() > + : mType(Invalid) Please make this take an arg of some kind so we can find bugs easier. You can have a private no-arg constructor, but the public one should require something. @@ +44,5 @@ > + KeyPath(const KeyPath& aOther) > + : mType(aOther.mType), > + mStrings(aOther.mStrings) > + { > + MOZ_COUNT_CTOR(KeyPath); *this = aOther. @@ +53,5 @@ > + MOZ_COUNT_DTOR(KeyPath); > + } > + > + static bool > + Parse(JSContext* aCx, const JS::Value& aValue, KeyPath& aKeyPath); Let's do this as an nsresult @@ +64,5 @@ > + JS::Value& aOutVal) const; > + > + typedef nsresult > + (*ExtractOrCreateKeyCallback)(JSContext* aCx, > + void* aClosure); Nit: can fit on one line @@ +71,5 @@ > + ExtractOrCreateKey(JSContext* aCx, const JS::Value& aValue, Key& aKey, > + ExtractOrCreateKeyCallback aCallback, > + void* aClosure) const; > + > + inline bool IsValid() const { Let's nuke the 'inline' here and below @@ +95,5 @@ > + > + nsresult ToJSVal(JSContext* aCx, JS::Value* aValue) const; > + > + bool IsAllowedForObjectStore(bool aAutoIncrement) const; > +private: Nit: extra newline above here ::: dom/indexedDB/ipc/IndexedDBParent.cpp @@ +959,5 @@ > > const CreateIndexParams& params = aParams.get_CreateIndexParams(); > const IndexInfo& info = params.info(); > > // Copy... No longer needed ::: dom/indexedDB/ipc/SerializationHelpers.h @@ +42,5 @@ > + mozilla::dom::indexedDB::KeyPath::EndGuard> > +{ }; > + > +template <> > +struct ParamTraits<mozilla::dom::indexedDB::KeyPath> Please add KeyPath.h to includes here.
Attachment #634131 -
Flags: review?(bent.mozilla) → review+
Comment 3•12 years ago
|
||
There's a leading utf-8 vomit at the beginning of first line of keypath.h and keypath.cpp.. that chokes gcc here. dom/indexedDB/Key.cpp: ASCII C program text dom/indexedDB/Key.h: ASCII C program text dom/indexedDB/KeyPath.cpp: UTF-8 Unicode (with BOM) C program text dom/indexedDB/KeyPath.h: UTF-8 Unicode (with BOM) C program text ../../dist/include/mozilla/dom/indexedDB/KeyPath.h:1: error: stray '\357' in program ../../dist/include/mozilla/dom/indexedDB/KeyPath.h:1: error: stray '\273' in program ../../dist/include/mozilla/dom/indexedDB/KeyPath.h:1: error: stray '\277' in program First line look like : /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
Comment 4•12 years ago
|
||
Attachment #636155 -
Flags: review?(khuey)
Assignee | ||
Updated•12 years ago
|
Attachment #636155 -
Flags: review?(khuey) → review+
Comment 5•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/4d17545dba03 for the BOM removal (http://hg.mozilla.org/mozilla-central/rev/f65133bc5723 for the initial commit)
Assignee | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f65133bc5723 https://hg.mozilla.org/mozilla-central/rev/0b7e8d848bff
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in
before you can comment on or make changes to this bug.
Description
•