Many places using Vector<UChar> actually using it as StringBuilder to build a string. They should be changed to use StringBuilder to support 8-bit string buffers.
Created attachment 105137 [details] First patch
Attachment 105137 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1 Source/JavaScriptCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Source/WebCore/rendering/mathml/RenderMathMLFenced.cpp:37: wtf includes should be <wtf/file.h> instead of "wtf/file.h". [build/include] [4] Source/WebCore/editing/MarkupAccumulator.h:115: The parameter name "entityMask" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/editing/markup.cpp:136: The parameter name "element" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/tests/StringBuilderTest.cpp:37: Alphabetical sorting problem. [build/include_order] [4] Source/WebKit/chromium/tests/StringBuilderTest.cpp:51: Use 0 instead of NULL. [readability/null] [5] Source/WebCore/css/CSSOMUtils.h:48: The parameter name "string" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/rendering/InlineTextBox.h:40: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebCore/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Source/WebKit/chromium/ChangeLog:1: ChangeLog entry has no bug number [changelog/bugnumber] [5] Source/WebCore/xml/parser/MarkupTokenBase.h:81: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 11 in 77 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 105137 [details] First patch Attachment 105137 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9509220
Created attachment 105139 [details] patch v2
Comment on attachment 105139 [details] patch v2 Attachment 105139 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9513271
Created attachment 105330 [details] patch v3
Created attachment 105335 [details] patch v4 (trying to fix windows build break)
Haven't request review because of the failure on win-ews, but you can have a preview and comments are welcomed. Could anyone who has Windows building environment help resolve the problem Thanks!
Comment on attachment 105335 [details] patch v4 (trying to fix windows build break) Attachment 105335 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9541057
Comment on attachment 105335 [details] patch v4 (trying to fix windows build break) View in context: https://bugs.webkit.org/attachment.cgi?id=105335&action=review This patch has far too many changes all at once. This could and should be done in smaller pieces. Adding lots of new functions and using them in lots of new places all in one patch is unnecessary for a refactoring patch. Especially because these functions do pointer arithmetic. We need to carefully think about overflow in all the new code. A good approach is to start with a large patch like this one in your tree and try to do only the parts of it that are obviously correct and easy to review. That first patch could have a lot of changes, but all super-straightforward ones. Then the trickier parts, such as adding new functions, can be done carefully with one function at a time. I’d suggest posting a first patch that uses StringBuilder more in various places where that can be done without even modifying it, for example. > Source/JavaScriptCore/JavaScriptCore.order:-326 > -__ZN3WTF13StringBuilder6appendEPKtj There is no need to try to update the order file. There is no harm having old functions in there, and no benefit to trying to update it manually. It will be regenerated eventually. > Source/JavaScriptCore/wtf/text/StringBuilder.cpp:126 > + memcpy(m_bufferCharacters + insertPosition + insertLength, currentCharacters + insertPosition, static_cast<size_t>(m_length - insertPosition) * sizeof(UChar)); Adding insertPosition to insertLength can lead to overflow. This code has to be written in a way that deals with that possibility. > Source/JavaScriptCore/wtf/text/StringBuilder.cpp:152 > + memmove(m_bufferCharacters + position + length, m_bufferCharacters + position, static_cast<size_t>(m_length - position) * sizeof(UChar)); Adding position to length can lead to overflow. This code has to be written in a way that deals with that possibility.
(In reply to comment #10) Thanks for the advice about how to better split up the patch, Darin. Xianzhu, please let me know if you'd like any help with the smaller patches. > Especially because these functions do pointer arithmetic. We need to carefully think about overflow in all the new code. In addition to thinking carefully about overflow when writing the code, it seems like unit tests would be helpful to make sure everything is correct. I couldn't find documentation on this, but it looks like: 1. Tests for these classes Tools/TestWebKitAPI/Tests/WTF/ 2. You can run the tests using the script Tools/Scripts/run-api-tests Is that correct?
(In reply to comment #11) > (In reply to comment #10) > > Thanks for the advice about how to better split up the patch, Darin. Xianzhu, please let me know if you'd like any help with the smaller patches. > I'll file some sub-bugs to track the smaller patches and take some of them. > > In addition to thinking carefully about overflow when writing the code, it seems like unit tests would be helpful to make sure everything is correct. I couldn't find documentation on this, but it looks like: > > 1. Tests for these classes Tools/TestWebKitAPI/Tests/WTF/ > 2. You can run the tests using the script Tools/Scripts/run-api-tests > > Is that correct? I've included a unit test in the patch, but only for chromium (Source/WebKit/tests/StringBuilderTest.cpp). I think it should be moved to under Tools/TestWebKitAPI/Tests/WTF.
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > Thanks for the advice about how to better split up the patch, Darin. Xianzhu, please let me know if you'd like any help with the smaller patches. > > > > I'll file some sub-bugs to track the smaller patches and take some of them. Yes, the current patch is way too big. Please cc me on the bug where you touch any file in Source/WebCore/editing especially ones related to MarkupAccumulator.cpp and markup.cpp.