[webkit-reviews] review denied: [Bug 11540] Port of WebKit ToT to S60 : [Attachment 11418] platform/PlatformString.h changes were missing from the prev patch

[bugzilla-request-daemon at macosforge.org]

Maciej Stachowiak <mjs at apple.com> has denied Maciej Stachowiak
<mjs at apple.com>'s request for review:
Bug 11540: Port of WebKit ToT to S60
http://bugs.webkit.org/show_bug.cgi?id=11540

Attachment 11418: platform/PlatformString.h changes were missing from the prev
patch
http://bugs.webkit.org/attachment.cgi?id=11418&action=edit

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
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!