Bug 11540 - Port of WebKit ToT to S60
Summary: Port of WebKit ToT to S60
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 420+
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-11-07 11:21 PST by zalan
Modified: 2008-06-11 00:21 PDT (History)
7 users (show)

See Also:


Attachments
contains basic types like point, rect, size and string on symbian platform (32.88 KB, patch)
2006-11-07 12:56 PST, zalan
no flags Details | Formatted Diff | Diff
platform/PlatformString.h changes were missing from the prev patch (33.56 KB, patch)
2006-11-07 15:18 PST, zalan
mjs: review-
Details | Formatted Diff | Diff
basic types for Symbian platform (31.04 KB, patch)
2006-11-09 12:15 PST, zalan
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description zalan 2006-11-07 11:21:29 PST
This bug brings S60 to WebKit ToT.
Comment 1 zalan 2006-11-07 12:56:02 PST
Created attachment 11417 [details]
contains basic types like point, rect, size and string on symbian platform
Comment 2 zalan 2006-11-07 15:18:03 PST
Created attachment 11418 [details]
platform/PlatformString.h changes were missing from the prev patch
Comment 3 Maciej Stachowiak 2006-11-07 22:02:24 PST
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!
Comment 4 Antti Koivisto 2006-11-08 09:13:49 PST
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.
Comment 5 Yongjun Zhang 2006-11-08 09:32:06 PST
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.
Comment 6 zalan 2006-11-08 10:21:30 PST
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?
Comment 7 zalan 2006-11-09 12:15:33 PST
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)
Comment 8 Maciej Stachowiak 2006-11-10 03:33:55 PST
OK, the code changes all look great, let me check on the license on our end.

Comment 9 Maciej Stachowiak 2006-11-10 14:44:01 PST
License is fine too. r=me. Yay for the first S60 merge patch!
Comment 10 Mark Rowe (bdash) 2006-11-10 19:04:43 PST
Landed in r17722.