<html> <head> <base href="https://bugs.freedesktop.org/" /> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW --- - [TAGGEDPDF] Parse the Tagged-PDF document structure tree when present" href="https://bugs.freedesktop.org/show_bug.cgi?id=64815#c49">Comment # 49</a> on <a class="bz_bug_link bz_status_NEW " title="NEW --- - [TAGGEDPDF] Parse the Tagged-PDF document structure tree when present" href="https://bugs.freedesktop.org/show_bug.cgi?id=64815">bug 64815</a> from <a class="email" href="mailto:carlosgc@gnome.org" title="Carlos Garcia Campos <carlosgc@gnome.org>"> Carlos Garcia Campos</a> <pre>Comment on <a href="attachment.cgi?id=86677" name="attach_86677" title="[PATCH v8 04/15] Tagged-PDF: Parsing of StructElem standard types and">attachment 86677</a> <a href="attachment.cgi?id=86677&action=edit" title="[PATCH v8 04/15] Tagged-PDF: Parsing of StructElem standard types and">[details]</a> <a href='page.cgi?id=splinter.html&bug=64815&attachment=86677'>[review]</a> [PATCH v8 04/15] Tagged-PDF: Parsing of StructElem standard types and Review of <a href="attachment.cgi?id=86677" name="attach_86677" title="[PATCH v8 04/15] Tagged-PDF: Parsing of StructElem standard types and">attachment 86677</a> <a href="attachment.cgi?id=86677&action=edit" title="[PATCH v8 04/15] Tagged-PDF: Parsing of StructElem standard types and">[details]</a> <a href='page.cgi?id=splinter.html&bug=64815&attachment=86677'>[review]</a>: ----------------------------------------------------------------- ::: poppler/Makefile.am @@ +299,2 @@ > StructElement.cc \ > + StructTreeRoot.cc \ This change looks unrelated. ::: 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. @@ +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? @@ +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. @@ +288,5 @@ > + > +static const AttributeMapEntry attributeMapCommonShared[] = > +{ > + ATTR_D(Placement, gFalse, isPlacementName, Inline), > + ATTR_D(WritingMode, gFalse, isWritingModeName, LrTb), WritingMode is inheritable @@ +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? @@ +341,5 @@ > + ATTR_LIST_END > +}; > + > +static const AttributeMapEntry attributeMapCommonList[] = { > + ATTR_D(ListNumbering, gFalse, isListNumberingName, None), This is inheritable @@ +740,5 @@ > + formatted->Set(formattedA); > + else > + formatted = new GooString(formattedA); > + } else { > + delete formatted; You should set formatted to NULL here @@ +744,5 @@ > + delete formatted; > + } > +} > + > +GBool Attribute::typeCheck(StructElement *element) checkType sounds a bit better to me @@ +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. @@ +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. @@ +936,5 @@ > +{ > + if (isContent()) > + return parent->findAttribute(attributeType, inherit, attributeOwner); > + > + if (attributeType != Attribute::Unknown && attributeType != Attribute::UserProperty) { I would return early here. @@ +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() @@ +1133,5 @@ > + getAttribute(j)->setRevision(revision); > + } else { > + error(errSyntaxWarning, -1, "C item is wrong type ({0:s})", iobj.getTypeName()); > + } > + } attr.free() ::: poppler/StructElement.h @@ +27,2 @@ > class StructTreeRoot; > +class TextWordList; What do you need this for? @@ +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</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>