Implement TextDecoder and TextEncoder
Created attachment 292305 [details] Patch
Created attachment 292306 [details] Patch
Comment on attachment 292306 [details] Patch Attachment 292306 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2335040 New failing tests: js/dom/global-constructors-attributes-dedicated-worker.html
Created attachment 292313 [details] Archive of layout-test-results from ews102 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 292306 [details] Patch Attachment 292306 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2335083 New failing tests: js/dom/global-constructors-attributes.html js/dom/global-constructors-attributes-dedicated-worker.html
Created attachment 292314 [details] Archive of layout-test-results from ews107 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 292306 [details] Patch Attachment 292306 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2335060 New failing tests: js/dom/global-constructors-attributes-dedicated-worker.html
Created attachment 292315 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 292318 [details] Patch
Comment on attachment 292318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292318&action=review > LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-labels-expected.txt:3170 > +FAIL "cseuckr" => "euc-kr" assert_equals: label for encoding should match expected "euc-kr" but got "windows-949" These failures can only be fixed by changing TextCodecICU.cpp which I don't want to touch in this patch. > LayoutTests/imported/w3c/web-platform-tests/encoding/textdecoder-utf16-surrogates-expected.txt:5 > +FAIL utf-16le - lone surrogate trail assert_equals: expected "�" but got "ðÂ" These failures are related to the FIXME comment in TextCodecUTF16::decode which I don't want to touch in this patch.
Created attachment 292319 [details] Patch
Created attachment 292323 [details] Patch
Could somebody test the efl build? I don't know why it fails.
Why are there linker failures with typed arrays? This is super confusing
Comment on attachment 292323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292323&action=review > Source/WebCore/dom/TextDecoder.idl:39 > +[Constructor(optional DOMString label = "utf-8", optional TextDecoderOptions options), > +Exposed=(Window,Worker), > +ImplementationLacksVTable, > +ConstructorMayThrowException] > +interface TextDecoder { This is weird formatting for extended attributes. We usually do something like: [ Foo, Bar ] interface TextDecoder { > Source/WebCore/dom/TextEncoder.idl:29 > +[Constructor, > +Exposed=(Window,Worker), > +ImplementationLacksVTable] > +interface TextEncoder { Formatting.
Comment on attachment 292323 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=292323&action=review > Source/WebCore/dom/TextDecoder.cpp:39 > +static bool isEncodingWhitespace(UChar c) > +{ > + return c == 0x09 > + || c == 0x0A > + || c == 0x0C > + || c == 0x0D > + || c == 0x20; > +} This function does the same thing as isHTMLSpace, but less efficiently. I suggest either using isHTMLSpace directly, or having this function be an inline that calls isHTMLSpace. > Source/WebCore/dom/TextDecoder.cpp:41 > +ExceptionOr<Ref<TextDecoder>> TextDecoder::create(String label, Options options) This function should take a const String&, not a String. > Source/WebCore/dom/TextDecoder.cpp:43 > + String strippedLabel = label.stripWhiteSpace(isEncodingWhitespace); This should use the much more efficient stripLeadingAndTrailingHTMLSpaces function rather than the one where we pass a function in. > Source/WebCore/dom/TextDecoder.cpp:46 > + if (strippedLabel != utf8Label.data()) > + return Exception { RangeError }; What is the point of this? I think this is just a slow way to check wither the label has any non-ASCII characters in it. We have faster ways to check that. > Source/WebCore/dom/TextDecoder.cpp:48 > + Ref<TextDecoder> decoder = adoptRef(*new TextDecoder(utf8Label, options)); Should use auto here, I think. > Source/WebCore/dom/TextDecoder.cpp:54 > +TextDecoder::TextDecoder(CString label, Options options) This should take const char*, not CString. > Source/WebCore/dom/TextDecoder.cpp:93 > + const UChar utf16BEBOM[2] = {0xFEFF, '\0'}; > + return makeString(utf16BEBOM, decoded); This is so expensive, requires copying the entire string to prepend this single character. Is there another way to accomplish this? Can we put this into the buffer beforehand or something? > Source/WebCore/dom/TextDecoder.cpp:136 > + m_hasDecoded = true; > + return WTFMove(result); Should just remove these two lines of code. The function will do the same thing without them. > Source/WebCore/dom/TextEncoder.cpp:38 > +RefPtr<Uint8Array> TextEncoder::encode(String&& input) const This should return Ref, not RefPtr. > Source/WebCore/dom/TextEncoder.cpp:41 > + return Uint8Array::create(input.characters8(), input.length()); This is wrong if any characters are non-ASCII. The Latin-1 characters won’t be converted to UTF-8 and so the encoding will be done incorrectly. Need test cases to cover this. > Source/WebCore/dom/TextEncoder.cpp:43 > + CString utf8 = input.utf8(); Using this function means copying the UTF-8 bytes, rather than decoding them directly into a Uint8Array buffer. We have the functions that could do this more efficiently or we can refactor to make sure we don’t need to alllocate an extra buffer and I would like to consider doing that instead. The utf8() function can fail, returning an empty string, when the input text is not valid UTF-16. Is it OK that we create an empty array in that case? Do we have test cases covering that? > Source/WebCore/dom/TextEncoder.h:35 > +class TextEncoder : public RefCounted<TextEncoder> { The class is named TextEncoder, but it always encodes as UTF-8?
Comment on attachment 292323 [details] Patch review- because of the Latin-1/UTF-8 mistake, which is a showstopper Also, there seems to be insufficient test coverage in the tests we imported for this.
Created attachment 295003 [details] Patch
Please make sure to update the features.json file with this change.
Also, might be worth splitting out the patch from the tests for review, as 2.4 MB is a bit big.
Created attachment 295053 [details] Patch
Created attachment 295066 [details] Patch
Created attachment 295092 [details] source_changes
Comment on attachment 295092 [details] source_changes View in context: https://bugs.webkit.org/attachment.cgi?id=295092&action=review > Source/WebCore/dom/TextEncoder.cpp:43 > + // FIXME: We should not need to allocate a CString to encode into a Uint8Array. > + CString utf8 = input.utf8(); > + return Uint8Array::create(reinterpret_cast<const uint8_t*>(utf8.data()), utf8.length()); While I agree with your comment, is there no way to have Uint8Array adopt the internal buffer of the c-string, which can relinquish control? Copying here seems really unfortunate. > Source/WebCore/dom/TextEncoder.h:39 > + RefPtr<Uint8Array> encode(String&&) const; This can return a Ref. > Source/WebCore/dom/TextEncoder.h:41 > + TextEncoder() { }; Can this be = default? > Source/WebCore/dom/TextEncoder.idl:34 > + > + // FIXME: This should have [NewObject] > + Uint8Array encode(optional USVString input = ""); Why not add it? What happens?
Comment on attachment 295092 [details] source_changes Attachment 295092 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2533704 New failing tests: imported/w3c/web-platform-tests/encoding/textencoder-constructor-non-utf.html imported/w3c/web-platform-tests/encoding/textdecoder-streaming.html imported/w3c/web-platform-tests/encoding/textdecoder-fatal-single-byte.html imported/w3c/web-platform-tests/encoding/api-replacement-encodings.html imported/w3c/web-platform-tests/encoding/idlharness.html imported/w3c/web-platform-tests/encoding/textencoder-utf16-surrogates.html imported/w3c/web-platform-tests/encoding/api-basics.html imported/w3c/web-platform-tests/encoding/iso-2022-jp-decoder.html js/dom/global-constructors-attributes-dedicated-worker.html imported/w3c/web-platform-tests/encoding/api-surrogates-utf8.html imported/w3c/web-platform-tests/encoding/textdecoder-utf16-surrogates.html imported/w3c/web-platform-tests/encoding/textdecoder-ignorebom.html imported/w3c/web-platform-tests/encoding/textdecoder-fatal-streaming.html imported/w3c/web-platform-tests/encoding/textdecoder-fatal.html imported/w3c/web-platform-tests/encoding/textdecoder-labels.html imported/w3c/web-platform-tests/encoding/api-invalid-label.html imported/w3c/web-platform-tests/encoding/single-byte-decoder.html imported/w3c/web-platform-tests/encoding/textdecoder-byte-order-marks.html
Created attachment 295102 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 295092 [details] source_changes Attachment 295092 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2533712 New failing tests: imported/w3c/web-platform-tests/encoding/textencoder-constructor-non-utf.html imported/w3c/web-platform-tests/encoding/textdecoder-streaming.html imported/w3c/web-platform-tests/encoding/textdecoder-fatal-single-byte.html imported/w3c/web-platform-tests/encoding/api-replacement-encodings.html imported/w3c/web-platform-tests/encoding/idlharness.html imported/w3c/web-platform-tests/encoding/textencoder-utf16-surrogates.html imported/w3c/web-platform-tests/encoding/api-basics.html imported/w3c/web-platform-tests/encoding/iso-2022-jp-decoder.html js/dom/global-constructors-attributes-dedicated-worker.html imported/w3c/web-platform-tests/encoding/api-surrogates-utf8.html imported/w3c/web-platform-tests/encoding/textdecoder-utf16-surrogates.html imported/w3c/web-platform-tests/encoding/textdecoder-ignorebom.html imported/w3c/web-platform-tests/encoding/textdecoder-fatal-streaming.html imported/w3c/web-platform-tests/encoding/textdecoder-fatal.html imported/w3c/web-platform-tests/encoding/textdecoder-labels.html imported/w3c/web-platform-tests/encoding/api-invalid-label.html imported/w3c/web-platform-tests/encoding/single-byte-decoder.html imported/w3c/web-platform-tests/encoding/textdecoder-byte-order-marks.html js/dom/global-constructors-attributes.html
Created attachment 295103 [details] Archive of layout-test-results from ews104 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 295092 [details] source_changes Attachment 295092 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2533764 New failing tests: imported/w3c/web-platform-tests/encoding/textencoder-constructor-non-utf.html imported/w3c/web-platform-tests/encoding/textdecoder-streaming.html imported/w3c/web-platform-tests/encoding/textdecoder-fatal-single-byte.html imported/w3c/web-platform-tests/encoding/api-replacement-encodings.html imported/w3c/web-platform-tests/encoding/idlharness.html imported/w3c/web-platform-tests/encoding/textencoder-utf16-surrogates.html imported/w3c/web-platform-tests/encoding/api-basics.html imported/w3c/web-platform-tests/encoding/iso-2022-jp-decoder.html js/dom/global-constructors-attributes-dedicated-worker.html imported/w3c/web-platform-tests/encoding/api-surrogates-utf8.html imported/w3c/web-platform-tests/encoding/textdecoder-utf16-surrogates.html imported/w3c/web-platform-tests/encoding/textdecoder-ignorebom.html imported/w3c/web-platform-tests/encoding/textdecoder-fatal-streaming.html imported/w3c/web-platform-tests/encoding/textdecoder-fatal.html imported/w3c/web-platform-tests/encoding/textdecoder-labels.html imported/w3c/web-platform-tests/encoding/api-invalid-label.html imported/w3c/web-platform-tests/encoding/single-byte-decoder.html imported/w3c/web-platform-tests/encoding/textdecoder-byte-order-marks.html
Created attachment 295104 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 295092 [details] source_changes Attachment 295092 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2533771 New failing tests: imported/w3c/web-platform-tests/encoding/textencoder-constructor-non-utf.html imported/w3c/web-platform-tests/encoding/textdecoder-streaming.html imported/w3c/web-platform-tests/encoding/textdecoder-fatal-single-byte.html imported/w3c/web-platform-tests/encoding/api-replacement-encodings.html imported/w3c/web-platform-tests/encoding/idlharness.html imported/w3c/web-platform-tests/encoding/textencoder-utf16-surrogates.html imported/w3c/web-platform-tests/encoding/api-basics.html imported/w3c/web-platform-tests/encoding/iso-2022-jp-decoder.html js/dom/global-constructors-attributes-dedicated-worker.html imported/w3c/web-platform-tests/encoding/api-surrogates-utf8.html imported/w3c/web-platform-tests/encoding/textdecoder-utf16-surrogates.html imported/w3c/web-platform-tests/encoding/textdecoder-ignorebom.html imported/w3c/web-platform-tests/encoding/textdecoder-fatal-streaming.html imported/w3c/web-platform-tests/encoding/textdecoder-fatal.html imported/w3c/web-platform-tests/encoding/textdecoder-labels.html imported/w3c/web-platform-tests/encoding/api-invalid-label.html imported/w3c/web-platform-tests/encoding/single-byte-decoder.html imported/w3c/web-platform-tests/encoding/textdecoder-byte-order-marks.html
Created attachment 295106 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
http://trac.webkit.org/changeset/208872
*** Bug 160653 has been marked as a duplicate of this bug. ***