Closed Bug 399264 Opened 13 years ago Closed 13 years ago

stop hard coding folder roots in place: urls

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 3 beta5

People

(Reporter: moco, Assigned: sdwilsh)

References

()

Details

Attachments

(1 file, 6 obsolete files)

stop hard coding folder roots in place: urls

spun off from bug #387996 comment #23

we currently hard code the bookmark root (usually that's id 2) in eight places:

from http://lxr.mozilla.org/seamonkey/search?string=place%3Afolder%3D2

/browser/base/content/browser-menubar.inc, line 378 -- place="place:folder=2&group=3&expandQueries=1"
/browser/components/preferences/selectBookmark.xul, line 28 -- place="place:folder=2&group=3&excludeQueries=1"
/browser/components/places/content/bookmarksPanel.xul, line 73 -- place="place:folder=2&queryType=1"
/browser/components/places/content/bookmarkProperties.xul, line 167 -- place="place:folder=2&group=3&excludeItems=1&excludeQueries=1&excludeReadOnlyFolders=1"
/browser/components/places/content/editBookmarkOverlay.xul, line 147 -- place="place:folder=2&group=3&excludeItems=1&excludeQueries=1&excludeReadOnlyFolders=1"
/browser/components/places/content/moveBookmarks.xul, line 72 -- place="place:folder=2&group=3&excludeItems=1&excludeReadOnlyFolders=1"
/browser/components/places/content/controller.js, line 43 -- const ORGANIZER_ROOT_BOOKMARKS = "place:folder=2&group=3&excludeItems=1&queryType=1";
/browser/components/places/content/places.xul, line 355 -- place="place:folder=2&group=3&excludeItems=1&queryType=1"
drivers: saved searches are not portable across profiles, nor across restores due to this bug.
Flags: blocking-firefox3?
Priority: -- → P1
Target Milestone: --- → Firefox 3 M11
note to dietrich and drivers, I think we could still hardcode folder roots, if we fixed bug #405938.

note, separate from folder roots, we will have to hard code folder ids in place queries for user created folders.
> I think we could still hardcode folder roots

actually, this bug is about something else.

it is about the hard coding (and assumption) that folder id 2 is the bookmark root, which it may not always be.

it appears that we only do this in 3 places now:

/browser/base/content/browser-menubar.inc, line 378 -- place="place:folder=2&expandQueries=1"
/browser/components/preferences/selectBookmark.xul, line 28 -- place="place:folder=2&excludeQueries=1"
/browser/components/places/content/controller.js, line 43 -- const ORGANIZER_ROOT_BOOKMARKS = "place:folder=2&excludeItems=1&queryType=1";

> drivers: saved searches are not portable across profiles, nor across restores
> due to this bug.

that is bug #405938.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: P1 → P2
Target Milestone: Firefox 3 beta3 → Firefox 3 beta4
Assignee: nobody → sdwilsh
I'm not sure why bug 408125 depends on this - some care to explain?
I'm not doing so well with fixing this bug - suggestions on how to attack this?
Status: NEW → ASSIGNED
(In reply to comment #4)
> I'm not sure why bug 408125 depends on this - some care to explain?

there is an hardcoded value there

 28         place="place:folder=2&excludeQueries=1"

so fixing there could remove one of the hardcoded values here... probably not really a dependance.

(In reply to comment #5)
> I'm not doing so well with fixing this bug - suggestions on how to attack this?
> 

what are you having problems with?

you could define some consts on the idl that represent the special folders, and use those when encountered while parsing/serializing the query strings.
Well, a few issues really.  The big one is how (even a general idea I can work off of would be great) do I go from that static query to doing a generated one? I'm going to assume now that it's an xbl binding (which I haven't been able to find yet) that takes that static query and generates the content.  Is this correct?  Pointing me to the xbl binding may be sufficient for all I know too.


Did I ever mention that the places API is hard to get your head around?
Attached patch v0.1 (obsolete) — Splinter Review
test isn't working, but I'm not really sure how to fix it either...
Attached patch v1.0 (obsolete) — Splinter Review
yay - test finally works!
Attachment #305856 - Attachment is obsolete: true
Attachment #306062 - Flags: review?(dietrich)
Whiteboard: [has patch][needs review dietrich]
Attached patch v1.1 (obsolete) — Splinter Review
wrong root folder :/
Attachment #306062 - Attachment is obsolete: true
Attachment #306071 - Flags: review?(dietrich)
Attachment #306062 - Flags: review?(dietrich)
if LXR does not fail ORGANIZER_ROOT_BOOKMARKS is not used anywhere (http://lxr.mozilla.org/seamonkey/search?string=ORGANIZER_ROOT_BOOKMARKS)... we could drop it (since that should be a root why is it actually pointing to folder=2 that appear to be the bookmarks menu?)
Target Milestone: Firefox 3 beta4 → Firefox 3
Comment on attachment 306071 [details] [diff] [review]
v1.1

this looks fine so far. however, the other half of this fix is emitting the special identifiers when serializing queries (queriesToQueryString).
Attachment #306071 - Flags: review?(dietrich) → review-
Attached patch v2.0 (obsolete) — Splinter Review
Now with more tests! (I ran out of cowbell)

In an effort to better organize some of this code, I've gone and namespace'd the methods I added.  Using namespaces is fine - we've done it in storage code already and nobody has complained.  Self organizing code FTW!
Attachment #306071 - Attachment is obsolete: true
Attachment #306584 - Flags: review?(dietrich)
Comment on attachment 306584 [details] [diff] [review]
v2.0

whoops - should have checked make check output before submitting.  I caused another test to fail because it was expecting a number...
Attachment #306584 - Flags: review?(dietrich)
Attached patch v2.1 (obsolete) — Splinter Review
silly hardcoded queries :)
Attachment #306584 - Attachment is obsolete: true
Attachment #306586 - Flags: review?(dietrich)
Note: looking in the places organizer, I'm still seeing hardcoded values for the primary folders even though things are "fixed".  Are those saved somewhere?
(In reply to comment #17)
> Note: looking in the places organizer, I'm still seeing hardcoded values for
> the primary folders even though things are "fixed".  Are those saved somewhere?
> 

here are the places that immediately come to mind:

smart folders:
http://mxr.mozilla.org/seamonkey/source/browser/components/nsBrowserGlue.js#539

"all bookmarks" folder children:

http://mxr.mozilla.org/seamonkey/source/browser/components/places/content/utils.js#1952

we really should fix that code to use queriesToQueryString() instead of manually building the URIs.
Blocks: 408125
No longer depends on: 408125
Why? place: uris are supposed to be human readable & writable. This is especially wrong given that this code is perf-sensitive.
(In reply to comment #19)
> Why? place: uris are supposed to be human readable & writable. This is
> especially wrong given that this code is perf-sensitive.
> 

so that in cases like this we don't have to go update all callers?

that said, i buy that the perf-sensitive nature of the browserglue code justifies manually constructing the URIs there.
Comment on attachment 306586 [details] [diff] [review]
v2.1


>+
>+    // It wasn't one of our named constants, so just convert it to a string 
>+    aQuery.AppendInt(aFolderID);
>+  }
>+};

i don't think you need the end semicolon there. no compiler complaints on mac though...

add the smart-bookmark and left-pane fixes previously mentioned and this should be good to go.
Attachment #306586 - Flags: review?(dietrich) → review-
Whiteboard: [has patch][needs review dietrich] → [needs new patch]
Blocks: 405938
Whiteboard: [needs new patch] → [needs new patch][swag 1h]
Attached patch v2.2Splinter Review
addresses review comments
Attachment #306586 - Attachment is obsolete: true
Attachment #308100 - Flags: review?(dietrich)
Comment on attachment 308100 [details] [diff] [review]
v2.2

>+function folder_id(aQuery)
>+{
>+  var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
>+           getService(Ci.nsINavHistoryService);
>+
>+  dump("Checking query '" + aQuery + "'\n");
>+  var options = { };
>+  var queries = { };
>+  var size = { };
>+  hs.queryStringToQueries(aQuery, queries, size, options);
>+  var result = hs.executeQueries(queries.value, size.value, options.value);
>+  var root = result.root;
>+  root.containerOpen = true;
>+  do_check_true(root.hasChildren);
>+  var folderID = root.getChild(0).parent.itemId;

root and parent are the same node, should be able to do root.itemId instead.

in fact, you could probably not even add the items or open the container for an isolated test of this change, just check root.itemId. no matter though, broader coverage is a-ok :)

r=me
Attachment #308100 - Flags: review?(dietrich) → review+
I found that root.hasChildren would be false if I didn't add anything.  I never thought of checking the root.itemId.  I'll look into that maybe later today.
Whiteboard: [needs new patch][swag 1h] → [has patch][has review][can land]
talk about the bit rot :(

Checking in browser/base/content/browser-menubar.inc;
new revision: 1.153; previous revision: 1.152
Checking in browser/base/content/browser-places.js;
new revision: 1.121; previous revision: 1.120
Checking in browser/components/nsBrowserGlue.js;
new revision: 1.76; previous revision: 1.75
Checking in browser/components/places/content/controller.js;
new revision: 1.224; previous revision: 1.223
Checking in browser/components/places/content/utils.js;
new revision: 1.124; previous revision: 1.123
Checking in toolkit/components/places/src/nsNavHistoryQuery.cpp;
new revision: 1.37; previous revision: 1.36
Checking in toolkit/components/places/tests/unit/test_399264_query_to_string.js;
initial revision: 1.1
Checking in toolkit/components/places/tests/unit/test_399264_string_to_query.js;
initial revision: 1.1
Checking in toolkit/components/places/tests/unit/test_placeURIs.js;
new revision: 1.3; previous revision: 1.2
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][can land]
Target Milestone: Firefox 3 → Firefox 3 beta5
Flags: in-testsuite+
Flags: in-litmus-
Sometime between 2008031506 (OK) and 2008031514 (Broken), Places in new profiles is broken, the Library window is empty and nothing shows up in the Bookmarks sidebar. Could this be the cause?
(In reply to comment #26)
> Sometime between 2008031506 (OK) and 2008031514 (Broken), Places in new
> profiles is broken, the Library window is empty and nothing shows up in the
> Bookmarks sidebar. Could this be the cause?
> 

OK : 20080315_1126_firefox-3.0b5pre.en-US.win32.zip
NG : 20080315_1259_firefox-3.0b5pre.en-US.win32.zip (this bug checked in)

in Error Console,

[open sidebar]
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebNavigation.document]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/bindings/browser.xml :: get_contentDocument :: line 0"  data: no]

Error: uncaught exception: [Exception... "Component returned failure code: 0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS) [nsINavBookmarksService.runInBatchMode]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/places/utils.js :: anonymous :: line 1180"  data: no]


[open Library]
Error: uncaught exception: [Exception... "Component returned failure code: 0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS) [nsINavBookmarksService.runInBatchMode]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/places/utils.js :: anonymous :: line 1180"  data: no]
Depends on: 423267
also:

browser/components/places/utils.js
1145         uri = self._uri("place:folder=TAGS");
1146         uri = PlacesUtils._uri("place:folder=" + PlacesUtils.tagsFolderId);

this is not in https://bugzilla.mozilla.org/attachment.cgi?id=308100, did you checked in a different patch?
(In reply to comment #28)
> this is not in https://bugzilla.mozilla.org/attachment.cgi?id=308100, did you
> checked in a different patch?
No, but the patch was fairly bitrotten so I had fun trying to make sure everything was still caught :/
(In reply to comment #31)
> fixed in bug 423267.
> 

not all calls to _uri are fixed there... do we need a new bug?
Attachment #309952 - Attachment is obsolete: true
Attachment #309952 - Flags: review?(mano)
probably best to do a new bug.
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.