Bug 51736 – Implement RemoteFontStream's skip behavior (in FontCustomPlatformData.cpp)

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Xianzhu Wang]

The implementation of RemoteFontStream (in WebCore/platform/graphics/skia/FontCustomPlatformData.cpp) is not complete, missing 'skip' feature. Though Chromium port doesn't use the skip feature (it reads the whole stream data at once), other ports using Skia might encounter problems if the stream is passed to Skia. The SkStream::skip() and SkStream::read(NULL, skip) functionalities are used frequently in Skia code. This will cause failures when loading a font from a stream.

[Xianzhu Wang]

Created attachment 77665 [details]
The patch

[WebKit Review Bot]

Attachment 77665 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/graphics/skia/FontCustomPlatformData.cpp']" exit_code: 1
WebCore/platform/graphics/skia/FontCustomPlatformData.cpp:152:  Use 0 instead of NULL.  [readability/null] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Xianzhu Wang]

Created attachment 77666 [details]
Updated patch (fixed style break)

[Eric Seidel (no email)]

Comment on attachment 77666 [details]
Updated patch (fixed style break)

View in context: https://bugs.webkit.org/attachment.cgi?id=77666&action=review

> 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?

[Xianzhu Wang]

(In reply to comment #4)
> (From update of attachment 77666 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77666&action=review
> 
> > 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.

[Xianzhu Wang]

Hi, reviewers?

[David Levin]

Comment on attachment 77666 [details]
Updated patch (fixed style break)

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.)

[Xianzhu Wang]

(In reply to comment #7)

Thanks for review!

> (From update of attachment 77666 [details])
> 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?
>

Added reason of no test in ChangeLog.

> >>> 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.
>

Done.
 
> 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.)

Changed to bytesToConsume.

I agree this is a problem, but the 'read or skip' semantic is defined by SkStream::read() in Skia and we can only follow it.

[Xianzhu Wang]

Created attachment 78084 [details]
updated patch

[David Levin]

Comment on attachment 78084 [details]
updated patch

Sounds like "No new tests because existing test(s) already exercise this functionality (for ports other than Chromium using Skia)." (would have been sufficient).

[WebKit Commit Bot]

Comment on attachment 78084 [details]
updated patch

Clearing flags on attachment: 78084

Committed r75140: <http://trac.webkit.org/changeset/75140>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.