Consolidated fixes to character encoders/decoders #11707
Conversation
The relevant step fixed by this pull request says "Return error."; thus, the rest of the process should continue with the next iteration, rather than run the rest of the handler for the given byte.
The relevant step in the Encoding Standard says "Prepend _lead_ and _byte_ to _stream_." However, the two bytes are prepended in the wrong order in the reference implementation. Note that under the Encoding Standard, "[w]hen one or more tokens are prepended to a stream, those tokens must be inserted, _in given order_, before the first token in the stream." (Note that the code, at the time of this request, moves _lead_ to the front of the array, then moves _byte_ to the front of the array.) There may be other issues like this elsewhere in the multiple-byte encoder reference implementations.
Makes one "if" statement conditional rather than unconditional
…suggestions by 'inexorabletash'
There was a problem hiding this comment.
Looks good but the change to line 49 in sjis-decoder.js in #11651 got dropped:
if (ptr != null && ptr >= 8836 && ptr <= 10715) {
|
Also, thank you for adding those missing |
To conform to the most recent Encoding Standard.
|
Awesome, thank you! Waiting for the CI to complete before merging. |
|
Looks like the failure was due to missing META.yml lint check, and that lint check was reverted. Let's see if we can re-run this... |
|
I'm really sorry about the delay here. Can you make a whitespace change or something to trigger a re-run? |
|
Thanks - now it just looks like it's hitting a timeout error (?) on the firefox runs? @gsnedders - can you provide guidance here? These fixes shouldn't be dramatically altering the runtime of the tests. |
|
Yeah there are too many affected tests. @inexorabletash would you mind mirroring the patch to Chromium to test it through a CQ dry run? |
|
Yeah, I'll give it a whirl. Poke me if I haven't reported back in a couple days. |
|
Passed locally - CL is up for a CQ run at: https://chromium-review.googlesource.com/c/chromium/src/+/1148994 |
|
... and it passed on our CI. Do you want me to go ahead and land it from our side? |
This is #11707 applied to the chromium code base to get CQ run results since we're getting timeouts on the GitHub CI. Original change by @peteroupc Change-Id: Ief53379fb2da87f90b6c2b3be968f02f25a99e12
|
Thank you, @inexorabletash . I just admin-merged this PR (I prefer this one to preserve more review history). |
Supersedes #11645, #11648, #11650, #11651, #11652.