Bug 66661 – Replace some usages of Vector<UChar> to StringBuilder

• Component: Web Template Framework
• Status: UNCONFIRMED

[Xianzhu Wang]

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.

[Xianzhu Wang]

Created attachment 105137 [details]
First patch

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 105137 [details]
First patch

Attachment 105137 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9509220

[Xianzhu Wang]

Created attachment 105139 [details]
patch v2

[Early Warning System Bot]

Comment on attachment 105139 [details]
patch v2

Attachment 105139 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9513271

[Xianzhu Wang]

Created attachment 105330 [details]
patch v3

[Xianzhu Wang]

Created attachment 105335 [details]
patch v4 (trying to fix windows build break)

[Xianzhu Wang]

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!

[Early Warning System Bot]

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

[Darin Adler]

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.

[Annie Sullivan]

(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?

[Xianzhu Wang]

(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.

[Ryosuke Niwa]

(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.