Closed
Bug 510634
Opened 15 years ago
Closed 15 years ago
Wrong icons on bookmarks sidebar
Categories
(Firefox :: Bookmarks & History, defect, P2)
Firefox
Bookmarks & History
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)
6.93 KB,
patch
|
Details | Diff | Splinter Review |
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•15 years ago
|
Flags: blocking-firefox3.6?
Updated•15 years ago
|
Assignee: nobody → dietrich
Flags: blocking-firefox3.6? → blocking-firefox3.6+
Comment 2•15 years ago
|
||
Marco: although I assigned this to Dietrich as the original regressor, I imagine he wouldn't mind if you took it. :)
Comment 3•15 years ago
|
||
since PlacesUIUtils is per-window, the left pane cache needs to be initialized in the sidebar's window scope. patch coming.
Updated•15 years ago
|
OS: Windows XP → All
Hardware: x86 → All
Comment 4•15 years ago
|
||
Attachment #402037 -
Flags: review?(mak77)
Updated•15 years ago
|
Whiteboard: [has patch][needs review mak]
Updated•15 years ago
|
Attachment #402037 -
Flags: review?(mak77) → review-
Comment 5•15 years ago
|
||
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
Updated•15 years ago
|
Priority: -- → P2
Updated•15 years ago
|
Assignee: dietrich → mano
Updated•15 years ago
|
Flags: in-testsuite?
Whiteboard: [has patch][needs review mak] → [has patch][needs new patch with tests]
Updated•15 years ago
|
Whiteboard: [has patch][needs new patch with tests] → [needs new patch with tests]
Assignee | ||
Comment 7•15 years ago
|
||
Attachment #402179 -
Attachment is obsolete: true
Attachment #404792 -
Flags: review?(dietrich)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs new patch with tests] → [needs review dietrich]
Updated•15 years ago
|
Attachment #404792 -
Flags: review?(dietrich) → review+
Comment 8•15 years ago
|
||
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.
Assignee | ||
Comment 9•15 years ago
|
||
Attachment #404792 -
Attachment is obsolete: true
Assignee | ||
Comment 10•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs review dietrich]
Assignee | ||
Comment 11•15 years ago
|
||
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.
Assignee | ||
Comment 12•15 years ago
|
||
status1.9.2:
--- → beta1-fixed
Comment 13•15 years ago
|
||
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
Assignee | ||
Comment 14•15 years ago
|
||
Thanks Gavin.
Updated•9 years ago
|
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•