[webkit-reviews] review denied: [Bug 39958] [Qt] TextBreakIteratorQt performance : [Attachment 76718] removed extra argument passed to the fucntion

[bugzilla-daemon at webkit.org]

Holger Freyther <zecke at selfish.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 76718: removed extra argument passed to the fucntion
https://bugs.webkit.org/attachment.cgi?id=76718&action=review

------- Additional Comments from Holger Freyther <zecke at selfish.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=76718&action=review

I am not convinced based on the commit message. Please explain in the commit
message where this speed up is coming from.


> WebCore/ChangeLog:12
> +

I don't understand this message. Are you fixing a crash introduced by a
previous commit? Or is this patch improving the performance? Where is this
speed up coming from?

> WebCore/platform/text/qt/TextBreakIteratorQt.cpp:47
> +	       : QTextBoundaryFinder(type, string)

This is just using 'QString String::operator()'. Is there any change here?
Avoiding a deep copy or such?

> WebCore/platform/text/qt/TextBreakIteratorQt.cpp:-65
> -	       && memcmp(string, iterator.string, length) == 0) {

Where is the speed up coming from? Avoiding the memcmp as
equal(StringImpl*,StringImpl*) is more clever? Is iterator.isValid() or
iterator.type() doing more work?