[webkit-reviews] review denied: [Bug 54912] Minimize calls to ubrk_setText() : [Attachment 83579] Final changes.

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied nholbrook at apple.com's request for review:
Bug 54912: Minimize calls to ubrk_setText()
https://bugs.webkit.org/show_bug.cgi?id=54912

Attachment 83579: Final changes.
https://bugs.webkit.org/attachment.cgi?id=83579&action=review

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=83579&action=review

> Source/WebCore/ChangeLog:34
> +2011-02-23  Ned Holbrook  <nholbrook at apple.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Minimize calls to ubrk_setText()
> +	   https://bugs.webkit.org/show_bug.cgi?id=54912
> +	   <rdar://problem/9032774>
> +
> +	   No new tests. (OOPS!)
> +
> +	   * platform/text/TextBreakIterator.h:
> +	   (WebCore::LazyLineBreakIterator::LazyLineBreakIterator):
> +	   (WebCore::LazyLineBreakIterator::~LazyLineBreakIterator):
> +	   (WebCore::LazyLineBreakIterator::string):
> +	   (WebCore::LazyLineBreakIterator::length):
> +	   (WebCore::LazyLineBreakIterator::get):
> +	   (WebCore::LazyLineBreakIterator::reset):
> +	   * platform/text/TextBreakIteratorICU.cpp:
> +	   (WebCore::acquireLineBreakIterator):
> +	   (WebCore::releaseLineBreakIterator):
> +	   * platform/text/qt/TextBreakIteratorQt.cpp:
> +	   (WebCore::acquireLineBreakIterator):
> +	   (WebCore::releaseLineBreakIterator):
> +	   * rendering/RenderBlock.h:
> +	   * rendering/RenderBlockLineLayout.cpp:
> +	   (WebCore::RenderBlock::layoutInlineChildren):
> +	   (WebCore::RenderBlock::findNextLineBreak):
> +	   * rendering/RenderText.cpp:
> +	   (WebCore::RenderText::computePreferredLogicalWidths):
> +	   * rendering/break_lines.cpp:
> +	   (WebCore::nextBreakablePosition):
> +	   * rendering/break_lines.h:
> +	   (WebCore::isBreakable):
> +

Sorry, Ned, this is not how it works (even if some contributors sometimes get
away with it) :-(.

For a change like this, at the very least you should give an overview of the
change before the file and function listing. This would also be a great place
to say by how much the change reduces calls to ubrk_setText(). Alternatively,
or in addition to this, you should fill out the function listing with comments
about the changes in each function, where it makes sense.

Finally, you should remove the “No new tests. (OOPS!)” line. The change log
can’t be committed with that line, and instead you can comment that there are
no new tests because this is only a performance optimization. But just removing
the line is also fine, in my opinion.

Thanks!