From 60abc31cdfcc6ffaa016fec7700077d8c472598c Mon Sep 17 00:00:00 2001
From: vng <viktor.govako@gmail.com>
Date: Tue, 29 Jan 2013 16:43:16 +0300
Subject: [PATCH] - Fix error processing logic when loading bookmarks (do not
 create BookmarkCategory when xml parse error). - Add test for special xml
 names.

---
 map/bookmark.cpp                 | 24 +++++++++-----
 map/bookmark.hpp                 |  2 +-
 map/map_tests/bookmarks_test.cpp | 57 ++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 9 deletions(-)

diff --git a/map/bookmark.cpp b/map/bookmark.cpp
index cc6d253e44..2d22202692 100644
--- a/map/bookmark.cpp
+++ b/map/bookmark.cpp
@@ -13,6 +13,7 @@
 
 #include "../std/fstream.hpp"
 #include "../std/algorithm.hpp"
+#include "../std/auto_ptr.hpp"
 
 
 void BookmarkCategory::AddBookmarkImpl(Bookmark const & bm, double scale)
@@ -268,29 +269,36 @@ namespace bookmark_impl
   };
 }
 
-void BookmarkCategory::LoadFromKML(ReaderPtr<Reader> const & reader)
+bool BookmarkCategory::LoadFromKML(ReaderPtr<Reader> const & reader)
 {
   ReaderSource<ReaderPtr<Reader> > src(reader);
   bookmark_impl::KMLParser parser(*this);
-  if (!ParseXML(src, parser, true))
+  if (ParseXML(src, parser, true))
+    return true;
+  else
+  {
     LOG(LERROR, ("XML read error. Probably, incorrect file encoding."));
+    return false;
+  }
 }
 
 BookmarkCategory * BookmarkCategory::CreateFromKMLFile(string const & file)
 {
-  BookmarkCategory * cat = new BookmarkCategory("");
+  auto_ptr<BookmarkCategory> cat(new BookmarkCategory(""));
   try
   {
-    cat->LoadFromKML(new FileReader(file));
-    cat->m_file = file;
+    if (cat->LoadFromKML(new FileReader(file)))
+      cat->m_file = file;
+    else
+      cat.reset();
   }
   catch (std::exception const & e)
   {
     LOG(LWARNING, ("Error while loading bookmarks from", file, e.what()));
-    delete cat;
-    cat = 0;
+    cat.reset();
   }
-  return cat;
+
+  return cat.release();
 }
 
 namespace
diff --git a/map/bookmark.hpp b/map/bookmark.hpp
index 579ec80796..3cfa470ae9 100644
--- a/map/bookmark.hpp
+++ b/map/bookmark.hpp
@@ -92,7 +92,7 @@ public:
   /// @name Theese fuctions are public for unit tests only.
   /// You don't need to call them from client code.
   //@{
-  void LoadFromKML(ReaderPtr<Reader> const & reader);
+  bool LoadFromKML(ReaderPtr<Reader> const & reader);
   void SaveToKML(ostream & s);
 
   /// Uses the same file name from which was loaded, or
diff --git a/map/map_tests/bookmarks_test.cpp b/map/map_tests/bookmarks_test.cpp
index fa997ce23f..52e2255a27 100644
--- a/map/map_tests/bookmarks_test.cpp
+++ b/map/map_tests/bookmarks_test.cpp
@@ -464,3 +464,60 @@ UNIT_TEST(BookmarkCategory_EmptyName)
   char const * arrFiles[] = { "Bookmarks", "xxx" };
   DeleteCategoryFiles(arrFiles);
 }
+
+namespace
+{
+// Do check for "bad" names without CDATA markers.
+char const * kmlString3 =
+    "<?xml version=\"1.0\" encoding=\"UTF-8\"?>"
+    "<kml xmlns=\"http://earth.google.com/kml/2.1\">"
+    "<Document>"
+     "<name>3663 and M <![CDATA[&]]> J Seafood Branches</name>"
+     "<visibility>1</visibility>"
+      "<Placemark>"
+       "<name>![X1]{X2}(X3)</name>"
+       "<Point>"
+        "<coordinates>50, 50</coordinates>"
+       "</Point>"
+      "</Placemark>"
+   "</Document>"
+   "</kml>";
+
+  bool EqualBookmarks(Bookmark const & b1, Bookmark const & b2)
+  {
+    if (b1.GetName() != b2.GetName())
+      return false;
+    if (b1.GetDescription() != b2.GetDescription())
+      return false;
+    if (b1.GetType() != b2.GetType())
+      return false;
+    if (!m2::AlmostEqual(b1.GetOrg(), b2.GetOrg()))
+      return false;
+    if (!my::AlmostEqual(b1.GetScale(), b2.GetScale()))
+      return false;
+
+    // do not check timestamp
+    return true;
+  }
+}
+
+UNIT_TEST(Bookmarks_SpecialXMLNames)
+{
+  BookmarkCategory cat1("");
+  TEST(cat1.LoadFromKML(new MemReader(kmlString3, strlen(kmlString3))), ());
+
+  TEST_EQUAL(cat1.GetBookmarksCount(), 1, ());
+  TEST(cat1.SaveToKMLFile(), ());
+
+  scoped_ptr<BookmarkCategory> cat2(BookmarkCategory::CreateFromKMLFile(cat1.GetFileName()));
+  TEST(cat2.get(), ());
+  TEST_EQUAL(cat2->GetBookmarksCount(), 1, ());
+
+  TEST_EQUAL(cat1.GetName(), "3663 and M & J Seafood Branches", ());
+  TEST_EQUAL(cat1.GetName(), cat2->GetName(), ());
+  TEST_EQUAL(cat1.GetFileName(), cat2->GetFileName(), ());
+  TEST(EqualBookmarks(*cat1.GetBookmark(0), *cat2->GetBookmark(0)), ());
+  TEST_EQUAL(cat1.GetBookmark(0)->GetName(), "![X1]{X2}(X3)", ());
+
+  TEST(my::DeleteFileX(cat1.GetFileName()), ());
+}