525299 - Make Library left pane creation more robust

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

Zpao sent me a db with this problem, while we should investigate if we can find what caused that, we should also make the left pane building code more robust to these kind of corruptions.

while touching that code would be nice to fix bug 523361 and bug 498162

Comment 1

[Marco Bonardo [:mak]]

And Bug 489681, uargh :(

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.0

this needs some test.

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v1.1

first impl of a test, still missing test cases but good first infrastructure.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.2

The test is quite complete, covers a large variety of possibile corruptions, and i learned a couple useful tricks for future tests.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 409503 [details] [diff] [review]
patch v1.2

>diff --git a/browser/components/pl
>+    // This is the list of the left pane queries.
>+    let queries = {
>+      "PlacesRoot": { title: "" },
>+      "History": { title: this.getString("OrganizerQueryHistory") },
>+      "Tags": { title: this.getString("OrganizerQueryTags") },
>+      "AllBookmarks": { title: this.getString("OrganizerQueryAllBookmarks") },
>+      //"Downloads": { title: this.getString("OrganizerQueryDownloads") },

just leave it out. we can add back in when we actually implement that feature.

>+      "BookmarksToolbar":
>+        { title: null,
>+          concreteTitle: PlacesUtils.getString("BookmarksToolbarFolderTitle"),
>+          concreteId: PlacesUtils.toolbarFolderId },
>+      "BookmarksMenu":
>+        { title: null,
>+          concreteTitle: PlacesUtils.getString("BookmarksMenuFolderTitle"),
>+          concreteId: PlacesUtils.bookmarksMenuFolderId },
>+      "UnfiledBookmarks":
>+        { title: null,
>+          concreteTitle: PlacesUtils.getString("UnsortedBookmarksFolderTitle"),
>+          concreteId: PlacesUtils.unfiledBookmarksFolderId },
>+    };
>+    // All queries but PlacesRoot.
>+    const EXPECTED_QUERIES = 6;

EXPECTED_QUERY_COUNT ?

>+
>+    // Removes an item and associated annotations, ignoring eventual errors.
>+    function safeRemoveItem(aItemId) {
>+      try {
>+        if (as.itemHasAnnotation(aItemId, ORGANIZER_QUERY_ANNO) &&
>+            !(as.getItemAnnotation(aItemId, ORGANIZER_QUERY_ANNO) in queries)) {
>+          // Some extension did annotate their roots with our query annotation,
>+          // we should avoid to delete them for dataloss issues.

s/did annotate/annotated/

and the whole next line is confusing, maybe "so we should not delete them."?

>+          return;
>+        }
>+        // removeItemAnnotation does not check if item exists, nor the anno,
>+        // so this is safe to do.
>+        as.removeItemAnnotation(aItemId, ORGANIZER_FOLDER_ANNO);
>+        as.removeItemAnnotation(aItemId, ORGANIZER_QUERY_ANNO);
>+        // This will throw if the annotation is orphan.
>+        bs.removeItem(aItemId);

"is an orphan"

to here....

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 409503 [details] [diff] [review]
patch v1.2

                                                .getItemTitle(itemId) });
>+    var query = { name: queryName,
>+                  itemId: itemId,
>+                  correctTitle: PlacesUtils.bookmarks.getItemTitle(itemId) }
>+    switch (queryName) {
>+      case "BookmarksToolbar":
>+        query.concreteId = PlacesUtils.toolbarFolderId;
>+        query.concreteTitle = PlacesUtils.bookmarks.getItemTitle(query.concreteId)
>+        break;
>+      case "BookmarksMenu":
>+        query.concreteId = PlacesUtils.bookmarksMenuFolderId;
>+        query.concreteTitle = PlacesUtils.bookmarks.getItemTitle(query.concreteId)
>+        break;
>+      case "UnfiledBookmarks":
>+        query.concreteId = PlacesUtils.unfiledBookmarksFolderId;
>+        query.concreteTitle = PlacesUtils.bookmarks.getItemTitle(query.concreteId)
>+        break;
>+    }
>+    leftPaneQueries.push(query);
>     // Rename to a bad title.

missing end statement semicolons on a bunch of lines above.

r=me. please cc l10n for the string changes if necessary.

Comment 7

[Marco Bonardo [:mak]]

if we want this on 1.9.2 i can make a version without string changes. would make sense.

Comment 8

[Marco Bonardo [:mak]]

Attachment: patch v1.3

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/5dce7c1445d5