Bug 320259 r=bryner sqlite compatability updates, fix bookmark InsertItem

browser/components/places/public/nsINavBookmarksService.idl

@@ -191,7 +191,8 @@ interface nsINavBookmarksService : nsISupports
   readonly attribute PRInt64 toolbarRoot;
 
   /**
-   * Inserts a child item into the given folder.
+   * Inserts a child item into the given folder. If this item already exists in
+   * the given folder, it will be moved to the new position.
    *  @param folder     The id of the parent folder
    *  @param item       The URI to insert
    *  @param index      The index to insert at, or -1 to append

browser/components/places/src/nsBookmarksHTML.cpp

@@ -56,15 +56,33 @@
  *   HREF is the destination of the bookmark
  *   LAST_CHARSET should be stored as an annotation (FIXME TODO) so that the
  *     next time we go to that page we remember the user's preference.
- *   ICON should be stored in the annotation service (FIXME TODO)
+ *   ICON will be stored in the favicon service
  *   Text of the <a> container is the name of the bookmark
  *   Ignored: ADD_DATE, LAST_VISIT, LAST_MODIFIED, ID
  * Bookmark comment := dd
  *   This affects the previosly added bookmark
  * Separator := hr
- *   Insert a separator into the current container
+ *   Insert a separator into the current container (FIXME TODO)
  * The folder hierarchy is defined by <dl>/<ul>/<menu> (the old importing code
  *     handles all these cases, when we write, use <dl>).
+ *
+ * Overall design
+ * --------------
+ *
+ * We need to emulate a recursive parser. A "Bookmark import frame" is created
+ * corresponding to each folder we encounter. These are arranged in a stack,
+ * and contain all the state we need to keep track of.
+ *
+ * A frame is created when we find a heading, which defines a new container.
+ * The frame also keeps track of the nesting of <DL>s, (in well-formed
+ * bookmarks files, these will have a 1-1 correspondence with frames, but we
+ * try to be a little more flexible here). When the nesting count decreases
+ * to 0, then we know a frame is complete and to pop back to the previous
+ * frame.
+ *
+ * Note that a lot of things happen when tags are CLOSED because we need to
+ * get the text from the content of the tag. For example, link and heading tags
+ * both require the content (= title) before actually creating it.
  */
 
 #include "nsBrowserCompsCID.h"

browser/components/places/src/nsNavBookmarks.cpp

@@ -166,14 +166,13 @@ nsNavBookmarks::Init()
   // item_child, and folder_child from moz_bookmarks.  This selects only
   // _item_ children which are in moz_history.
   NS_NAMED_LITERAL_CSTRING(selectItemChildren,
-    "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, MAX(fullv.visit_date), f.url, a.position, a.item_child, a.folder_child, null "
+    "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
+      "(SELECT MAX(visit_date) FROM moz_historyvisit WHERE page_id = h.id), "
+      "f.url, a.position, a.item_child, a.folder_child, null "
     "FROM moz_bookmarks a "
     "JOIN moz_history h ON a.item_child = h.id "
     "LEFT OUTER JOIN moz_favicon f ON h.favicon = f.id "
-    "LEFT JOIN moz_historyvisit v ON h.id = v.page_id "
-    "LEFT JOIN moz_historyvisit fullv ON h.id = fullv.page_id "
-    "WHERE a.parent = ?1 AND a.position >= ?2 AND a.position <= ?3 "
-    "GROUP BY h.id");
+    "WHERE a.parent = ?1 AND a.position >= ?2 AND a.position <= ?3 ");
 
   // Construct a result where the first columns are padded out to the width
   // of mDBGetVisitPageInfo, containing additional columns for position,
@@ -504,8 +503,36 @@ nsNavBookmarks::InsertItem(PRInt64 aFolder, nsIURI *aItem, PRInt32 aIndex)
   nsresult rv = History()->GetUrlIdFor(aItem, &childID, PR_TRUE);
   NS_ENSURE_SUCCESS(rv, rv);
 
