[webkit-reviews] review denied: [Bug 51736] Implement RemoteFontStream's skip behavior (in FontCustomPlatformData.cpp) : [Attachment 77666] Updated patch (fixed style break)

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Xianzhu Wang
<wangxianzhu at google.com>'s request for review:
Bug 51736: Implement RemoteFontStream's skip behavior (in
FontCustomPlatformData.cpp)
https://bugs.webkit.org/show_bug.cgi?id=51736

Attachment 77666: Updated patch (fixed style break)
https://bugs.webkit.org/attachment.cgi?id=77666&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=77666&action=review

> WebCore/ChangeLog:8
> +	   No new tests.

Should say why. We can see there are no new tests.

Is it not testable for some reason?

>>> WebCore/platform/graphics/skia/FontCustomPlatformData.cpp:153
>>>	     if (!m_buffer->data() || !m_buffer->size())
>> 
>> I'm confused.  Your'e removing a null check of buffer, yet using buffer
unconditionally?
> 
> The condition is now at line 157 of the new file. The 'read' and 'skip'
features only differ at line 158.

The change removes the check on buffer (which is the skip bytes feature being
implemented). The code is only using m_buffer (except on line 158 where it does
the check for buffer).

> WebCore/platform/graphics/skia/FontCustomPlatformData.cpp:156
> +	   size_t toReadOrSkip = (left > size) ? size : left;

Seems like this should use std::min.

Also the name "toReadOrSkip" is awkward. How about bytesToRead or
bytesToConsume? (Even though the bytes may not be written to buffer, they are
still "read" or else this function ("read") should be named something
different.)