Bug 91162

Summary: Move WebCore/platform/text/Base64 to WTF/wtf/text
Product: WebKit Reporter: Xianzhu Wang <wangxianzhu>
Component: Web Template FrameworkAssignee: Xianzhu Wang <wangxianzhu>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, danw, darin, gns, japhet, jochen, mifenton, mrobinson, paroga, rakuco, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch (for EWS, not requesting review)
none
Patch
none
Add symbols into JavaScriptCore.def
none
Patch none

Description Xianzhu Wang 2012-07-12 16:23:53 PDT
Base64 looks a generic function what would be useful if it's part of WTF. The actual reason is that I want to use Base64 in Chromium DumpRenderTree (which directly depends on only chromium webkit_support, chromium WebKit API and WTF).
Comment 1 Xianzhu Wang 2012-07-13 10:52:55 PDT
Created attachment 152301 [details]
Patch (for EWS, not requesting review)
Comment 2 Adam Barth 2012-07-13 11:07:28 PDT
Comment on attachment 152301 [details]
Patch (for EWS, not requesting review)

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

> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:103
> -    return base64Encode(reinterpret_cast<char*>(key), nonceSize);
> +    return WTF::base64Encode(reinterpret_cast<char*>(key), nonceSize);

Typically we put a "using WTF::base64Encode" in the header so we don't need to use the WTF prefix everywhere.  Take a look at RefPtr, for example.
Comment 3 Build Bot 2012-07-13 11:22:38 PDT
Comment on attachment 152301 [details]
Patch (for EWS, not requesting review)

Attachment 152301 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13232313
Comment 4 Xianzhu Wang 2012-07-13 14:40:52 PDT
Created attachment 152336 [details]
Patch
Comment 5 Xianzhu Wang 2012-07-13 14:43:11 PDT
Comment on attachment 152301 [details]
Patch (for EWS, not requesting review)

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

I've no idea how to successfully build on Windows. What did I miss?

>> Source/WebCore/Modules/websockets/WebSocketHandshake.cpp:103
>> +    return WTF::base64Encode(reinterpret_cast<char*>(key), nonceSize);
> 
> Typically we put a "using WTF::base64Encode" in the header so we don't need to use the WTF prefix everywhere.  Take a look at RefPtr, for example.

