[Webkit-unassigned] [Bug 54912] Minimize calls to ubrk_setText()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54912


mitz at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #83579|review?                     |review-
               Flag|                            |




--- Comment #19 from mitz at webkit.org  2011-02-23 16:59:11 PST ---
(From update of attachment 83579)
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!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.