Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(30)

Issue 2803593005: Avoid unnecessary byte-swapping in blink's SerializedScriptValue. (Closed)

Created:
8 months, 2 weeks ago by pwnall
Modified:
8 months, 1 week ago
Reviewers:
kinuko, jbroman, jsbell
CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-bindings_chromium.org, chromium-reviews, dglazkov+blink, jbroman+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Avoid unnecessary byte-swapping in blink's SerializedScriptValue. For historical reasons, SerializedStringValue byte-swaps its buffer when serializing to a string. This CL removes the unnecessary flipping for current values, and sets up a legacy deserialization code path, so byte-swapped SSVs can still be read correctly. BUG= Review-Url: https://codereview.chromium.org/2803593005 Cr-Commit-Position: refs/heads/master@{#464833} Committed: https://chromium.googlesource.com/chromium/src/+/9e27be00af394259afe4fd40b0c5955a29b29c40

Patch Set 1 #

Patch Set 2 : Fix test bug highlighted by ASAN. #

Total comments: 2

Patch Set 3 : Correct handling for SSV version 0. #

Total comments: 22

Patch Set 4 : Address jsbell feedback. #

Patch Set 5 : Rebased past the Blink rename. #

Total comments: 3

Patch Set 6 : Addressed jbroman feedback. #

Total comments: 10

Patch Set 7 : Addressed jbroman feedback. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+239 lines, -36 lines) Patch
M third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.h View 1 2 3 4 1 chunk +6 lines, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp View 1 2 3 4 5 6 5 chunks +121 lines, -21 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/SerializedScriptValueTest.cpp View 1 2 3 4 5 6 1 chunk +103 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp View 1 2 3 4 2 chunks +8 lines, -13 lines 0 comments Download
M third_party/WebKit/public/web/WebSerializedScriptValueVersion.h View 1 chunk +1 line, -1 line 0 comments Download
Trybot results:  win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_compile_dbg_ng   mac_chromium_rel_ng   ios-simulator-xcode-clang   ios-device-xcode-clang   ios-simulator   ios-device   linux_chromium_tsan_rel_ng   linux_chromium_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_asan_rel_ng   chromium_presubmit   chromeos_amd64-generic_chromium_compile_only_ng   chromeos_daisy_chromium_compile_only_ng   cast_shell_linux   android_n5x_swarming_rel   cast_shell_android   linux_android_rel_ng   android_compile_dbg   android_cronet   android_clang_dbg_recipe   android_arm64_dbg_recipe   linux_android_rel_ng   win_clang   win_chromium_x64_rel_ng   win_chromium_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_rel_ng   mac_chromium_compile_dbg_ng   ios-simulator-xcode-clang   ios-simulator   linux_chromium_tsan_rel_ng   ios-device-xcode-clang   ios-device   linux_chromium_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_chromeos_rel_ng   linux_chromium_chromeos_ozone_rel_ng   chromium_presubmit   linux_chromium_asan_rel_ng   chromeos_amd64-generic_chromium_compile_only_ng   chromeos_daisy_chromium_compile_only_ng   cast_shell_android   cast_shell_linux   linux_android_rel_ng   android_n5x_swarming_rel   android_cronet   android_compile_dbg   android_clang_dbg_recipe   android_arm64_dbg_recipe 

Messages