Done. (I had made a version like that but removed the using directives before uploading the patch because of potential name conflicts in Source/WebKit/blackberry. Now I found it's easy to overcome the conflict)
Comment 6 Adam Barth 2012-07-13 14:47:56 PDT
You probably need to export ?base64Encode@WTF@@YA?AVString@1@PBDI_N@Z in some def file.  Maybe grep the def files for exports related to WTF?
Comment 7 Adam Barth 2012-07-13 14:48:27 PDT
Also ?base64Decode@WTF@@YA_NABVString@1@AAV?$Vector@D$0A@@1@W4Base64DecodePolicy@1@@Z
Comment 8 Xianzhu Wang 2012-07-13 15:02:41 PDT
(In reply to comment #7)
> Also ?base64Decode@WTF@@YA_NABVString@1@AAV?$Vector@D$0A@@1@W4Base64DecodePolicy@1@@Z

Oh. I was thinking 'WTF_EXPORT_PRIVATE's were enough. It looks weird that we need to export WTF symbols in JavaScriptCore.def. Do we need to export all symbols with WTF_EXPORT_PRIVATE, or only the symbols used (which caused unresolved external error)?
Comment 9 Xianzhu Wang 2012-07-13 15:04:02 PDT
Created attachment 152347 [details]
Add symbols into JavaScriptCore.def
Comment 10 Adam Barth 2012-07-13 15:05:36 PDT
> Oh. I was thinking 'WTF_EXPORT_PRIVATE's were enough.

Windows might not be smart enough to use that macro.

> It looks weird that we need to export WTF symbols in JavaScriptCore.def.

WebCore.dll gets its WTF from JavaScriptCore.dll.

> Do we need to export all symbols with WTF_EXPORT_PRIVATE, or only the symbols used (which caused unresolved external error)?

Just the ones that cause link errors.
Comment 11 Build Bot 2012-07-13 16:02:18 PDT
Comment on attachment 152347 [details]
Add symbols into JavaScriptCore.def

Attachment 152347 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13246181
Comment 12 Adam Barth 2012-07-13 16:08:27 PDT
Looks like here are two more:

4>JavaScriptCore.exp : error LNK2001: unresolved external symbol "class WTF::String __cdecl WTF::base64Encode(char const *,unsigned int,bool)" (?base64Encode@WTF@@YA?AVString@1@PBDI_N@Z)
4>JavaScriptCore.exp : error LNK2001: unresolved external symbol "void __cdecl WTF::base64Encode(char const *,unsigned int,class WTF::Vector<char,0> &,bool)" (?base64Encode@WTF@@YAXPBDIAAV?$Vector@D$0A@@1@_N@Z)
Comment 13 Xianzhu Wang 2012-07-13 16:21:59 PDT
(In reply to comment #12)
> Looks like here are two more:
> 
> 4>JavaScriptCore.exp : error LNK2001: unresolved external symbol "class WTF::String __cdecl WTF::base64Encode(char const *,unsigned int,bool)" (?base64Encode@WTF@@YA?AVString@1@PBDI_N@Z)
> 4>JavaScriptCore.exp : error LNK2001: unresolved external symbol "void __cdecl WTF::base64Encode(char const *,unsigned int,class WTF::Vector<char,0> &,bool)" (?base64Encode@WTF@@YAXPBDIAAV?$Vector@D$0A@@1@_N@Z)

Actually these were added in the last patch, but I forgot that I changed the signature of the functions (bool -> Base64EncodePolicy). Will fix that asap.
Comment 14 Adam Barth 2012-07-13 16:28:13 PDT
Comment on attachment 152347 [details]
Add symbols into JavaScriptCore.def

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

> Source/WTF/WTF.xcodeproj/project.pbxproj:771
> +				8134013615B092FD001FF0B8 /* Base64.cpp */,
> +				8134013715B092FD001FF0B8 /* Base64.h */,

You might want to run ./Tools/Scripts/sort-xcodeproj

> Source/WebKit/blackberry/Api/WebKitTextCodec.cpp:105
> +    // Must make sure Base64EncodePolicy is the same in WebKit and WTF!

Maybe add a COMPILE_ASSERT?
Comment 15 Xianzhu Wang 2012-07-13 16:51:02 PDT
Comment on attachment 152347 [details]
Add symbols into JavaScriptCore.def

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

>> Source/WTF/WTF.xcodeproj/project.pbxproj:771
>> +				8134013715B092FD001FF0B8 /* Base64.h */,
> 
> You might want to run ./Tools/Scripts/sort-xcodeproj

I just tried it but it produced 78 lines of change to the file. Is that expected?
Comment 16 Adam Barth 2012-07-13 16:55:56 PDT
> I just tried it but it produced 78 lines of change to the file. Is that expected?

That just means it hasn't been sorted in a while.  I would sort it, but you shouldn't feel like you need to.  Sort it is mostly about avoiding merge conflicts, but if you're getting 78 lines of change, that probably means you're raising your chance of a merge conflict.
Comment 17 Xianzhu Wang 2012-07-13 17:07:33 PDT
Created attachment 152378 [details]
Patch
Comment 18 WebKit Review Bot 2012-07-13 18:03:28 PDT
Comment on attachment 152378 [details]
Patch

Clearing flags on attachment: 152378

Committed r122652: <http://trac.webkit.org/changeset/122652>
Comment 19 WebKit Review Bot 2012-07-13 18:03:34 PDT
All reviewed patches have been landed.  Closing bug.