961286 - Use move semantics for JSAutoStructuredCloneBuffer and wrappers

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: 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.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Now with less Forward<T>

Comment 2

[Jason Orendorff [:jorendorff]]

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.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Review comments addressed.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dffbfd00ac4e

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dffbfd00ac4e