<html> <head> <base href="https://bugs.freedesktop.org/"> </head> <body> <div> <a class="bz_bug_link bz_status_NEW " title="NEW - support tags, links, page labels, outline, metadata in cairo backend" href="https://bugs.freedesktop.org/show_bug.cgi?id=103912#c10">Comment # 10</a> on <a class="bz_bug_link bz_status_NEW " title="NEW - support tags, links, page labels, outline, metadata in cairo backend" href="https://bugs.freedesktop.org/show_bug.cgi?id=103912">bug 103912</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=135718" name="attach_135718" title="write document logical structure">attachment 135718</a> <a href="attachment.cgi?id=135718&action=edit" title="write document logical structure">[details]</a> <a href='page.cgi?id=splinter.html&bug=103912&attachment=135718'>[review]</a> write document logical structure Review of <a href="attachment.cgi?id=135718" name="attach_135718" title="write document logical structure">attachment 135718</a> <a href="attachment.cgi?id=135718&action=edit" title="write document logical structure">[details]</a> <a href='page.cgi?id=splinter.html&bug=103912&attachment=135718'>[review]</a>: ----------------------------------------------------------------- LGTM, but I have a few comments ::: poppler/CairoOutputDev.cc @@ +170,5 @@ > actualText = NULL; > + logicalStruct = false; > + pdfPageNum = 0; > + cairoPageNum = 0; > + forwardLinkCount = 0; Initialize firstPage to gFalse here too. @@ +179,4 @@ > align_stroke_coords = gFalse; > adjusted_stroke_width = gFalse; > xref = NULL; > + annotations = nullptr; linkAnnot should be initialized to nullptr here too I guess. @@ +201,5 @@ > text->decRefCnt(); > if (actualText) > + delete actualText; > + if (annotations) > + delete annotations; Now that we have C++11 support I think it's a good idea to use unique_ptr in newly added code at least. This could be a unique_ptr<Annots> and we don't need this explicit delete. @@ +227,4 @@ > } > } > > +GBool CairoOutputDev::isPDF() this could be const @@ +322,5 @@ > + if (logicalStruct && isPDF()) { > + int numDests = doc->getCatalog()->numDestNameTree(); > + for (int i = 0; i < numDests; i++) { > + GooString *name = doc->getCatalog()->getDestNameTreeName(i); > + LinkDest *dest = doc->getCatalog()->getDestNameTreeDest(i); We could use unique_ptr here too... @@ +325,5 @@ > + GooString *name = doc->getCatalog()->getDestNameTreeName(i); > + LinkDest *dest = doc->getCatalog()->getDestNameTreeDest(i); > + if (dest->isPageRef()) { > + destsMap[dest->getPageRef()].insert( > + std::make_pair(std::unique_ptr<GooString>(name->copy()), std::unique_ptr<LinkDest>(dest))); then we just std::move() the dest here @@ +327,5 @@ > + if (dest->isPageRef()) { > + destsMap[dest->getPageRef()].insert( > + std::make_pair(std::unique_ptr<GooString>(name->copy()), std::unique_ptr<LinkDest>(dest))); > + } else { > + delete dest; And we don't need this. @@ +336,5 @@ > + for (int i = 0; i < numDests; i++) { > + const char *name = doc->getCatalog()->getDestsName(i); > + LinkDest *dest = doc->getCatalog()->getDestsDest(i); > + if (dest->isPageRef()) { > + destsMap[dest->getPageRef()].insert( Note that we would need to get the ref here first if we use std::move(), using a local var. @@ +339,5 @@ > + if (dest->isPageRef()) { > + destsMap[dest->getPageRef()].insert( > + std::make_pair(std::unique_ptr<GooString>(new GooString(name)), > + std::unique_ptr<LinkDest>(dest))); > + } And same here, so we don't leak the dest in case it's not page ref. @@ +366,5 @@ > + cairoPageNum++; > + > + if (logicalStruct && isPDF()) { > + if (annotations) > + delete annotations; We wouldn't need this @@ +368,5 @@ > + if (logicalStruct && isPDF()) { > + if (annotations) > + delete annotations; > + Object obj = doc->getPage(pageNum)->getAnnotsObject(xref); > + annotations = new Annots(doc, pageNum, &obj); and here we would just std::make_unique<Annots>. I don't know if I pushed the patch that adds make_unique impl in the end, though. @@ +3474,5 @@ > + } else { > + // Link page ref is to a page that has not been emitted. > + // Create a named destination and add the name to destsMap so destination will > + // be create if/when the page is emitted. > + GooString *name = new GooString(); here I would use make_unique and then move below, but it's ok this way too. ::: poppler/CairoOutputDev.h @@ +263,5 @@ > void type3D1(GfxState *state, double wx, double wy, > double llx, double lly, double urx, double ury) override; > > + virtual void beginMarkedContent(GfxState *state, char *name, Dict *properties) override; > + virtual void endMarkedContent(GfxState *state) override; virtual is not needed here, override is enough ::: poppler/OutputDev.h @@ +148,4 @@ > virtual void startPage(int pageNum, GfxState *state, XRef *xref) {} > > // End a page. > + virtual void endPage(GfxState *state) {} Could we make state default to nullptr? This way we avoid having to modify all the callers to pass nullptr when unused. ::: poppler/UTF.h @@ +84,5 @@ > + > +// Convert a PDF Text String to UTF-8 > +// textStr - PDF text string > +// returns UTF-8 string. > +char *TextStringToUtf8(GooString *textStr); Changes to UTF could probably be split to a different patch.</pre> </div> <hr> You are receiving this mail because: <ul> <li>You are the assignee for the bug.</li> </ul> </body> </html>