[webkit-reviews] review denied: [Bug 71316] decodeEscapeSequences() not correct for some encodings (GBK, Big5, ...). : [Attachment 122472] Patch, tests, fix changelog again.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Thomas Sepez
<tsepez at chromium.org>'s request for review:
Bug 71316: decodeEscapeSequences() not correct for some encodings (GBK, Big5,
...).
https://bugs.webkit.org/show_bug.cgi?id=71316

Attachment 122472: Patch, tests, fix changelog again.
https://bugs.webkit.org/attachment.cgi?id=122472&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
decodeEscapeSequences() is used in many other places in WebCore besides XSS
Auditor. How did you verify that the changes there are compatible with other
browsers? Please add tests that directly involve this function as part of KURL
accessors, ScriptController::executeIfJavaScriptURL(), etc.

I'm really confused about the approach here. Why does a simple function that
decodes percent escapes need to know about encodings? That looks like a
layering violation.

I think that the actual bug here is that we flush the decoder at the end of
decodeRun() function. We should just pass all text through it, unless it can be
demonstrated to be a performance problem.

r- for lack of tests primarily, although the fix may need to be reconsidered,
too.