1213024 - Comment the structured clone reading and writing

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

I have to wrap my brain in knots all over again whenever I read it, and I just screwed up a review because I didn't take the time to remember how all this worked.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Comment the structured clone reading and writing

These are the comments I wished I had had when looking at this stuff today. I really ought to comment ::write() too, but that'll be for another day.

Don't worry about bitrotting this patch. I will happily rebase it after your SavedFrame cloning lands.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8671569 [details] [diff] [review]
Comment the structured clone reading and writing

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

Very helpful!

::: js/src/vm/StructuredClone.cpp
@@ +1913,5 @@
>  {
>      if (!readTransferMap())
>          return false;
>  
> +    // Start out by reading in the main object (probably an Array) and pushing

I think you can get rid of "(probably an Array)"; it's probably wrong (ha; at least in my experience, I'm structured cloning objects) and is a bit irrelevant either way.

@@ +1948,5 @@
> +        // loop and beginning to process the child obj.
> +        //
> +        // Note that this means the ordering in the stream is a little funky
> +        // for things like Map. See the comment above startWrite() for an
> +        // example.

This is already explained a little bit above `entries` in the JSStructuredCloneReader definition. I'd suggest either expanding that existing comment where appropriate and pointing to it from here, or move everything here and point to this comment from there. The latter might be nicer since this is where the action is.

Comment 3

[Pulsebot]

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

Comment 4

[Carsten Book [:Tomcat]]

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