Total messages: 74 (47 generated)
pwnall
PTAL? I stumbled upon the byte-flipping code while writing an IndexedDB serialization test, and it ...
8 months, 2 weeks ago (2017-04-05 22:50:45 UTC) #9
jbroman
tl;dr: I think it's slightly more complicated than it seems, but I'd be happy to ...
8 months, 2 weeks ago (2017-04-06 01:04:37 UTC) #14
pwnall
jsbell: Can you please also weigh in on the SSV version burning idea below, and ...
8 months, 2 weeks ago (2017-04-06 09:26:25 UTC) #18
jsbell
While I ponder your questions, here's another idea to throw out: is there an unambiguous ...
8 months, 2 weeks ago (2017-04-06 17:07:47 UTC) #19
jbroman
On 2017/04/06 at 17:07:47, jsbell wrote: > While I ponder your questions, here's another idea ...
8 months, 2 weeks ago (2017-04-06 17:41:17 UTC) #20
jsbell
On 2017/04/06 17:07:47, jsbell wrote: > While I ponder your questions, here's another idea to ...
8 months, 2 weeks ago (2017-04-06 17:45:25 UTC) #21
pwnall
On 2017/04/06 17:45:25, jsbell wrote: > On 2017/04/06 17:07:47, jsbell wrote: > > While I ...
8 months, 2 weeks ago (2017-04-07 01:46:30 UTC) #27
jsbell
looking good, I think... https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp#newcode93 third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:93: // Versions 16 and below ...
8 months, 2 weeks ago (2017-04-07 23:40:34 UTC) #30
pwnall
jsbell: Thank you very much for the super-useful feedback! PTAL? https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp#newcode93 ...
8 months, 2 weeks ago (2017-04-08 01:09:08 UTC) #34
pwnall
I rebased past the Blink rename. Sorry, the diff between patch sets 4 and 5 ...
8 months, 1 week ago (2017-04-10 22:37:38 UTC) #39
jsbell
lgtm https://codereview.chromium.org/2803593005/diff/140001/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): https://codereview.chromium.org/2803593005/diff/140001/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp#newcode136 third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:136: constexpr static size_t kSSVHeaderBlinkVersionOffset = 1; Should we ...
8 months, 1 week ago (2017-04-10 22:49:39 UTC) #40
jbroman
https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp#newcode148 third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:148: // Slower path that would kick in after version ...
8 months, 1 week ago (2017-04-11 15:01:47 UTC) #43
jbroman
On 2017/04/11 at 15:01:47, jbroman wrote: > https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp > File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): > > https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp#newcode148 ...
8 months, 1 week ago (2017-04-11 15:02:00 UTC) #44
pwnall
jbroman: Thank you very much for the review! PTAL? https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2803593005/diff/100001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp#newcode148 third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:148: ...
8 months, 1 week ago (2017-04-11 19:10:55 UTC) #49
jsbell
https://codereview.chromium.org/2803593005/diff/140001/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp File third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp (right): https://codereview.chromium.org/2803593005/diff/140001/third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp#newcode136 third_party/WebKit/Source/bindings/modules/v8/V8BindingForModulesTest.cpp:136: constexpr static size_t kSSVHeaderBlinkVersionOffset = 1; On 2017/04/11 19:10:55, ...
8 months, 1 week ago (2017-04-11 20:37:38 UTC) #51
jbroman
lgtm It might be wise to wait until after the M59 branch to land, to ...
8 months, 1 week ago (2017-04-12 18:34:04 UTC) #54
pwnall
kinuko@chromium.org: Please review the change to third_party/WebKit/public/web/WebSerializedScriptValueVersion.h https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp (right): https://codereview.chromium.org/2803593005/diff/160001/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp#newcode187 third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:187: // which ...
8 months, 1 week ago (2017-04-12 21:37:30 UTC) #58
pwnall
On 2017/04/12 18:34:04, jbroman wrote: > lgtm > > It might be wise to wait ...
8 months, 1 week ago (2017-04-12 21:44:42 UTC) #59
kinuko
lgtm
8 months, 1 week ago (2017-04-13 03:53:29 UTC) #62
pwnall
On 2017/04/13 03:53:29, kinuko wrote: > lgtm jsbell, jbroman, kinuko: Thank you very much for ...
8 months, 1 week ago (2017-04-13 06:14:34 UTC) #63
jbroman
On 2017/04/12 at 21:44:42, pwnall wrote: > On 2017/04/12 18:34:04, jbroman wrote: > > lgtm ...
8 months, 1 week ago (2017-04-13 14:47:57 UTC) #64
jbroman
On 2017/04/13 at 06:14:34, pwnall wrote: > On 2017/04/13 03:53:29, kinuko wrote: > > lgtm ...
8 months, 1 week ago (2017-04-13 14:48:31 UTC) #65
jsbell
On 2017/04/13 14:48:31, jbroman wrote: > On 2017/04/13 at 06:14:34, pwnall wrote: > > On ...
8 months, 1 week ago (2017-04-13 15:32:25 UTC) #66
jsbell
Sorry about the dummy reply, hit the wrong button... On 2017/04/13 14:47:57, jbroman wrote: > ...
8 months, 1 week ago (2017-04-13 15:34:18 UTC) #67
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2803593005/180001
8 months, 1 week ago (2017-04-14 22:20:21 UTC) #70
pwnall
On 2017/04/13 15:34:18, jsbell wrote: > Sorry about the dummy reply, hit the wrong button... ...
8 months, 1 week ago (2017-04-14 22:20:48 UTC) #71
commit-bot: I haz the power
8 months, 1 week ago (2017-04-14 23:57:54 UTC) #74
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/9e27be00af394259afe4fd40b0c5...

Powered by Google App Engine
This is Rietveld 0eb02b776