Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844543002:
SerializedScriptValue: Add a warning about making wire format changes near branch. (Closed)
DescriptionSerializedScriptValue: Add a warning about making wire format changes near branch.
BUG=715232
Review-Url: https://codereview.chromium.org/2844543002
Cr-Commit-Position: refs/heads/master@{#467732}
Committed: https://chromium.googlesource.com/chromium/src/+/f9f2d84e6e7e578e3b7f248184d6564d1f92c43f
Patch Set 1 #
Total comments: 2
Patch Set 2 : update wording #Messages
Total messages: 19 (11 generated)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
jbroman@chromium.org changed reviewers: + jsbell@chromium.org
Similar V8-side change is here: https://codereview.chromium.org/2839903003
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm other that nitpicking over the wording. https://codereview.chromium.org/2844543002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right): https://codereview.chromium.org/2844543002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:84: // WARNING: Increasing this value is a backward-incompatible change. "backward-incompatible" isn't quite correct - "backwards compatibile" means new code can read old data, which we always maintain. I don't know a succinct way to say it; maybe "breaks forwards-compatibility for previous releases" ?
https://codereview.chromium.org/2844543002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h (right): https://codereview.chromium.org/2844543002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h:84: // WARNING: Increasing this value is a backward-incompatible change. On 2017/04/25 at 20:08:38, jsbell wrote: > "backward-incompatible" isn't quite correct - "backwards compatibile" means new code can read old data, which we always maintain. > > I don't know a succinct way to say it; maybe "breaks forwards-compatibility for previous releases" ? Ah, you're quite correct. Hmm...I'm not sure your wording is as clear as I'd like. WDYT of: // WARNING: Increasing this value is a change which cannot safely be rolled // back without breaking compatibility with data stored on disk. It is // strongly recommended that you do not make such changes near a release // milestone branch point. // // Recent changes are routinely reverted in preparation for branch, and this // has been the cause of at least one bug in the past.
On 2017/04/25 20:13:44, jbroman wrote: > > // WARNING: Increasing this value is a change which cannot safely be rolled > // back without breaking compatibility with data stored on disk. It is > // strongly recommended that you do not make such changes near a release > // milestone branch point. > // > // Recent changes are routinely reverted in preparation for branch, and this > // has been the cause of at least one bug in the past. lgtm !!
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/25 at 20:21:16, jsbell wrote: > On 2017/04/25 20:13:44, jbroman wrote: > > > > // WARNING: Increasing this value is a change which cannot safely be rolled > > // back without breaking compatibility with data stored on disk. It is > > // strongly recommended that you do not make such changes near a release > > // milestone branch point. > > // > > // Recent changes are routinely reverted in preparation for branch, and this > > // has been the cause of at least one bug in the past. > > lgtm !! Done. I'm going to wait for V8-side review before landing, because the House and Senate^W^W^W^W Chromium and V8 should adopt exactly the same wording. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org Link to the patchset: https://codereview.chromium.org/2844543002/#ps20001 (title: "update wording")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493307662694000,
"parent_rev": "6d34a3f1dd1242affe788d62d9b6025e239bf030", "commit_rev":
"f9f2d84e6e7e578e3b7f248184d6564d1f92c43f"}
Message was sent while issue was closed.
Description was changed from ========== SerializedScriptValue: Add a warning about making wire format changes near branch. BUG=715232 ========== to ========== SerializedScriptValue: Add a warning about making wire format changes near branch. BUG=715232 Review-Url: https://codereview.chromium.org/2844543002 Cr-Commit-Position: refs/heads/master@{#467732} Committed: https://chromium.googlesource.com/chromium/src/+/f9f2d84e6e7e578e3b7f248184d6... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f9f2d84e6e7e578e3b7f248184d6... |
