[webkit-reviews] review denied: [Bug 46695] [Qt] Invalid pointer access & incomplete memcmp in setUpIterator : [Attachment 82806] (fix to chromium build break)

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied chris reiss
<christopher.reiss at nokia.com>'s request for review:
Bug 46695: [Qt] Invalid pointer access & incomplete memcmp in setUpIterator
https://bugs.webkit.org/show_bug.cgi?id=46695

Attachment 82806: (fix to chromium build break)
https://bugs.webkit.org/attachment.cgi?id=82806&action=review

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

> Source/WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

This line should be replaced, either mention what tests are added, or explain
why none are needed.

> Source/WebCore/ChangeLog:9
> +

I'm missing a comment block here explaining that you're adding a way for
TextBreakIterators to hold a reference to the last string they operate on for
reuse purposes.

> Source/WebCore/platform/text/qt/TextBreakIteratorQt.cpp:48
> +	   TextBreakIterator(QTextBoundaryFinder::BoundaryType type, const
UChar* string, int length, PassRefPtr<StringImpl> origText = 0)

"origText" is a bad variable name, we stay away from abbreviations in WebKit
style. Repeated below.

> Source/WebCore/platform/text/gtk/TextBreakIteratorGtk.cpp:244
> +TextBreakIterator* lineBreakIterator(const UChar* string, int length,
PassRefPtr<StringImpl> origText = 0 /* used for memory-safe recycling of
Iterator */)

No need for the " = 0 /* used for memory-safe recycling of Iterator */" here.

> Source/WebCore/rendering/break_lines.h:28
> +typedef PassRefPtr<StringImpl> Blerk; 

Blerk?