[webkit-reviews] review denied: [Bug 39958] [Qt] TextBreakIteratorQt performance : [Attachment 76640] Fix as per the commnet

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Raju Kunnath
<raju.kunnath at nokia.com>'s request for review:
Bug 39958: [Qt] TextBreakIteratorQt performance
https://bugs.webkit.org/show_bug.cgi?id=39958

Attachment 76640: Fix as per the commnet
https://bugs.webkit.org/attachment.cgi?id=76640&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76640&action=review

This solution is OK as a temporary workaround, but we should really fix this
properly to avoid deep-copying the string.

r- for using String(UChar*) instead of String(UChar*, int).

> WebCore/ChangeLog:12
> +	   The previous fix for the performance improvement had a reference to
a
> +	   pointer which is deleted and hence the memcmp crashed. A check is
added
> +	   to validate the string is alive or not. The root cause of this issue
is
> +	   that in Symbian the Fastallocator claim the memory fast and hence
the crash.
> +	   Also this is a sporadic crash.

"A check is added to validate the string is alive or not." - This sentence no
longer applies.

> WebCore/ChangeLog:14
> +	   Tested the patch on QtTestBrowser on S60 devices and windows.

This is implied and can be left out.

> WebCore/platform/text/qt/TextBreakIteratorQt.cpp:51
> +	       , string("") {} 

Is there any reason to initialize string with String("") instead of String()?

> WebCore/platform/text/qt/TextBreakIteratorQt.cpp:60
> +	   String localString = string;

This will cause an unnecessary loop over 'string' to determine its length.
Should be:

String localString(string, length);