[Webkit-unassigned] [Bug 25116] Move VDMX parsing into the Chromium Linux port.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Apr 9 16:02:02 PDT 2009


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


fishd at chromium.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #29376|review?(fishd at chromium.org) |review-
               Flag|                            |




------- Comment #5 from fishd at chromium.org  2009-04-09 16:02 PDT -------
(From update of attachment 29376)
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.


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



More information about the webkit-unassigned mailing list