Use move semantics for JSAutoStructuredCloneBuffer and wrappers

RESOLVED FIXED in mozilla29

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 8361984 [details] [diff] [review]
Patch

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)
Created attachment 8363121 [details] [diff] [review]
Patch

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+
Created attachment 8368811 [details] [diff] [review]
Patch

Review comments addressed.
Attachment #8363121 - Attachment is obsolete: true
Attachment #8368811 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dffbfd00ac4e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.