Add functions to collect bookmarks from Pdfium

pdf/pdfium/pdfium_engine.cc

Index: pdf/pdfium/pdfium_engine.cc
diff --git a/pdf/pdfium/pdfium_engine.cc b/pdf/pdfium/pdfium_engine.cc
index 213214f5311a35a1064722d5b26b97cb7d3cb06c..44744dc78e8a065fbabe12d363011c9ce6fc21cd 100644
--- a/pdf/pdfium/pdfium_engine.cc
+++ b/pdf/pdfium/pdfium_engine.cc
@@ -30,6 +30,7 @@
 #include "ppapi/cpp/trusted/browser_font_trusted.h"
 #include "ppapi/cpp/url_response_info.h"
 #include "ppapi/cpp/var.h"
+#include "ppapi/cpp/var_dictionary.h"
 #include "third_party/pdfium/fpdfsdk/include/fpdf_ext.h"
 #include "third_party/pdfium/fpdfsdk/include/fpdf_flatten.h"
 #include "third_party/pdfium/fpdfsdk/include/fpdf_searchex.h"
@@ -2297,6 +2298,55 @@ int PDFiumEngine::GetNumberOfPages() {
   return pages_.size();
 }
 
+int NumChildren(FPDF_DOCUMENT doc, FPDF_BOOKMARK bookmark)
+{
+  int num = 0;
+  FPDF_BOOKMARK child = FPDFBookmark_GetFirstChild(doc, bookmark);
+  while (child != NULL) {
+      child = FPDFBookmark_GetNextSibling(doc, child);
+      num++;
+  }
+  return num;
+}
+
+void TraverseBookmarks(FPDF_BOOKMARK bookmark, pp::VarDictionary *dict,

[Sam McNally, 2014/12/16 05:58:34]

Put this in the anonymous namespace above.

Return a pp::VarDictionary instead of taking one to fill. Also, to be
consistent, you should take |doc| before |bookmark|.

[Alexandre Carlton, 2014/12/16 22:14:51]

Done.

+                       FPDF_DOCUMENT doc) {
+  base::string16 title;
+  size_t buffer_size = FPDFBookmark_GetTitle(bookmark, NULL, 0);
+  if (buffer_size > 1) {
+    void *data = WriteInto(&title, buffer_size + 1);
+    FPDFBookmark_GetTitle(bookmark, data, buffer_size);

[Sam McNally, 2014/12/16 05:58:34]

I would pass the result of WriteInto(...) directly into FPDFBookmark_GetTitle.

[Alexandre Carlton, 2014/12/16 22:14:51]

Done.

+  }
+  std::string shortTitle = base::UTF16ToUTF8(title);
+  dict->Set(pp::Var("title"), pp::Var(shortTitle));

[Sam McNally, 2014/12/16 05:58:34]

Remove shortTitle and pass base::UTF16ToUTF8(title).

[Alexandre Carlton, 2014/12/16 22:14:51]

Done.

+
+  FPDF_DEST dest = FPDFBookmark_GetDest(doc, bookmark);
+  int pageIndex = dest ? FPDFDest_GetPageIndex(doc, dest) : -1;

[Sam McNally, 2014/12/16 05:58:34]

How about only setting "page" for bookmarks that have a destination page?

[Alexandre Carlton, 2014/12/16 22:14:51]

Done.

+  dict->Set(pp::Var("page"), pp::Var(pageIndex));
+
+  int length = NumChildren(doc, bookmark);
+  pp::VarArray *children = new pp::VarArray();

[Sam McNally, 2014/12/16 05:58:35]

Put this on the stack. Otherwise, it will leak.

[Alexandre Carlton, 2014/12/16 22:14:51]

Done.

+  children->SetLength(length);

[Sam McNally, 2014/12/16 05:58:34]

You don't need this. VarArray resizes to fit on calls to Set().

[Alexandre Carlton, 2014/12/16 22:14:51]

Done.

+
+  int i = 0;
+  pp::VarDictionary *childDict;

[Sam McNally, 2014/12/16 05:58:34]

Declare this inside the loop (or remove it entirely once this returns the
dictionary instead of taking one). See above about leaking too.

[Alexandre Carlton, 2014/12/16 22:14:51]

Done.

+  bookmark = FPDFBookmark_GetFirstChild(doc, bookmark);

[Sam McNally, 2014/12/16 05:58:35]

Use a different variable for the child bookmarks.

[Alexandre Carlton, 2014/12/16 22:14:51]

Done.

+  while (bookmark != NULL) {

[Sam McNally, 2014/12/16 05:58:34]

I think this would be nicer as a for loop.

[Alexandre Carlton, 2014/12/16 22:14:51]

Done.

+    childDict = new pp::VarDictionary();
+    TraverseBookmarks(bookmark, childDict, doc);
+    children->Set(i, *childDict);
+    bookmark = FPDFBookmark_GetNextSibling(doc, bookmark);
+    i++;
+  }
+  dict->Set(pp::Var("children"), *children);
+}
+
+pp::VarDictionary PDFiumEngine::GetBookmarks() {
+  pp::VarDictionary *dict = new pp::VarDictionary();
+  TraverseBookmarks(NULL, dict, doc_);
+  return *dict;
+}
+
 int PDFiumEngine::GetNamedDestinationPage(const std::string& destination) {
   // Look for the destination.
   FPDF_DEST dest = FPDF_GetNamedDestByName(doc_, destination.c_str());