Bug 91162 – Move WebCore/platform/text/Base64 to WTF/wtf/text

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

[Xianzhu Wang]

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

[Xianzhu Wang]

Created attachment 152301 [details]
Patch (for EWS, not requesting review)

[Adam Barth]

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.

[Build Bot]

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

[Xianzhu Wang]

Created attachment 152336 [details]
Patch

[Xianzhu Wang]

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)

[Adam Barth]

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?

[Adam Barth]

Also ?base64Decode@WTF@@YA_NABVString@1@AAV?$Vector@D$0A@@1@W4Base64DecodePolicy@1@@Z

[Xianzhu Wang]

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

[Xianzhu Wang]

Created attachment 152347 [details]
Add symbols into JavaScriptCore.def

[Adam Barth]

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

[Build Bot]

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

[Adam Barth]

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)

[Xianzhu Wang]

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

[Adam Barth]

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?

[Xianzhu Wang]

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?

[Adam Barth]

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

[Xianzhu Wang]

Created attachment 152378 [details]
Patch

[WebKit Review Bot]

Comment on attachment 152378 [details]
Patch

Clearing flags on attachment: 152378

Committed r122652: <http://trac.webkit.org/changeset/122652>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.