[webkit-reviews] review granted: [Bug 173998] Ensure backward compatibility in the serialization format of structured clones : [Attachment 316661] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jiewen Tan <jiewen_tan at apple.com>'s
request for review:
Bug 173998: Ensure backward compatibility in the serialization format of
structured clones
https://bugs.webkit.org/show_bug.cgi?id=173998

Attachment 316661: Patch

https://bugs.webkit.org/attachment.cgi?id=316661&action=review




--- Comment #15 from Darin Adler <darin at apple.com> ---
Comment on attachment 316661
  --> https://bugs.webkit.org/attachment.cgi?id=316661
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316661&action=review

Great idea. Do these tests cover enough?

> Source/WebCore/ChangeLog:3
> +	   Ensure backward compatibility in the serialization format of
structured clones

It’s great to add a few backward compatibility tests and add a comment
reminding people not to break compatibility and pointing them at the tests.

However, I personally would not call that “ensuring backward compatibility”.

I would write something more like “add tests to help us detect mistakes in
backward compatibility in the future when we change the structured clone
algorithm, since it’s used for data stored in persistent databases”.

> Source/WebCore/ChangeLog:8
> +	   No changes of behavior.

Don’t need this comment. It’s clear that just adding a comment does not change
behavior. Please just delete this line.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:120
> +// When making changes to this list please cover your new type(s) in the API
test "IndexedDB.BackwardCompatibility"

This comment is good but insufficient.

After all, it’s not just changes to this list that can break compatibility, but
rather any changes at all.

Also, the key thing to mention is that some serialized values are stored in a
persistent database, which is *why* we need compatibility. I also am unclear on
how we will handle migration. Do we only need to be able to read old serialized
data, or will we ever share an IndexedDB database between an older and newer
version of WebKit? If we might be sharing one, then we need compatibility in
both directions. Not only do we need to be able to read old data, but the old
version needs to be able to safely read data from the new version without
crashing and has to have some strategy for dealing with data it does not
understand.

This comment about the tag is the smaller issue. Really any change to any of
the serialization code in this entire file has to consider compatibility.

Or maybe not. If types in IndexedDB are limited, then only a part of the code
in this file depends on that. It would be good to draw a clear distinction
between the part that has to be compatible and the part that does not.