Bug 11540 – Port of WebKit ToT to S60

• Component: Platform
• Status: CLOSED FIXED

[zalan]

This bug brings S60 to WebKit ToT.

[zalan]

Created attachment 11417 [details]
contains basic types like point, rect, size and string on symbian platform

[zalan]

Created attachment 11418 [details]
platform/PlatformString.h changes were missing from the prev patch

[Maciej Stachowiak]

Comment on attachment 11418 [details]
platform/PlatformString.h changes were missing from the prev patch

Could you explain these changes a little more:

+#if PLATFORM(SYMBIAN)
+#undef CHECK_FOR_HANDLE_LEAKS
+// tot:fixme need page aligned allocations
+#define CHECK_FOR_HANDLE_LEAKS 1
+#endif
+

+#elif PLATFORM(SYMBIAN)
+    // fixme needs to do page aligned allocation
+    block = NULL;

Does Symbian not have a way to allocate at page boundaries? If so you will probably also have problems with JavaScriptCore.


I do't think it's necessary to put the comment below in the code, we usually just put stuff like that in the ChangeLog unless what it's doing is really weird:

+    // char _internalBuffer has to be in front of the bitfields as
+    // Codewarrior (3.2.5 build 461) compiler cannot cope with bitfields and breaks byte aligment
+    char _internalBuffer[WEBCORE_DS_INTERNAL_BUFFER_SIZE]; // Pad out to a (((size + 1) & ~15) + 14) size
+

I suggest using lowercase "symbian" as the platform directory name, we usually use lowercase even when the official name of the relevant platform is capitalized:

Index: WebCore/platform/Symbian/DeprecatedStringSymbian.cpp

The BSD license in DeprecatedStringSymbian.cpp doesn't quite match the BSD license in the rest of WebKit. Mich of the difference is cosmetic, but it also has the following extra clause:

+*      * Neither the name of the Nokia Corporation nor the names of its
+*        contributors may be used to endorse or promote products derived
+*        from this software without specific prior written permission.

Is there any problem with using the usual two-clause BSD license that's in other WebKit BSD-licensed code? Please use that if possible. The same applies to a couple of other files.

Did you mean to include the patent license here? There's no WebKitS60 directory on the trunk otherwise. We'll have to check with our legal department to include this, I sent an email. In the meantime, would it be ok to submit the rest of the patch without this? I don't think the patent covers anything submitted so far.

r- for now. Please address these comments and resubmit. Thanks!

[Antti Koivisto]

No, Symbian does not have valloc() or anything similar. There is no easy way to do page aligned allocations as far as I can find out.

[Yongjun Zhang]

valloc is not internal supported by Symbian. The good news is that we are going to use dlmalloc to replace Symbian's default allocator, and we can simulate page-aligned memory allocation by using dlvalloc.

[zalan]

yes, i think we can submit the patch without the patentlicense.txt. 
Nokia uses this modified BSD license text throughout its (BSD)open source contribution but I will check with the legal department whether that is something we can change for WebKit. 
Do you think if you could land the code with the current license header as the legal check could take a while and i really don't want to block commits just because of that?

[zalan]

Created attachment 11444 [details]
basic types for Symbian platform

-lowercase symbian dir
-moved byte alignment comment from .h to WebCore/Changlog
-removed WebKitS60 dir
-haven't received anything back on the modified BSD header so this patch has the same header as the previous patch had (except the removed patentlicense text)

[Maciej Stachowiak]

OK, the code changes all look great, let me check on the license on our end.

[Maciej Stachowiak]

License is fine too. r=me. Yay for the first S60 merge patch!

[Mark Rowe (bdash)]

Landed in r17722.