Closed
Bug 667652
Opened 13 years ago
Closed 13 years ago
Make the fix for Bug 663479 less hacky
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: khuey, Assigned: khuey)
References
Details
Attachments
(2 files, 1 obsolete file)
1.70 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
15.89 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•13 years ago
|
||
Attachment #542305 -
Flags: review?(jorendorff)
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #542307 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 3•13 years ago
|
||
This implements Jonas's idea that instead of manually trawling through the clone buffer we store the offset and write directly to that offset later.
Comment 4•13 years ago
|
||
Comment on attachment 542305 [details] [diff] [review] Add a JS_FRIEND_API to get the current position in the buffer namespace, pretty please with a cherry on top?
(In reply to comment #4) > namespace, pretty please with a cherry on top? That's not how the public/friend api works.
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to comment #5) > (In reply to comment #4) > > namespace, pretty please with a cherry on top? > > That's not how the public/friend api works. Right, which is why I didn't do that.
Comment 7•13 years ago
|
||
Comment on attachment 542305 [details] [diff] [review] Add a JS_FRIEND_API to get the current position in the buffer Sure, ok.
Attachment #542305 -
Flags: review?(jorendorff) → review+
Shouldn't you be using writeDouble rather than writeBytes?
Comment on attachment 542307 [details] [diff] [review] Change IDB code to be less hacky Review of attachment 542307 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBObjectStore.cpp @@ +76,5 @@ > JSAutoStructuredCloneBuffer& aCloneBuffer, > const Key& aKey, > bool aOverwrite, > + nsTArray<IndexUpdateInfo>& aIndexUpdateInfo, > + PRUint64 aOffset) Nit: This should be more descriptive. Offset to what? @@ +112,5 @@ > JSAutoStructuredCloneBuffer mCloneBuffer; > Key mKey; > const bool mOverwrite; > nsTArray<IndexUpdateInfo> mIndexUpdateInfo; > + PRUint64 mOffset; Same here. @@ +897,5 @@ > > nsresult > IDBObjectStore::ModifyValueForNewKey(JSAutoStructuredCloneBuffer& aBuffer, > + Key& aKey, > + PRUint64 aOffset) Same here. @@ +932,5 @@ > +StructuredCloneWriteDummyProp(JSContext* aCx, > + JSStructuredCloneWriter* aWriter, > + JSObject* aObj, > + void* aClosure) > +{ Need to only do this for objects of your special class. Right now this hook will run for *any* unrecognized object, like security wrappers, canvas things, DOM elements, etc. You'll want to check based on JS_GET_CLASS(aCx, aObj) == &gDummyPropClass. @@ +940,5 @@ > + *closure = js_GetSCOffset(aWriter); > + > + PRUint64 value = 0; > + return JS_WriteBytes(aWriter, &value, sizeof(value)); > +} You need to call the main runtime's hooks if you didn't handle this. Otherwise you may miss globally understood types. Also, if JS_WriteBytes fails you need to set a CLONE_ERR exception. @@ +947,5 @@ > +{ "dummy", 0, > + JS_PropertyStub, JS_PropertyStub, > + JS_PropertyStub, JS_StrictPropertyStub, > + JS_EnumerateStub, JS_ResolveStub, > + JS_ConvertStub, nsnull }; Please add these to the anonymous namespace, then drop the static on sDummyPropClass, then s/sDummy/gDummy/. Also swap the order so that you can access gDummyPropClass from the write hook. @@ +1008,5 @@ > if (!mKeyPath.IsEmpty() && aKey.IsUnset()) { > NS_ASSERTION(mAutoIncrement, "Should have bailed earlier!"); > > + JSObject* obj = JS_NewObject(aCx, &sDummyPropClass, nsnull, nsnull); > + NS_ENSURE_TRUE(obj, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); Nit: newline here.
Assignee | ||
Comment 10•13 years ago
|
||
Review comments addressed.
Attachment #542307 -
Attachment is obsolete: true
Attachment #542836 -
Flags: review?(bent.mozilla)
Attachment #542307 -
Flags: review?(bent.mozilla)
Comment on attachment 542836 [details] [diff] [review] Patch Review of attachment 542836 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these addressed too. Thanks! ::: dom/indexedDB/IDBObjectStore.cpp @@ +430,5 @@ > +{ "dummy", 0, > + JS_PropertyStub, JS_PropertyStub, > + JS_PropertyStub, JS_StrictPropertyStub, > + JS_EnumerateStub, JS_ResolveStub, > + JS_ConvertStub, nsnull }; Oops, missed this before, use JSCLASS_NO_OPTIONAL_MEMBERS rather than 'nsnull' at the end there. Nit: Put the closing brace on its own line at 0 offset. @@ +935,5 @@ > NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > } > > +JSBool > +StructuredCloneWriteDummyProp(JSContext* aCx, Please move this into the anonymous namespace too. @@ +950,5 @@ > + PRUint64 value = 0; > + return JS_WriteBytes(aWriter, &value, sizeof(value)); > + } > + > + // Something failed above, try using the runtime callbacks instead. Nit: This comment isn't accurate ("something failed above"), just remove that clause. @@ +968,5 @@ > jsval aKeyVal, > JSAutoStructuredCloneBuffer& aCloneBuffer, > Key& aKey, > + nsTArray<IndexUpdateInfo>& aUpdateInfoArray, > + PRUint64& aOffsetToKeyProp) I'd prefer not to use a reference here, let's use a pointer instead. That way you can see at the call site that it's an out param. @@ +1035,5 @@ > > + JSStructuredCloneCallbacks callbacks = > + { nsnull, /* no special reading */ > + StructuredCloneWriteDummyProp, > + nsnull /* no special error handling */ }; Nit: JSSCC callbacks = { nsnull, SCWDP, nsnull };
Attachment #542836 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 12•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/594b9b4cb1df http://hg.mozilla.org/mozilla-central/rev/b2a1b2fd6d23
Status: NEW → RESOLVED
Closed: 13 years ago
OS: Windows 7 → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Version: unspecified → Trunk
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•