Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2245003002:
[Binding] Load 4 bytes to detect BOM (Closed)
DescriptionLoad 4 bytes to detect BOM.
It is guaranteed that the shared buffer contains s_smallScriptThreashold (=30*1024)
bytes, but it is not guaranteed that a chunked segment holds >=2 bytes.
Before this CL, we might hit such a short segment and failed to detect
BOM of the script.
This CL copies leading 4 bytes, which is needed and enough to detect BOM,
and avoids to lack data.
BUG=634935
Committed: https://crrev.com/58cdacd743ce7c608a0284906549366a211df0f5
Cr-Commit-Position: refs/heads/master@{#412206}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Create a method in SharedBuffer #Patch Set 3 : Split CL #
Total comments: 3
Patch Set 4 : Fix compile error #Messages
Total messages: 25 (9 generated)
peria@chromium.org changed reviewers: + tzik@chromium.org
tzik@ Could you check if this CL goes to a right direction?
https://codereview.chromium.org/2245003002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp (right): https://codereview.chromium.org/2245003002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:502: char maybeBom[length + 1] = {}; nit: s/Bom/BOM/ for consistency? nit: Do we need "+ 1" and "= {}" here?
Description was changed from ========== Load at least 4 bytes to detect BOM. It is guaranteed that the shared buffer contains s_smallScriptThreashold (=30*1024) bytes, but it is not guaranteed that a chunked segment holds >=2 bytes. In this case, 4 bytes are needed and enough for our objective. So this CL copies this small data to a local buffer. BUG=634935 ========== to ========== Load 4 bytes to detect BOM. It is guaranteed that the shared buffer contains s_smallScriptThreashold (=30*1024) bytes, but it is not guaranteed that a chunked segment holds >=2 bytes. Before this CL, we might hit such a short segment and failed to detect BOM of the script. This CL copies leading 4 bytes, which is needed and enough to detect BOM, and avoids to lack data. BUG=634935 ==========
https://codereview.chromium.org/2245003002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp (right): https://codereview.chromium.org/2245003002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:502: char maybeBom[length + 1] = {}; On 2016/08/15 06:02:22, tzik wrote: > nit: s/Bom/BOM/ for consistency? > nit: Do we need "+ 1" and "= {}" here? Done: BOM, +1 I put "= {}" just in case to clear maybeBOM[] with zeros.
peria@chromium.org changed reviewers: + yukishiino@chromium.org
add yukishiino@ in reviewers
haraken@chromium.org changed reviewers: + haraken@chromium.org, marja@chromium.org
LGTM +marja FYI
lgtm
Thank you for the review, but I split the part of SharedBuffer into another CL. http://crrev.com/2249533003
lgtm
kouhei@chromium.org changed reviewers: + kouhei@chromium.org
https://codereview.chromium.org/2245003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp (right): https://codereview.chromium.org/2245003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:500: // than enough for detecting a BOM. Let's update this comment too.
https://codereview.chromium.org/2245003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp (right): https://codereview.chromium.org/2245003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:500: // than enough for detecting a BOM. On 2016/08/15 07:43:49, kouhei wrote: > Let's update this comment too. Discussed offline, let me retract this.
https://codereview.chromium.org/2245003002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp (right): https://codereview.chromium.org/2245003002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp:500: // than enough for detecting a BOM. On 2016/08/15 08:19:14, kouhei wrote: > On 2016/08/15 07:43:49, kouhei wrote: > > Let's update this comment too. > Discussed offline, let me retract this. Acknowledged.
I will commit this after marja@'s review.
lgtm
The CQ bit was checked by peria@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yukishiino@chromium.org, tzik@chromium.org Link to the patchset: https://codereview.chromium.org/2245003002/#ps60001 (title: "Fix compile error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Load 4 bytes to detect BOM. It is guaranteed that the shared buffer contains s_smallScriptThreashold (=30*1024) bytes, but it is not guaranteed that a chunked segment holds >=2 bytes. Before this CL, we might hit such a short segment and failed to detect BOM of the script. This CL copies leading 4 bytes, which is needed and enough to detect BOM, and avoids to lack data. BUG=634935 ========== to ========== Load 4 bytes to detect BOM. It is guaranteed that the shared buffer contains s_smallScriptThreashold (=30*1024) bytes, but it is not guaranteed that a chunked segment holds >=2 bytes. Before this CL, we might hit such a short segment and failed to detect BOM of the script. This CL copies leading 4 bytes, which is needed and enough to detect BOM, and avoids to lack data. BUG=634935 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Load 4 bytes to detect BOM. It is guaranteed that the shared buffer contains s_smallScriptThreashold (=30*1024) bytes, but it is not guaranteed that a chunked segment holds >=2 bytes. Before this CL, we might hit such a short segment and failed to detect BOM of the script. This CL copies leading 4 bytes, which is needed and enough to detect BOM, and avoids to lack data. BUG=634935 ========== to ========== Load 4 bytes to detect BOM. It is guaranteed that the shared buffer contains s_smallScriptThreashold (=30*1024) bytes, but it is not guaranteed that a chunked segment holds >=2 bytes. Before this CL, we might hit such a short segment and failed to detect BOM of the script. This CL copies leading 4 bytes, which is needed and enough to detect BOM, and avoids to lack data. BUG=634935 Committed: https://crrev.com/58cdacd743ce7c608a0284906549366a211df0f5 Cr-Commit-Position: refs/heads/master@{#412206} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/58cdacd743ce7c608a0284906549366a211df0f5 Cr-Commit-Position: refs/heads/master@{#412206} |
