[Poppler-bugs] [Bug 64815] [TAGGEDPDF] Parse the Tagged-PDF document structure tree when present

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=64815

--- Comment #51 from Adrian Perez de Castro <aperez at igalia.com> ---
(In reply to comment #49)
> Comment on attachment 86677 [details] [review]
> [PATCH v8 04/15] Tagged-PDF: Parsing of StructElem standard types and
> 
> Review of attachment 86677 [details] [review]:
> -----------------------------------------------------------------
> 
> ::: poppler/Makefile.am
> @@ +299,2 @@
> >  	StructElement.cc	\
> > +	StructTreeRoot.cc	\
> 
> This change looks unrelated.

Yep.

> ::: poppler/StructElement.cc
> @@ +260,5 @@
> > +  Object Auto;
> > +  Object Start;
> > +  Object None;
> > +  Object Before;
> > +  Object Nat1;
> 
> It's weird that struct members start with a capital letter, but I guess it
> looks better in the  macros below.

Yes, they do look easier in the eyes when used in the macros below
(at least for me). Should I change them?

> @@ +282,5 @@
> > +static const AttributeDefaults attributeDefaults;
> > +
> > +
> > +#define ATTR_LIST_END      { Attribute::Unknown, NULL, NULL, gFalse, NULL }
> > +#define ATTR_D(x, i, c, v) { Attribute::x, #x, &attributeDefaults.v, i, c }
> 
> What D stand for? Default?

Yes, it stands for “default”. ATTR_D defines an attribute with a 
default value.

> @@ +283,5 @@
> > +
> > +
> > +#define ATTR_LIST_END      { Attribute::Unknown, NULL, NULL, gFalse, NULL }
> > +#define ATTR_D(x, i, c, v) { Attribute::x, #x, &attributeDefaults.v, i, c }
> > +#define ATTR_N(x, i, c)    { Attribute::x, #x, NULL, i, c }
> 
> And N? Normal? NotDefault? What about using ATTR and ATTR_WITH_DEFAULT? Also
> I would use words instead of abbreviations to improve the readability type,
> inheritable, check, defaultValue.

“N” is for “no-default“. Sure I will change this to use descriptive names.

> @@ +288,5 @@
> > +
> > +static const AttributeMapEntry attributeMapCommonShared[] =
> > +{
> > +  ATTR_D(Placement,       gFalse, isPlacementName, Inline),
> > +  ATTR_D(WritingMode,     gFalse, isWritingModeName, LrTb),
> 
> WritingMode is inheritable

Good catch. Fixing.

> @@ +295,5 @@
> > +  ATTR_D(BorderStyle,     gFalse, isBorderStyle, None),
> > +  ATTR_N(BorderThickness, gTrue,  isPositiveOrOptionalArray4),
> > +  ATTR_D(Padding,         gFalse, isPositiveOrArray4, Zero),
> > +  ATTR_N(Color,           gTrue,  isRGBColor),
> > +  ATTR_LIST_END
> 
> Do we really need this being the array statically allocated?

With “this” I suppose you mean the ATTR_LIST_END entry. Removing
the end marker would make it necessary to store the length of the
array in the entries of the “attributeMapAll” (and related) arrays.
The amount or static data is just a few bytes more having the end
markers in the arrays, and makes the code simpler. If there is any
good reason to remove the end markers, it could be done, but I
would rather leave them there.

> @@ +341,5 @@
> > +  ATTR_LIST_END
> > +};
> > +
> > +static const AttributeMapEntry attributeMapCommonList[] = {
> > +  ATTR_D(ListNumbering, gFalse, isListNumberingName, None),
> 
> This is inheritable

You're right. Fixing.

> @@ +740,5 @@
> > +      formatted->Set(formattedA);
> > +    else
> > +      formatted = new GooString(formattedA);
> > +  } else {
> > +    delete formatted;
> 
> You should set formatted to NULL here

Good catch, thanks.

> @@ +744,5 @@
> > +    delete formatted;
> > +  }
> > +}
> > +
> > +GBool Attribute::typeCheck(StructElement *element)
> 
> checkType sounds a bit better to me

Sure, I am changing it to checkType().

> @@ +747,5 @@
> > +
> > +GBool Attribute::typeCheck(StructElement *element)
> > +{
> > +  // If an element is passed, tighther type-checking can be done.
> > +  if (element) {
> 
> You could use an early return here, to save an indentation level.

Sure, it will be fixed.

> @@ +765,5 @@
> > +
> > +  return gTrue;
> > +}
> > +
> > +Attribute::Type Attribute::typeForName(const char *name, StructElement *element)
> 
> You are using get for all other methods, use getTypeForName here for
> consistency.

Sure.

> @@ +936,5 @@
> > +{
> > +  if (isContent())
> > +    return parent->findAttribute(attributeType, inherit, attributeOwner);
> > +
> > +  if (attributeType != Attribute::Unknown && attributeType != Attribute::UserProperty) {
> 
> I would return early here.

Okay. I am changing it.

> @@ +1132,5 @@
> > +            for (unsigned j = attrIndex; j < getNumAttributes(); j++)
> > +              getAttribute(j)->setRevision(revision);
> > +          } else {
> > +            error(errSyntaxWarning, -1, "C item is wrong type ({0:s})", iobj.getTypeName());
> > +          }
> 
> iobj.free()

Fixed.

> @@ +1133,5 @@
> > +              getAttribute(j)->setRevision(revision);
> > +          } else {
> > +            error(errSyntaxWarning, -1, "C item is wrong type ({0:s})", iobj.getTypeName());
> > +          }
> > +        }
> 
> attr.free()

Fixed as well.

> ::: poppler/StructElement.h
> @@ +27,2 @@
> >  class StructTreeRoot;
> > +class TextWordList;
> 
> What do you need this for?

It had sense at some point when the feature was WIP, but it ended
up not being needed. I will remove it.

> @@ +116,5 @@
> > +  static Type typeForName(const char *name, StructElement *element = NULL);
> > +  static Attribute *parseUserProperty(Dict *property);
> > +
> > +  friend class StructElement;
> > +};
> 
> The Attribute class implementation is huge, you might consider moving it to
> its own file

Sure thing.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/poppler-bugs/attachments/20131015/72e46196/attachment.html>