Bug 25116 – Move VDMX parsing into the Chromium Linux port.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Adam Langley]

Move VDMX parsing into the Chromium Linux port.

VDMX tables are optional tables in TrueType fonts which contain the exact pixel height of a given font at a given pel size. In order to match Windows font metrics we have to use these numbers.

Previously, the parsing was performed in Skia. As part of the merge with upstream Skia, an interface for getting table data from a font has been added to Skia and we're moving the parsing into WebKit.

[Adam Langley]

Created attachment 29367 [details]
patch

[Mark Rowe (bdash)]

Comment on attachment 29367 [details]
patch

This code doesn't seem to follow the Webkit naming and style conventions.  Functions should not have an initial uppercase letter and arguments should not be named with lower_case_and_underscores.  The "Chromium" prefix on ChromiumVDMXParse is a bit strange.

[Adam Langley]

Heck, sorry. I'm switching between so many coding styles that I forgot about WebKit's lowercase method names. I'll fix.

[Adam Langley]

Created attachment 29376 [details]
patch

Changed method names to camelCase (my bad). Changed the argument names in the prototype to camalCase too (missed them before). Renamed ChromiumVDMXParse to trueTypeVDMXParse. (Just aiming for the donkey's behind here, I'll take any suggestions as to the correct name for this function.)

cheers

[Darin Fisher (:fishd, Google)]

Comment on attachment 29376 [details]
patch

I don't know much about TrueType fonts or VDMX tables.  Is there someone on the
Chrome team who can review this solution?  Dean perhaps?

Just nits:


> +++ b/WebCore/platform/graphics/chromium/FontPlatformDataLinux.h

> +    // -------------------------------------------------------------------------
> +    // Return Skia's unique id for this font. This encodes both the style and
> +    // the font's file name so refers to a single face.
> +    // -------------------------------------------------------------------------

nit: this block comment style is not used very uniformly in our code...  it seems a bit
jarring to me, but i guess that's just personal preference.  consistency across our
code would be nice.


> +++ b/WebCore/platform/graphics/chromium/FontTrueTypeLinux.cpp

nit: We should be using the suffix ChromiumLinux.cpp for linux specific files
in the chromium/ directory.  We have not been very consistent with this in the
Linux specific files, which is unfortunate.  Maybe you can at least fix this
one that you are adding.


