Wrong icons on bookmarks sidebar

RESOLVED FIXED

Status

()

P2
normal
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: ShareBird, Assigned: mano)

Tracking

({regression})

Trunk
regression
Points:
---
Bug Flags:
blocking-firefox3.6 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

9 years ago
The bookmarks sidebar exhibit the folder-item.png for "Bookmarks Toolbar", "Bookmarks Menu" and "Unsorte3d Bookmarks" ignoring the specified CSS styles for them.

This is probably a regression from  Bug 493636 - when possible, use cached left pane query ids instead of the anno service directly
(Reporter)

Updated

9 years ago
Flags: blocking-firefox3.6?

Comment 1

9 years ago
Confirming that this is a regression from bug 493636.
Blocks: 493636
Assignee: nobody → dietrich
Flags: blocking-firefox3.6? → blocking-firefox3.6+
Marco: although I assigned this to Dietrich as the original regressor, I imagine he wouldn't mind if you took it. :)
since PlacesUIUtils is per-window, the left pane cache needs to be initialized in the sidebar's window scope. patch coming.
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [has patch][needs review mak]

Updated

9 years ago
Attachment #402037 - Flags: review?(mak77) → review-
Comment on attachment 402037 [details] [diff] [review]
fix

>diff --git a/browser/components/places/content/bookmarksPanel.js b/browser/components/places/content/bookmarksPanel.js
>--- a/browser/components/places/content/bookmarksPanel.js
>+++ b/browser/components/places/content/bookmarksPanel.js
>@@ -32,17 +32,17 @@
> # and other provisions required by the GPL or the LGPL. If you do not delete
> # the provisions above, a recipient may use your version of this file under
> # the terms of any one of the MPL, the GPL or the LGPL.
> #
> # ***** END LICENSE BLOCK *****
> 
> function init() {
>   document.getElementById("bookmarks-view").place =
>-    "place:queryType=1&folder=" + window.top.PlacesUIUtils.allBookmarksFolderId;
>+    "place:queryType=1&folder=" + PlacesUIUtils.allBookmarksFolderId;

the only difference that this will make is that it will force initing the sidebar PUIU leftpane earlier, but if it's not initialized getLeftPaneQueryNameFromId should use the anno.

So, looking at the getLeftPaneQueryNameFromId util i found the problem:

+    if (this.__lookupGetter__("leftPaneFolderId")) {
+      try {
+        queryName = PlacesUtils.annotations.
+                                getItemAnnotation(itemId, ORGANIZER_QUERY_ANNO);

itemId is wrong, it should be aItemId

since this won't return anything we don't set the atom
Created attachment 402179 [details] [diff] [review]
actual fix

needs a test still
Attachment #402037 - Attachment is obsolete: true
Priority: -- → P2
Assignee: dietrich → mano

Updated

9 years ago
Flags: in-testsuite?
Whiteboard: [has patch][needs review mak] → [has patch][needs new patch with tests]

Updated

9 years ago
Whiteboard: [has patch][needs new patch with tests] → [needs new patch with tests]
Created attachment 404792 [details] [diff] [review]
patch with tests
Attachment #402179 - Attachment is obsolete: true
Attachment #404792 - Flags: review?(dietrich)
Whiteboard: [needs new patch with tests] → [needs review dietrich]

Updated

9 years ago
Attachment #404792 - Flags: review?(dietrich) → review+
Comment on attachment 404792 [details] [diff] [review]
patch with tests

initially i thought just caling the util and check return value was enough since we should try to avoid chrome and mochitests tests in favor of unit tests, but i see the additional value added by this test, that does no rely on a specific implementation. So it's fine!


>diff -r 0e8fae2dd64c browser/components/places/tests/chrome/test_bug510634.xul
>--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
>+   -
>+   - The Initial Developer of the Original Code is Mozilla Corp.

nit: Some time ago we asked about this and Gerv (iirc) told us we should use Mozilla Foundation. here

>+
>+    // converts nsISupportsArray of atoms to a simple JS-strings array
>+    function convertPropertiesToJSArray(aSupportsArray) {
>+      var results = [];
>+      var count = aSupportsArray.Count();
>+      for (var i = 0; i < count; i++)
>+        results.push(aSupportsArray.GetElementAt(i)
>+                                   .QueryInterface(Ci.nsIAtom).toString());

nit: aSupportsArray.QueryElementAt(i, Ci.nsIAtom).toString()


>+      // 0 - History
>+      let historyProperties = createSupportsArray();
>+      tree.view.getCellProperties(0, titleColumn, historyProperties);
>+      historyProperties = convertPropertiesToJSArray(historyProperties);
>+      ok(historyProperties.indexOf("OrganizerQuery_History") != -1,
>+         "OrganizerQuery_History is set");
>+
>+      // 1 - Tags
>+      let tagsProperties = createSupportsArray();
>+      tree.view.getCellProperties(1, titleColumn, tagsProperties);
>+      tagsProperties = convertPropertiesToJSArray(tagsProperties);
>+      ok(tagsProperties.indexOf("OrganizerQuery_Tags") != -1,
>+         "OrganizerQuery_Tags is set");
>+
>+      // 2 - All Bookmarks
>+      let abProperties = createSupportsArray();
>+      tree.view.getCellProperties(2, titleColumn, abProperties);
>+      abProperties = convertPropertiesToJSArray(abProperties);
>+      ok(abProperties.indexOf("OrganizerQuery_AllBookmarks") != -1,
>+         "OrganizerQuery_AllBookmarks is set");
>+
>+      // Open All Bookmarks
>+      var abNode = tree.view.nodeForTreeIndex(2);
>+      asContainer(abNode).containerOpen = true;
>+
>+      // 3 - Bookmarks Toolbar
>+      let btProperties = createSupportsArray();
>+      tree.view.getCellProperties(3, titleColumn, btProperties);
>+      btProperties = convertPropertiesToJSArray(btProperties);
>+      ok(btProperties.indexOf("OrganizerQuery_BookmarksToolbar") != -1,
>+         "OrganizerQuery_BookmarksToolbar is set");
>+
>+      // 4 - Bookmarks Menu
>+      let bmProperties = createSupportsArray();
>+      tree.view.getCellProperties(4, titleColumn, bmProperties);
>+      bmProperties = convertPropertiesToJSArray(bmProperties);
>+      ok(bmProperties.indexOf("OrganizerQuery_BookmarksMenu") != -1,
>+         "OrganizerQuery_BookmarksMenu is set");
>+
>+      // 5 - Unfiled Bookmarks
>+      let ufbProperties = createSupportsArray();
>+      tree.view.getCellProperties(5, titleColumn, ufbProperties);
>+      ufbProperties = convertPropertiesToJSArray(ufbProperties);
>+      ok(ufbProperties.indexOf("OrganizerQuery_UnfiledBookmarks") != -1,
>+         "OrganizerQuery_UnfiledBookmarks is set");

you could probably immediately open All Bookmarks, then create an array

let containers = [
  "History",
  "Tags",
  ...,
]

and apply the same function to all elements with forEach, since the code is always the same there is no need to repeat it

>+
>+      SimpleTest.finish();

please close previously open container, even if it does not matter in this case, when people look at our tests to find example i want to show that closing open containers is a good code practice.

r=me

please land this today before the midnight freeze.
http://hg.mozilla.org/mozilla-central/rev/5994d00c1b71

I forgot to close the container, will fix that ASAP.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs review dietrich]
http://hg.mozilla.org/mozilla-central/rev/54346a708e3e for that.

P.S. Yes Marco, I forgot to add a period. Please use some other example for that.
Looks like you forgot to add the file on 1.9.2, so I pushed it to fix the bustage:
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bd8e88deb21a

Updated

3 years ago
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.