<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#c51">Comment # 51</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:aperez@igalia.com" title="Adrian Perez de Castro <aperez@igalia.com>"> Adrian Perez de Castro</a> <pre>(In reply to <a href="show_bug.cgi?id=64815#c49">comment #49</a>) > 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> [review] > [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> [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.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are on the CC list for the bug.</li> </ul> </body> </html>