+  // see if this item is already in the folder
+  PRBool hasItem = PR_FALSE;
+  PRInt32 previousIndex = -1;
+  { // scope the mDBIndexOfItem statement
+    mozStorageStatementScoper scoper(mDBIndexOfItem);
+    rv = mDBIndexOfItem->BindInt64Parameter(0, childID);
+    NS_ENSURE_SUCCESS(rv, rv);
+    rv = mDBIndexOfItem->BindInt64Parameter(1, aFolder);
+    NS_ENSURE_SUCCESS(rv, rv);
+    rv = mDBIndexOfItem->ExecuteStep(&hasItem);
+    NS_ENSURE_SUCCESS(rv, rv);
+    if (hasItem)
+      previousIndex = mDBIndexOfItem->AsInt32(0);
+  }
+
   PRInt32 index = (aIndex == -1) ? FolderCount(aFolder) : aIndex;
 
+  if (hasItem && index == previousIndex)
+    return NS_OK; // item already at its desired position: nothing to do
+
+  if (hasItem) {
+    // remove any old items
+    rv = RemoveItem(aFolder, aItem);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    // since we just removed the item, everything after it shifts back by one
+    if (aIndex > previousIndex)
+      aIndex --;
+  }
+
   rv = AdjustIndices(aFolder, index, PR_INT32_MAX, 1);
   NS_ENSURE_SUCCESS(rv, rv);
 

browser/components/places/src/nsNavHistory.cpp

@@ -408,23 +408,23 @@ nsNavHistory::InitDB()
 
   // mDBGetURLPageInfoFull
   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
-      "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, MAX(fullv.visit_date), f.url "
+      "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
+        "(SELECT MAX(visit_date) FROM moz_historyvisit WHERE page_id = h.id), "
+        "f.url "
       "FROM moz_history h "
-      "LEFT JOIN moz_historyvisit v ON h.id = v.page_id "
-      "LEFT JOIN moz_historyvisit fullv ON h.id = fullv.page_id "
       "LEFT OUTER JOIN moz_favicon f ON h.favicon = f.id "
-      "WHERE h.url = ?1 "
-      "GROUP BY h.id"),
+      "WHERE h.url = ?1 "),
     getter_AddRefs(mDBGetURLPageInfoFull));
   NS_ENSURE_SUCCESS(rv, rv);
 
   // mDBGetIdPageInfoFull
   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
-      "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, MAX(fullv.visit_date), f.url "
-      "FROM moz_history h LEFT JOIN moz_historyvisit v ON h.id = v.page_id "
-      "LEFT JOIN moz_historyvisit fullv ON h.id = fullv.page_id "
+      "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
+        "(SELECT MAX(visit_date) FROM moz_historyvisit WHERE page_id = h.id), "
+        "f.url "
+      "FROM moz_history h "
       "LEFT OUTER JOIN moz_favicon f ON h.favicon = f.id "
-      "WHERE h.id = ?1 GROUP BY h.id"),
+      "WHERE h.id = ?1"),
     getter_AddRefs(mDBGetIdPageInfoFull));
   NS_ENSURE_SUCCESS(rv, rv);
 
@@ -1255,16 +1255,15 @@ nsNavHistory::ExecuteQueries(nsINavHistoryQuery** aQueries, PRUint32 aQueryCount
       "LEFT OUTER JOIN moz_favicon f ON h.favicon = f.id "
       "WHERE ");
   } else {
-    // For URLs, it is more complicated. Because we want each URL once. The
-    // GROUP BY clause gives us this. However, we also want the most recent
-    // visit time, so we add ANOTHER join with the visit table (this one does
-    // not have any restrictions on it from the query) and do MAX() to get the
-    // max visit time.
+    // For URLs, it is more complicated, because we want each URL once. The
+    // GROUP BY clause gives us this. To get the max visit time, we populate
+    // one column by using a nested SELECT on the visit table.
     queryString = NS_LITERAL_CSTRING(
-      "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, MAX(fullv.visit_date), f.url "
+      "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
+        "(SELECT MAX(visit_date) FROM moz_historyvisit WHERE page_id = h.id), "
+        "f.url "
       "FROM moz_history h "
       "JOIN moz_historyvisit v ON h.id = v.page_id "
-      "JOIN moz_historyvisit fullv ON h.id = fullv.page_id "
       "LEFT OUTER JOIN moz_favicon f ON h.favicon = f.id "
       "WHERE ");
     groupBy = NS_LITERAL_CSTRING(" GROUP BY h.id");