Fix Body::text() and Body::json() to handle BOMs #5621
Conversation
|
This one is technically observable, isn't it? Like what if I had crappy code that expected the BOM there |
This comment was marked as outdated.
This comment was marked as outdated.
anonrig
changed the title
anonrig
force-pushed
the
yagiz/fix-wpt-tests-text-json
branch
from
560fc65 to
d475224
Compare
anonrig
force-pushed
the
yagiz/fix-wpt-tests-text-json
branch
from
d475224 to
dc9792f
Compare
| auto size = part.size() - skipBytes; | ||
| skipBytes = 0; | ||
| KJ_DASSERT(size <= out.size() - pos); | ||
| out.slice(pos, pos + size).copyFrom(kj::arrayPtr(begin, size)); |
There was a problem hiding this comment.
The part really ought to be using slice also here...
Would need to test but I think something like ...
| out.slice(pos, pos + size).copyFrom(kj::arrayPtr(begin, size)); | |
| out.slice(pos, pos + size).copyFrom(part.slice(skipBytes)); | |
| skipBytes = 0; |
And removing the auto begin = ... and auto size = ...
You can also simplify this by trimming out on each iteration.
| auto option = ReadAllTextOption::NULL_TERMINATE; | ||
| if (FeatureFlags::get(js).getPedanticWpt()) { | ||
| option |= ReadAllTextOption::STRIP_BOM; | ||
| } |
There was a problem hiding this comment.
I'm not sure if bundling this under the pedantic wpt flag directly is correct. I think it's likely better to have it as a separate compat flag that can have a implied-by pedantic wpt.
I'm also not convinced that we should expose the option completely out to this point. Specifically, readAllText() here will always null terminate so exposing the option to do so or not here doesn't make sense and at this layer we should also assume it strips the BOM if necessary. The option was added internally in the impl only because we use the same base impl logic for both readAllText() and readAllBytes(), but at this level it shouldn't be something that is surfaced.
4 participants
Fixes #4712
Fixes #4977