OTS: Adds more layout common table supports....

OTS: Adds more layout common table supports.

Added Script table, Featre table, Lookup table, and Device table support. This is a part of adding GPOS/GSUB table support. The CL also contains unittests for layout common tables. Some fix for bugs which detected by the unittests also included.

BUG=27131
TEST=test/layout_common_table_test.cc

[bashi, 2011-02-03 00:48:41]

Hi,

I'm working on adding GPOS support. The CL could be large so at first I've added
more layout common table support, which are needed to parse GPOS/GSUB tables.
Could you review this?

Thanks,

[Yusuke Sato, 2011-02-04 06:12:52]

http://codereview.chromium.org/6410047/diff/1/include/ots-memory-stream.h
File include/ots-memory-stream.h (right):

http://codereview.chromium.org/6410047/diff/1/include/ots-memory-stream.h#new...
include/ots-memory-stream.h:21: virtual bool WriteRaw(const void *data, size_t
length) {
Please be consistent: if you add virtual to WriteRaw, please do it for Seek and
Tell as well.

http://codereview.chromium.org/6410047/diff/1/src/gdef.cc
File src/gdef.cc (right):

http://codereview.chromium.org/6410047/diff/1/src/gdef.cc#newcode209
src/gdef.cc:209: const unsigned mark_sets_end = static_cast<unsigned>(4) +
2*mark_set_count;
since file->gdef->num_mark_glyph_sets is 16bit unsigned, it might be better to
check integer overflow here. If you agree, please add

if (mark_sets_end > std::numeric_limits<uint16_t>::max()) {
  return OTS_FAILURE();
}

or something similar.

http://codereview.chromium.org/6410047/diff/1/src/layout.cc
File src/layout.cc (right):

http://codereview.chromium.org/6410047/diff/1/src/layout.cc#newcode54
src/layout.cc:54: OTS_FAILURE();
return is missing.

http://codereview.chromium.org/6410047/diff/1/src/layout.cc#newcode92
src/layout.cc:92: OTS_FAILURE();
ditto. please grep your change again.

http://codereview.chromium.org/6410047/diff/1/src/layout.cc#newcode184
src/layout.cc:184: if (lookup_flag & kMarkAttachmentTypeMask &&
I slightly prefer:

if ((lookup_flag & kMarkAttachmentTypeMask) &&

since the operator precedence of bitwise-AND is sometimes confusing (higher than
&& but lower than ==, as you should know.)

[bashi, 2011-02-07 01:17:07]

Hi Yusuke-san,

Thank you for review. I've revised the CL. (Sorry for bothering you about
pointing out missing returns again..)

Thanks!

http://codereview.chromium.org/6410047/diff/1/include/ots-memory-stream.h
File include/ots-memory-stream.h (right):

http://codereview.chromium.org/6410047/diff/1/include/ots-memory-stream.h#new...
include/ots-memory-stream.h:21: virtual bool WriteRaw(const void *data, size_t
length) {
On 2011/02/04 06:12:52, Yusuke Sato wrote:
> Please be consistent: if you add virtual to WriteRaw, please do it for Seek
and
> Tell as well.

Done.

http://codereview.chromium.org/6410047/diff/1/src/gdef.cc
File src/gdef.cc (right):

http://codereview.chromium.org/6410047/diff/1/src/gdef.cc#newcode209
src/gdef.cc:209: const unsigned mark_sets_end = static_cast<unsigned>(4) +
2*mark_set_count;
On 2011/02/04 06:12:52, Yusuke Sato wrote:
> since file->gdef->num_mark_glyph_sets is 16bit unsigned, it might be better to
> check integer overflow here. If you agree, please add
> 
> if (mark_sets_end > std::numeric_limits<uint16_t>::max()) {
>   return OTS_FAILURE();
> }
> 
> or something similar.

Done.

http://codereview.chromium.org/6410047/diff/1/src/layout.cc
File src/layout.cc (right):

http://codereview.chromium.org/6410047/diff/1/src/layout.cc#newcode54
src/layout.cc:54: OTS_FAILURE();
On 2011/02/04 06:12:52, Yusuke Sato wrote:
> return is missing.

Done.

http://codereview.chromium.org/6410047/diff/1/src/layout.cc#newcode92
src/layout.cc:92: OTS_FAILURE();
On 2011/02/04 06:12:52, Yusuke Sato wrote:
> ditto. please grep your change again.

Done.

http://codereview.chromium.org/6410047/diff/1/src/layout.cc#newcode184
src/layout.cc:184: if (lookup_flag & kMarkAttachmentTypeMask &&
On 2011/02/04 06:12:52, Yusuke Sato wrote:
> I slightly prefer:
> 
> if ((lookup_flag & kMarkAttachmentTypeMask) &&
> 
> since the operator precedence of bitwise-AND is sometimes confusing (higher
than
> && but lower than ==, as you should know.)

Done.

[Yusuke Sato, 2011-02-07 03:44:27]

lgtm

On 2011/02/07 01:17:07, bashik wrote:
> Hi Yusuke-san,
> 
> Thank you for review. I've revised the CL. (Sorry for bothering you about
> pointing out missing returns again..)
> 
> Thanks!
> 
> http://codereview.chromium.org/6410047/diff/1/include/ots-memory-stream.h
> File include/ots-memory-stream.h (right):
> 
>
http://codereview.chromium.org/6410047/diff/1/include/ots-memory-stream.h#new...
> include/ots-memory-stream.h:21: virtual bool WriteRaw(const void *data, size_t
> length) {
> On 2011/02/04 06:12:52, Yusuke Sato wrote:
> > Please be consistent: if you add virtual to WriteRaw, please do it for Seek
> and
> > Tell as well.
> 
> Done.
> 
> http://codereview.chromium.org/6410047/diff/1/src/gdef.cc
> File src/gdef.cc (right):
> 
> http://codereview.chromium.org/6410047/diff/1/src/gdef.cc#newcode209
> src/gdef.cc:209: const unsigned mark_sets_end = static_cast<unsigned>(4) +
> 2*mark_set_count;
> On 2011/02/04 06:12:52, Yusuke Sato wrote:
> > since file->gdef->num_mark_glyph_sets is 16bit unsigned, it might be better
to
> > check integer overflow here. If you agree, please add
> > 
> > if (mark_sets_end > std::numeric_limits<uint16_t>::max()) {
> >   return OTS_FAILURE();
> > }
> > 
> > or something similar.
> 
> Done.
> 
> http://codereview.chromium.org/6410047/diff/1/src/layout.cc
> File src/layout.cc (right):
> 
> http://codereview.chromium.org/6410047/diff/1/src/layout.cc#newcode54
> src/layout.cc:54: OTS_FAILURE();
> On 2011/02/04 06:12:52, Yusuke Sato wrote:
> > return is missing.
> 
> Done.
> 
> http://codereview.chromium.org/6410047/diff/1/src/layout.cc#newcode92
> src/layout.cc:92: OTS_FAILURE();
> On 2011/02/04 06:12:52, Yusuke Sato wrote:
> > ditto. please grep your change again.
> 
> Done.
> 
> http://codereview.chromium.org/6410047/diff/1/src/layout.cc#newcode184
> src/layout.cc:184: if (lookup_flag & kMarkAttachmentTypeMask &&
> On 2011/02/04 06:12:52, Yusuke Sato wrote:
> > I slightly prefer:
> > 
> > if ((lookup_flag & kMarkAttachmentTypeMask) &&
> > 
> > since the operator precedence of bitwise-AND is sometimes confusing (higher
> than
> > && but lower than ==, as you should know.)
> 
> Done.