Closed Bug 510634 Opened 15 years ago Closed 15 years ago

Wrong icons on bookmarks sidebar

Categories

(Firefox :: Bookmarks & History, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: ShareBird, Assigned: asaf)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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
Flags: blocking-firefox3.6?
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
Attached patch fix (obsolete) — Splinter Review
Attachment #402037 - Flags: review?(mak77)
Whiteboard: [has patch][needs review mak]
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
Attached patch actual fix (obsolete) — Splinter Review
needs a test still
Attachment #402037 - Attachment is obsolete: true
Priority: -- → P2
Assignee: dietrich → mano
Flags: in-testsuite?
Whiteboard: [has patch][needs review mak] → [has patch][needs new patch with tests]
Whiteboard: [has patch][needs new patch with tests] → [needs new patch with tests]
Attached patch patch with tests (obsolete) — Splinter Review
Attachment #402179 - Attachment is obsolete: true
Attachment #404792 - Flags: review?(dietrich)
Whiteboard: [needs new patch with tests] → [needs review dietrich]
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.
Attached patch for checkinSplinter Review
Attachment #404792 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/5994d00c1b71 I forgot to close the container, will fix that ASAP.
Status: NEW → RESOLVED
Closed: 15 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
Thanks Gavin.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: