[webkit-reviews] review denied: [Bug 25116] Move VDMX parsing into the Chromium Linux port. : [Attachment 29376] patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has denied Adam Langley
<agl at chromium.org>'s request for review:
Bug 25116: Move VDMX parsing into the Chromium Linux port.
https://bugs.webkit.org/show_bug.cgi?id=25116

Attachment 29376: patch
https://bugs.webkit.org/attachment.cgi?id=29376&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
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.