Bug 118355

Summary: Remove code duplication from StringImpl create()/reallocate() methods
Product: WebKit Reporter: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Component: Web Template FrameworkAssignee: Mikhail Pozdnyakov <mikhail.pozdnyakov>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, buildbot, cmarcelo, commit-queue, darin, kling, rniwa, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 118361    
Bug Blocks:    
Attachments:
Description Flags
patch
andersca: review+
patch v2
none
patch v3
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
none
patch v3 none

Description Mikhail Pozdnyakov 2013-07-03 06:05:36 PDT 
SSIA. StringImpl create()/reallocate() methods accepting LChar and UChar have duplicated code at the moment.
Comment 1 Mikhail Pozdnyakov 2013-07-03 06:17:57 PDT 
Created attachment 205996 [details]
patch
Comment 2 Mikhail Pozdnyakov 2013-07-03 06:19:45 PDT 
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.
Comment 3 Anders Carlsson 2013-07-03 08:29:14 PDT 
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.
Comment 4 Mikhail Pozdnyakov 2013-07-03 08:39:47 PDT 
Committed r152356: <http://trac.webkit.org/changeset/152356>
Comment 5 Mikhail Pozdnyakov 2013-07-03 08:40:20 PDT 
(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.
Comment 6 Simon Fraser (smfr) 2013-07-03 09:59:57 PDT 
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.
Comment 7 WebKit Commit Bot 2013-07-03 10:02:28 PDT 
Re-opened since this is blocked by bug 118361
Comment 8 Mikhail Pozdnyakov 2013-07-04 03:37:35 PDT 
(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 :(
Comment 9 Mikhail Pozdnyakov 2013-07-04 04:06:09 PDT 
Created attachment 206072 [details]
patch v2

Fixed the assertions.
Comment 10 Mikhail Pozdnyakov 2013-07-04 05:53:38 PDT 
Created attachment 206082 [details]
patch v3

Using constructInternal() also inside tryCreateUninitialized().
Comment 11 Build Bot 2013-07-04 08:05:32 PDT 
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
Comment 12 Build Bot 2013-07-04 08:05:36 PDT 
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
Comment 13 Mikhail Pozdnyakov 2013-07-04 08:24:49 PDT 
(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.
Comment 14 Mikhail Pozdnyakov 2013-07-04 08:25:36 PDT 
Created attachment 206092 [details]
patch v3

run EWS.
Comment 15 Andreas Kling 2013-07-05 04:00:54 PDT 
Comment on attachment 206092 [details]
patch v3

Looks correct.
Comment 16 WebKit Commit Bot 2013-07-05 04:24:28 PDT 
Comment on attachment 206092 [details]
patch v3

Clearing flags on attachment: 206092

Committed r152415: <http://trac.webkit.org/changeset/152415>
Comment 17 WebKit Commit Bot 2013-07-05 04:24:32 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 18 Benjamin Poulain 2013-07-16 20:32:23 PDT 
The typename for characters should be CharacterType, not CharType.