Bug 166636

Summary: Leverage Substring to create new AtomicStringImpl for StaticStringImpl and SymbolImpl
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: Web Template FrameworkAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, darin, dbates, saam, sam
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 166586    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
darin: review+
Patch none

Yusuke Suzuki
Reported 2017-01-02 01:53:42 PST
...
Attachments
Patch (4.12 KB, patch)
2017-01-02 13:41 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (7.61 KB, patch)
2017-01-02 14:03 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Patch (10.12 KB, patch)
2017-01-02 15:55 PST, Yusuke Suzuki 		darin: review+
DetailsFormatted DiffDiff
Patch (10.48 KB, patch)
2017-01-02 18:41 PST, Yusuke Suzuki 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1 2017-01-02 13:41:46 PST
Created attachment 297910 [details] Patch
Yusuke Suzuki
Comment 2 2017-01-02 14:03:23 PST
Created attachment 297911 [details] Patch
Yusuke Suzuki
Comment 3 2017-01-02 14:04:13 PST
Comment on attachment 297911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297911&action=review > Source/WTF/wtf/text/AtomicStringImpl.cpp:-474 > - This is lookUp. So the above special handling is not necessary.
Yusuke Suzuki
Comment 4 2017-01-02 15:55:42 PST
Created attachment 297916 [details] Patch
Darin Adler
Comment 5 2017-01-02 18:25:06 PST
Comment on attachment 297916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297916&action=review Love this. Really great. > Source/WTF/wtf/text/AtomicStringImpl.cpp:71 > +typedef HashSet<StringImpl*> StringTableImpl; In new code we are using "using" rather than typedef. > Source/WTF/wtf/text/StringImpl.h:562 > + StringImpl& get() Not sure that I really love the name get() for this. Maybe downcastStringImpl or downcast<StringImpl> (presumably those would be non-member functions) or maybe operator StringImpl&.
Yusuke Suzuki
Comment 6 2017-01-02 18:29:10 PST
Comment on attachment 297916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297916&action=review Thanks! >> Source/WTF/wtf/text/AtomicStringImpl.cpp:71 >> +typedef HashSet<StringImpl*> StringTableImpl; > > In new code we are using "using" rather than typedef. Looks fine! Fixed. >> Source/WTF/wtf/text/StringImpl.h:562 >> + StringImpl& get() > > Not sure that I really love the name get() for this. Maybe downcastStringImpl or downcast<StringImpl> (presumably those would be non-member functions) or maybe operator StringImpl&. This name `get()` is aligned with NeverDestroyed<>::get() thing. But operator StringImpl& seems nice.
Yusuke Suzuki
Comment 7 2017-01-02 18:31:59 PST
Comment on attachment 297916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=297916&action=review >>> Source/WTF/wtf/text/StringImpl.h:562 >>> + StringImpl& get() >> >> Not sure that I really love the name get() for this. Maybe downcastStringImpl or downcast<StringImpl> (presumably those would be non-member functions) or maybe operator StringImpl&. > > This name `get()` is aligned with NeverDestroyed<>::get() thing. But operator StringImpl& seems nice. And to avoid copying StaticStringImpl, I think making it noncopyable is nice.
Yusuke Suzuki
Comment 8 2017-01-02 18:41:13 PST
Created attachment 297920 [details] Patch Patch for landing
Yusuke Suzuki
Comment 9 2017-01-02 18:41:44 PST
Committed r210230: <http://trac.webkit.org/changeset/210230>
Note You need to log in before you can comment on or make changes to this bug.