> +class Buffer
> +{
> +  public:

nit: please use webkit style, which is:

  class Buffer {
  public:
      Buffer(...


> +    Buffer(const uint8_t* buffer, size_t length)
> +        : m_buffer(buffer)
> +        , m_length(length)
> +        , m_offset(0)
> +        { }

nit: webkit style would probably either be:

    Buffer(const uint8_t* buffer, size_t length)
        : m_buffer(buffer)
        , m_length(length)
        , m_offset(0) { }

or:

    Buffer(const uint8_t* buffer, size_t length)
        : m_buffer(buffer)
        , m_length(length)
        , m_offset(0)
    {
    }


> +    bool readU8(uint8_t* value)
> +    {
> +        if (m_offset + 1 > m_length)
> +            return false;
> +        *value = m_buffer[m_offset];
> +        m_offset++;
> +        return true;
> +    }
> +
> +    bool readU16(uint16_t* value)
> +    {
> +        if (m_offset + 2 > m_length)
> +            return false;
> +        memcpy(value, m_buffer + m_offset, sizeof(uint16_t));
> +        *value = ntohs(*value);
> +        m_offset += 2;
> +        return true;
> +    }

how about using sizeof(*value) or sizeof(uint16_t) and sizeof(uint8_t),
respectively, instead of the magic 1 and 2 constants?


> +  private:
> +    const uint8_t *const m_buffer;

nit: "private:" should not be indented.

nit: "const uint8_t* const m_buffer;"


> +// Freetype does not parse these tables so we do so here. In the future we
> +// might support loading of arbitary fonts. This is not something that one
> +// would wish to do, dangerous as it is, so we are careful where we tread.

we have code to load arbitrary truetype fonts from the web.  maybe we need to
worry sooner than later?


> +bool trueTypeVDMXParse(int* ymax, int* ymin,
> +                       const uint8_t* vdmx, const size_t vdmxLength,
> +                       const unsigned targetPelSize)

nit: yMax and yMin to conform to webkit variable naming conventions

nit: please use Pixel instead of Pel since Pel is not that common.


> +    if (!buf.skip(4) ||
> +        !buf.readU16(&numRatios))

nit: webkit style is to put the "||" on the next line or to just avoid the line
break, which is what i would do here.

> +        if (!buf.skip(1) ||
> +            !buf.readU8(&xRatio) ||
> +            !buf.readU8(&yRatio1) ||
> +            !buf.readU8(&yRatio2))
> +            return false;

same nit here and below

> +
> +        // This either covers 1:1, or this is the default entry (0, 0, 0)
> +        if ((xRatio == 1 && yRatio1 <= 1 && yRatio2 >= 1) ||
> +            (xRatio == 0 && yRatio1 == 0 && yRatio2 == 0)) {
> +            desiredRatio = i;
> +            break;
> +        }

as ugly as it may be, you are supposed to do:

  if (foo
      || bar) {
  }

and perhaps that is why most people just do:

  if (foo || bar) {

despite how long it can make some lines :(



> +    if (desiredRatio == 0xffffffff)  // no ratio found
> +        return false;

uber nits: one space instead of two before "//"


> +    if (!buf.readU16(&numRecords) ||
> +        !buf.skip(sizeof(uint16_t)))
> +        return false;

ding


> +            if (!buf.readS16(&tempYMax) ||
> +                !buf.readS16(&tempYMin))

nit: || is wrongly placed


> +            return true;
> +        } else {

nit: no need for else after return


> +          if (!buf.skip(2 * sizeof(int16_t)))

nit: should be 4 white spaces


> +++ b/WebCore/platform/graphics/chromium/FontTrueTypeLinux.h

same nit about file naming

maybe these files should be named VDMXParser.{h,cpp}?  it doesn't seem very
linux specific (though you are only using it in the chromium-linux port).


> +namespace WebCore {
> +    bool trueTypeVDMXParse(int* ymax, int* ymin,
> +                           const uint8_t* vdmx, const size_t vdmxLength,
> +                           const unsigned targetPelSize);

parseVDMX might be a nicer name?  seems like it would be more conventional
to list the out parameters last.

nit: "const size_t vdmxLength" should just be "size_t vdmxLength".  putting
const in front of a parameter type that is copy-by-value is extraneous.
same goes for targetPelSize.


> +}  // namespace WebCore

nit: one space before "//"



> +++ b/WebCore/platform/graphics/chromium/SimpleFontDataLinux.cpp

> +        uint8_t *vdmxTable = (uint8_t *) malloc(vdmxSize);
> +        if (vdmxTable
> +            && SkFontHost::GetTableData(fontID, vdmxTag, 0, vdmxSize, vdmxTable) == vdmxSize
> +            && trueTypeVDMXParse(&vdmxAscent, &vdmxDescent, vdmxTable, vdmxSize, pelSize))
> +            isVDMXValid = true;
> +        free(vdmxTable);

nit: please use fastMalloc and fastFree, which are what webkit uses instead
of malloc and free.

[Adam Langley]

Created attachment 29393 [details]
patch

Have fixed all the comments. Clearly I need to write a cpplint.py for WebKit style!

The code is previously from Skia and deanm reviewed it when it went in there. It has only changed in terms of style, not function, so should still be good.

[Darin Fisher (:fishd, Google)]

> Have fixed all the comments. Clearly I need to write a cpplint.py for WebKit
> style!

Yeah, it is painful to switch between coding styles :(


> The code is previously from Skia and deanm reviewed it when it went in there.
> It has only changed in terms of style, not function, so should still be good.

In the future, please note this and include a link to where the code review occurred.  Thanks!

[Darin Fisher (:fishd, Google)]

Landed as:  http://trac.webkit.org/changeset/42548