Closed Bug 961286 Opened 6 years ago Closed 6 years ago

Use move semantics for JSAutoStructuredCloneBuffer and wrappers

Categories

(Core :: Storage: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: khuey, Assigned: khuey)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
We don't ever really want to swap it anyways.  This fixes up IDB, although there are other opportunities to take advantage of this in workers and probably elsewhere.
Attachment #8361984 - Flags: review?(jorendorff)
Attachment #8361984 - Flags: review?(bent.mozilla)
Attached patch Patch (obsolete) — Splinter Review
Now with less Forward<T>
Attachment #8361984 - Attachment is obsolete: true
Attachment #8361984 - Flags: review?(jorendorff)
Attachment #8361984 - Flags: review?(bent.mozilla)
Attachment #8363121 - Flags: review?(jorendorff)
Attachment #8363121 - Flags: review?(bent.mozilla)
Comment on attachment 8363121 [details] [diff] [review]
Patch

Review of attachment 8363121 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great. I focused on the changes in StructuredClone.{h,cpp}.

::: js/src/vm/StructuredClone.cpp
@@ +1657,5 @@
> +JSAutoStructuredCloneBuffer&
> +JSAutoStructuredCloneBuffer::operator=(JSAutoStructuredCloneBuffer &&other)
> +{
> +    clear();
> +    other.steal(&data_, &nbytes_, &version_);

Please either assert that `&other != this` or make sure this is a no-op in that silly special case.
Attachment #8363121 - Flags: review?(jorendorff) → review+
Comment on attachment 8363121 [details] [diff] [review]
Patch

Review of attachment 8363121 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

::: dom/indexedDB/IDBIndex.cpp
@@ +34,5 @@
>  USING_INDEXEDDB_NAMESPACE
>  using namespace mozilla::dom;
>  using namespace mozilla::dom::indexedDB::ipc;
>  using mozilla::ErrorResult;
> +using mozilla::Move;

Hm, don't you need this in the other files too?

::: dom/indexedDB/IndexedDatabase.h
@@ +55,5 @@
>  struct StructuredCloneReadInfo
>  {
>    // In IndexedDatabaseInlines.h
>    inline StructuredCloneReadInfo();
> +  inline StructuredCloneReadInfo&

Nit: Add a newline above.

::: dom/indexedDB/IndexedDatabaseInlines.h
@@ +20,5 @@
>  {
>  }
>  
>  inline
> +StructuredCloneWriteInfo::StructuredCloneWriteInfo(StructuredCloneWriteInfo&& aCloneWriteInfo)

Nit: > 80 chars

@@ +23,5 @@
>  inline
> +StructuredCloneWriteInfo::StructuredCloneWriteInfo(StructuredCloneWriteInfo&& aCloneWriteInfo)
> +: mCloneBuffer(Move(aCloneWriteInfo.mCloneBuffer)),
> +  mTransaction(aCloneWriteInfo.mTransaction),
> +  mOffsetToKeyProp(aCloneWriteInfo.mOffsetToKeyProp)

Nit: ',' prefixes, not suffixes, I guess...

@@ +54,5 @@
>  {
>  }
>  
> +inline StructuredCloneReadInfo&
> +StructuredCloneReadInfo::operator=(StructuredCloneReadInfo&& aCloneReadInfo)

Need assert/protection against 'foo = foo' here.

::: dom/workers/XMLHttpRequest.cpp
@@ +1799,1 @@
>                       aClonedObjects, syncLoopTarget, hasUploadListeners);

Nit: This needs some wrapping help.
Attachment #8363121 - Flags: review?(bent.mozilla) → review+
Attached patch PatchSplinter Review
Review comments addressed.
Attachment #8363121 - Attachment is obsolete: true
Attachment #8368811 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/dffbfd00ac4e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.