Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1088)

Issue 2212393003: Fix BOM handling in TextResourceDecoder on partial data (Closed)

Created:
4 years, 4 months ago by tzik
Modified:
4 years, 4 months ago
Reviewers:
kouhei (in TOK)
CC:
chromium-reviews, blink-reviews, dglazkov+blink, blink-reviews-html_chromium.org, kinuko+watch, loading-reviews+parser_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix BOM handling in TextResourceDecoder on partial data TextResourceDecoder::decode() fails to handle BOM check failure when available chunk of the data is only few bytes. Also, TextResourceDecoder::checkForBOM() wrongly detects little endian UTF16 BOM as little endian UTF32 when avaibale data is only few bytes. This CL changes decode() to defer the decoding on failure on BOM detection, and fixes checkForBOM() for ambiguous BOM case. BUG=634935 Committed: https://crrev.com/c792d1ca228e3fd8fc254c9a8f3a51e5e5d76832 Cr-Commit-Position: refs/heads/master@{#410283}

Patch Set 1 #

Total comments: 2

Patch Set 2 : win bot fix #

Patch Set 3 : +comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+58 lines, -2 lines) Patch
M third_party/WebKit/Source/core/core.gypi View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp View 1 2 2 chunks +11 lines, -2 lines 0 comments Download
A third_party/WebKit/Source/core/html/parser/TextResourceDecoderTest.cpp View 1 1 chunk +46 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (13 generated)
tzik
PTAL
4 years, 4 months ago (2016-08-05 15:03:03 UTC) #4
kouhei (in TOK)
lgtm % comment. Thanks for the patch! https://codereview.chromium.org/2212393003/diff/1/third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp File third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp (right): https://codereview.chromium.org/2212393003/diff/1/third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp#newcode374 third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp:374: if (!m_checkedForBOM) ...
4 years, 4 months ago (2016-08-06 01:16:31 UTC) #7
tzik
https://codereview.chromium.org/2212393003/diff/1/third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp File third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp (right): https://codereview.chromium.org/2212393003/diff/1/third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp#newcode374 third_party/WebKit/Source/core/html/parser/TextResourceDecoder.cpp:374: if (!m_checkedForBOM) { On 2016/08/06 01:16:31, kouhei wrote: > ...
4 years, 4 months ago (2016-08-07 10:18:45 UTC) #12
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/2212393003/40001
4 years, 4 months ago (2016-08-07 10:19:11 UTC) #16
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 4 months ago (2016-08-07 11:25:58 UTC) #17
commit-bot: I haz the power
4 years, 4 months ago (2016-08-07 11:27:26 UTC) #19
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/c792d1ca228e3fd8fc254c9a8f3a51e5e5d76832
Cr-Commit-Position: refs/heads/master@{#410283}

Powered by Google App Engine
This is Rietveld 408576698