Bug 118355 – Remove code duplication from StringImpl create()/reallocate() methods

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Mikhail Pozdnyakov]

SSIA. StringImpl create()/reallocate() methods accepting LChar and UChar have duplicated code at the moment.

[Mikhail Pozdnyakov]

Created attachment 205996 [details]
patch

[Mikhail Pozdnyakov]

Comment on attachment 205996 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205996&action=review

> Source/WTF/wtf/text/StringImpl.h:773
> +    template <typename CharType> static PassRefPtr<StringImpl> constructInternal(StringImpl*, unsigned);

may be it is better to keep these functions free, but some of them would need to become friends as accessing the class private fields.

[Anders Carlsson]

Comment on attachment 205996 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=205996&action=review

> Source/WTF/wtf/text/StringImpl.cpp:186
> +inline PassRefPtr<StringImpl> StringImpl::constructInternal(StringImpl* arena, unsigned length)

I don't think arena is the right parameter name here. How about simply "stringImpl"?

> Source/WTF/wtf/text/StringImpl.cpp:192
> +inline PassRefPtr<StringImpl> StringImpl::constructInternal<LChar>(StringImpl* arena, unsigned length)

Ditto.

[Mikhail Pozdnyakov]

Committed r152356: <http://trac.webkit.org/changeset/152356>

[Mikhail Pozdnyakov]

(In reply to comment #3)
> (From update of attachment 205996 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=205996&action=review
> 
> > Source/WTF/wtf/text/StringImpl.cpp:186
> > +inline PassRefPtr<StringImpl> StringImpl::constructInternal(StringImpl* arena, unsigned length)
> 
> I don't think arena is the right parameter name here. How about simply "stringImpl"?
> 
> > Source/WTF/wtf/text/StringImpl.cpp:192
> > +inline PassRefPtr<StringImpl> StringImpl::constructInternal<LChar>(StringImpl* arena, unsigned length)
> 
> Ditto.
Done.

[Simon Fraser (smfr)]

JSCore tests are asserting after this change:

ASSERTION FAILED: originalString->is8Bit()
/Volumes/Data/slave/lion-debug/build/Source/WTF/wtf/text/StringImpl.cpp(230) : static PassRefPtr<WTF::StringImpl> WTF::StringImpl::reallocateInternal(PassRefPtr<WTF::StringImpl>, unsigned int, CharType *&) [CharType = unsigned short]
1   0x10fac5670 WTFCrash
2   0x10fb0397a WTF::PassRefPtr<WTF::StringImpl> WTF::StringImpl::reallocateInternal<unsigned short>(WTF::PassRefPtr<WTF::StringImpl>, unsigned int, unsigned short*&)
3   0x10fafb91a WTF::StringImpl::reallocate(WTF::PassRefPtr<WTF::StringImpl>, unsigned int, unsigned short*&)
4   0x10faf9af9 void WTF::StringBuilder::reallocateBuffer<unsigned short>(unsigned int)
5   0x10fafb2ba unsigned short* WTF::StringBuilder::appendUninitializedSlow<unsigned short>(unsigned int)
6   0x10fafac19 unsigned short* WTF::StringBuilder::appendUninitialized<unsigned short>(unsigned int)
7   0x10fafa09a WTF::StringBuilder::append(unsigned char const*, unsigned int)
8   0x10f6e9ecd WTF::StringBuilder::append(unsigned char)
9   0x10f6e9c5f WTF::StringBuilder::append(char)
10  0x10f9193d9 JSC::Stringifier::appendQuotedString(WTF::StringBuilder&, WTF::String const&)
11  0x10f91a2bb JSC::Stringifier::Holder::appendNextProperty(JSC::Stringifier&, WTF::StringBuilder&)
12  0x10f919292 JSC::Stringifier::appendStringifiedValue(WTF::StringBuilder&, JSC::JSValue, JSC::JSObject*, JSC::PropertyNameForFunctionCall const&)
13  0x10f9189e9 JSC::Stringifier::stringify(JSC::Handle<JSC::Unknown>)
14  0x10f91b5e7 JSC::JSONStringify(JSC::ExecState*, JSC::JSValue, unsigned int)
15  0x10f9396c0 JSValueCreateJSONString

http://build.webkit.org/builders/Apple%20Lion%20Debug%20WK1%20%28Tests%29

Please fix or the change will be reverted.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 118361

[Mikhail Pozdnyakov]

(In reply to comment #6)
> JSCore tests are asserting after this change:
> 
> ASSERTION FAILED: originalString->is8Bit()
> /Volumes/Data/slave/lion-debug/build/Source/WTF/wtf/text/StringImpl.cpp(230) : static PassRefPtr<WTF::StringImpl> WTF::StringImpl::reallocateInternal(PassRefPtr<WTF::StringImpl>, unsigned int, CharType *&) [CharType = unsigned short]
> 1   0x10fac5670 WTFCrash
> 2   0x10fb0397a WTF::PassRefPtr<WTF::StringImpl> WTF::StringImpl::reallocateInternal<unsigned short>(WTF::PassRefPtr<WTF::StringImpl>, unsigned int, unsigned short*&)
> 3   0x10fafb91a WTF::StringImpl::reallocate(WTF::PassRefPtr<WTF::StringImpl>, unsigned int, unsigned short*&)
> 4   0x10faf9af9 void WTF::StringBuilder::reallocateBuffer<unsigned short>(unsigned int)
> 5   0x10fafb2ba unsigned short* WTF::StringBuilder::appendUninitializedSlow<unsigned short>(unsigned int)
> 6   0x10fafac19 unsigned short* WTF::StringBuilder::appendUninitialized<unsigned short>(unsigned int)
> 7   0x10fafa09a WTF::StringBuilder::append(unsigned char const*, unsigned int)
> 8   0x10f6e9ecd WTF::StringBuilder::append(unsigned char)
> 9   0x10f6e9c5f WTF::StringBuilder::append(char)
> 10  0x10f9193d9 JSC::Stringifier::appendQuotedString(WTF::StringBuilder&, WTF::String const&)
> 11  0x10f91a2bb JSC::Stringifier::Holder::appendNextProperty(JSC::Stringifier&, WTF::StringBuilder&)
> 12  0x10f919292 JSC::Stringifier::appendStringifiedValue(WTF::StringBuilder&, JSC::JSValue, JSC::JSObject*, JSC::PropertyNameForFunctionCall const&)
> 13  0x10f9189e9 JSC::Stringifier::stringify(JSC::Handle<JSC::Unknown>)
> 14  0x10f91b5e7 JSC::JSONStringify(JSC::ExecState*, JSC::JSValue, unsigned int)
> 15  0x10f9396c0 JSValueCreateJSONString
> 
> http://build.webkit.org/builders/Apple%20Lion%20Debug%20WK1%20%28Tests%29
> 
> Please fix or the change will be reverted.
argh.. my bad, reallocate() methods should have input type-driven assertions: ASSERT(originalString->is8Bit()); and ASSERT(!originalString->is8Bit()); I missed it in my patch :(

[Mikhail Pozdnyakov]

Created attachment 206072 [details]
patch v2

Fixed the assertions.

[Mikhail Pozdnyakov]

Created attachment 206082 [details]
patch v3

Using constructInternal() also inside tryCreateUninitialized().

[Build Bot]

Comment on attachment 206082 [details]
patch v3

Attachment 206082 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/880891

New failing tests:
compositing/backgrounds/fixed-backgrounds.html
compositing/background-color/background-color-container.html

[Build Bot]

Created attachment 206089 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Mikhail Pozdnyakov]

(In reply to comment #11)
> (From update of attachment 206082 [details])
> Attachment 206082 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/880891
> 
> New failing tests:
> compositing/backgrounds/fixed-backgrounds.html
> compositing/background-color/background-color-container.html

That looks really weird for WTF::String change..Guess it's not related.

[Mikhail Pozdnyakov]

Created attachment 206092 [details]
patch v3

run EWS.

[Andreas Kling]

Comment on attachment 206092 [details]
patch v3

Looks correct.

[WebKit Commit Bot]

Comment on attachment 206092 [details]
patch v3

Clearing flags on attachment: 206092

Committed r152415: <http://trac.webkit.org/changeset/152415>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Benjamin Poulain]

The typename for characters should be CharacterType, not CharType.