[Devel] gvar patch reviewing

[Masatake YAMATO]

Here are random comments about ttgxvar.c. 

1. As Werner write, stream related code is bit odd and complext 
   to ME. How do you think introducing a sturcture and 
   FT_FRAME_START/FT_FRAME_END macros?
   In some case, above approach is overkill, slower and memory
   hog. However it is easy for me to read.

   My fvar code was not completed. But it may be good to
   be compared with George's one.

MINE/MINE/MINE/MINE/MINE/MINE/MINE/MINE/MINE/MINE/MINE/MINE/MINE

  FT_LOCAL ( FT_Error )
  gx_face_load_fvar ( GX_Face face,
                      FT_Stream stream,
                      GX_Fvar fvar )
  {
    FT_Error error;
    FT_UShort i, j;
    FT_Memory memory = stream->memory;

    static const FT_Frame_Field fvar_fields[] =
      {
#undef FT_STRUCTURE
#define FT_STRUCTURE GX_FvarRec
        FT_FRAME_START ( 14 ),
            FT_FRAME_LONG   ( version ),
            FT_FRAME_USHORT ( offsetToData ),
            FT_FRAME_USHORT ( countSizePairs ),
            FT_FRAME_USHORT ( axisCount ),
            FT_FRAME_USHORT ( axisSize ),
            FT_FRAME_USHORT ( instanceCount ),
            FT_FRAME_USHORT ( instanceSize ),
        FT_FRAME_END
      };
      ...

    if ( FT_STREAM_READ_FIELDS( fvar_fields, fvar ) )
      goto Failure;

YOURS/YOURS/YOURS/YOURS/YOURS/YOURS/YOURS/YOURS/YOURS/YOURS/YOURS

      if ( FT_FRAME_ENTER( table_len ) )
        goto Exit;

      fvar_start = FT_Stream_FTell( stream );
      version = FT_GET_LONG();
      data_off = FT_GET_USHORT();
      cnt = FT_GET_USHORT();
      axis_cnt = FT_GET_USHORT();
      axis_size = FT_GET_USHORT();
      instance_cnt = FT_GET_USHORT();
      instance_size = FT_GET_USHORT();

END/END/END/END/END/END/END/END/END/END/END/END/END/END/END/END/END


2. You change the variable names from the fields in the Apple's
   spec. I think it is good they should have the same name.
   e.g. 

   In spec: offsetToData <=> Your code: data_off

3. Tags
   How do you think moving following tags to ttgxvar.h?
        if ( a->tag == FT_MAKE_TAG( 'w', 'g', 'h', 't' ) )
          a->name = "Weight";
        else if ( a->tag == FT_MAKE_TAG( 'w', 'd', 't', 'h' ) )
          a->name = "Width";
        else if ( a->tag == FT_MAKE_TAG( 'o', 'p', 's', 'z' ) )
          a->name = "OpticalSize";
        else if ( a->tag == FT_MAKE_TAG( 's', 'l', 'n', 't' ) )


4. Could you give a name to magic value/number in the source code
   with using the term used in the Apple's spec?

  static FT_Fixed
  ft_var_apply_tuple(GX_Blend  blend,
                     FT_UShort tupleIndex,
                     FT_Fixed *tuple_coords,
                     FT_Fixed *im_start_coords,
                     FT_Fixed *im_end_coords )
                     ...
  else if ( !(tupleIndex&0x4000 ) )
                         ^^^^^^// Give this a name.


  In gxlayout, I give the name like: 

  typedef enum
  {
    GX_FEAT_MASK_EXCLUSIVE_SETTINGS = 0x8000,
    GX_FEAT_MASK_DYNAMIC_DEFAULT    = 0x4000,
    GX_FEAT_MASK_UNUSED             = 0x3F00,
    GX_FEAT_MASK_DEFAULT_SETTING    = 0x00FF
  } GX_FeatureFlagsMask ;

  This will make the code a bit easier to read
  even if it is overkill. Thre reader can compare
  the code with the spec.

5. About gxvar2.patch 

*** src/truetype/ttgxvar.c~     2004-04-03 21:48:05.000000000 -0800
--- src/truetype/ttgxvar.c      2004-04-03 21:55:36.000000000 -0800
***************
*** 527,532 ****
--- 527,533 ----
  
        if ( FT_ALLOC( face->blend, sizeof(GX_BlendRec)))
          goto FExit;
+       FT_MEM_ZERO( face->blend, sizeof(GX_BlendRec));         /* GWW: Is this 
needed? */
        face->blend->mmvar_len =
              sizeof(FT_MM_Var) +
              axis_cnt*sizeof(FT_Var_Axis) +


I think this line is not needed. FT_ALLOC fills the memory region with 0.

Anyway it is worth to install the code into HEAD of the CVS repository 
now because you can view Skia with ftmulti:-). 
Should the installation be delayed till release?

I can provide the patch especially about stream related code if the
code is in the CVS repository after installing. Werner will fix the
indentation of codes.

Masatake YAMATO