Closed Bug 372508 Opened 14 years ago Closed 14 years ago

should compact the bookmark apis


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




Firefox 3 alpha5


(Reporter: dietrich, Assigned: mano)




(4 files, 3 obsolete files)

Type-specific APIs should be folded into the type-agnostic APIs where appropriate. Also, there are other places where common patterns dictate API unification. 

get/setFolderTitle -> get/setItemTitle
getFolderURI/getItemURI -> getPlaceURI (to disambiguate from getBookmarkURI)
indexOfItem/indexOfFolder -> getItemIndex (see if indexOfItem has a use-case)
moveFolder -> moveItem
getFolderIdForItem -> getParentId
insertItem and insertSeparator should take a title parameter
getFolderReadonly -> getItemIsReadOnly (only for livemarks for now, but this allows future changes)

Note, this will require modification of the query result nodes, adding the property.
Depends on: 372501
Assignee: nobody → dietrich
Blocks: 371823
Blocks: 372722
Blocks: 325342
Blocks: 370099
Whiteboard: affects schema?
Attached patch id unification fix v1 (obsolete) — Splinter Review
this patch makes folder ids use instead of, thereby allowing us to convert apis to be type-agnostic where possible.

the upgrade story is to dump/import the bookmarks table, recreating all the ids. there will be some detritus left behind for those already testing places bookmarks builds: any annotations of place ids (until bug 375629 is fixed anyway). fwiw, i attempted a version of this patch that initialized the anno service early, and used it to migrate the annos, but it was a ton of infrastructure change for an insignificant win, and the upcoming move away from place URI annotation obviates the need anyways.

also, while testing this, i found that bug 376008 regressed ForceMigrateBookmarks: by moving to a pref instead of importing within the service itself, and not updating the migration code, bookmarks.html is not re-imported after blowing away the bookmarks tables. the fix in this patch is to set the pref in ForceMigrateBookmarks. this isn't a dependency on /browser as much as it's an awareness of it, but is still kind of distasteful. lmk if you have alternate suggestions for how to solve this problem.
Attachment #263161 - Attachment is obsolete: true
Attachment #263671 - Flags: review?(mano)
Hrm, do we really need moz_bookmarks_folders at all now?

It's only the type field that applies only to bookmark folders (that field could be either an empty string or "toolbar" or a contract id pointing to a class which implements the remote container interface, right?). I think we should rather merge the two tables before enabling places-bookmarks on trunk, the migration story is a non-issue until then IMO.
Depends on: 379986
separated folder id unification out into bug 379986, assigning this to mano per irc.
Assignee: dietrich → mano
Attachment #263671 - Attachment is obsolete: true
Attachment #263671 - Flags: review?(mano)
The schema changes for this bug were done in bug 379986, this bug by itself does not block turning on places-bookmarks.
No longer blocks: 370099
Whiteboard: affects schema?
Priority: -- → P2
Target Milestone: --- → Firefox 3 alpha5
So this is what i need on trunk to make places-hacking suck less, I will merge the more-complicated methods after we turn on places-bookmarks on trunk.
Attachment #264362 - Attachment is obsolete: true
Attachment #264366 - Flags: review?(sspitzer)
Comment on attachment 264366 [details] [diff] [review]
part 1 (checked in)

r=sspitzer, everything looks good (and much more compact.)

three questions:

why the switch to unsigned short (PRUint16) if the underlying db uses a long (PRInt32) for type?

+const unsigned short TYPE_BOOKMARK = 1;

+  *aType = (PRUint16)mDBGetItemProperties->AsInt32(kGetItemPropertiesIndex_Type);

Perhaps we should just make the consts be a long and make type be long, and use PRInt32 and avoid the cast?

2)  for the other api changes that dietrich suggested (like getItemIsReadOnly), will you log a spin off bug on those, or keep this bug open?

3)  is this part of another bug fix?

-            place="place:folder=1&group=3&excludeItems=1&excludeQueries=1&excludeReadOnlyFolders=1" 
+            place="place:folder=2&group=3&excludeItems=1&excludeReadOnlyFolders=1"
Attachment #264366 - Flags: review?(sspitzer) → review+
1) This is the interface, not the implementation, I would use short for the db if mozstorage allowed that. AFAICT, mozstorage actually uses long long (print64) either way.

2) I'm keeping this one open for now.

3) Well, yeah, but it's not filed; folder=1 is obsolete for some time now and we somehow forgot to fix this last use-case (hopefully!).
thanks for the quick answers.

What about the "excludeQueries=1" part?  What did you remove that?
mozilla/toolkit/components/places/public/nsINavBookmarksService.idl 1.38
mozilla/toolkit/components/places/src/nsNavBookmarks.cpp 1.86
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.93
mozilla/toolkit/components/places/src/nsNavHistoryResult.h 1.35
mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js 1.11
mozilla/browser/base/content/browser.js 1.782
mozilla/browser/components/places/content/bookmarkProperties.js 1.45
mozilla/browser/components/places/content/controller.js 1.147
mozilla/browser/components/places/content/moveBookmarks.xul 1.3
mozilla/browser/components/places/content/utils.js 1.35
mozilla/browser/components/places/src/nsPlacesImportExportService.cpp 1.3
(In reply to comment #10)
> thanks for the quick answers.
> What about the "excludeQueries=1" part?  What did you remove that?

Hrm, it was there so we hide the history and subscription queries. I guess this options should be applied to this dialog regardless of that though.

I will make sure to restore that in the next patch.
Attachment #264366 - Attachment description: part 1 → part 1 (checked in)
Comment on attachment 264606 [details] [diff] [review]
part 2 - folderId removal (checked in)

Attachment #264606 - Flags: review?(sspitzer) → review+
mozilla/browser/components/places/content/bookmarkProperties.js 1.46
mozilla/browser/components/places/content/controller.js 1.149
mozilla/browser/components/places/content/menu.xml 1.68
mozilla/browser/components/places/content/moveBookmarks.js 1.4
mozilla/browser/components/places/content/places.js 1.85
mozilla/browser/components/places/content/toolbar.xml 1.81
mozilla/browser/components/places/content/tree.xml 1.63
mozilla/browser/components/places/content/utils.js 1.36
mozilla/browser/components/places/src/nsPlacesImportExportService.cpp 1.6
mozilla/toolkit/components/places/public/nsINavHistoryService.idl 1.59
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.95
mozilla/toolkit/components/places/src/nsNavHistoryResult.h 1.37
Attachment #264606 - Attachment description: part 2 - folderId removal → part 2 - folderId removal (checked in)
Comment on attachment 265731 [details] [diff] [review]
onFolderChanged removal (checked in)

r=me, thanks.
Attachment #265731 - Flags: review?(dietrich) → review+
Attachment #265731 - Attachment description: onFolderChange removal → onFolderChanged removal
mozilla/toolkit/components/places/public/nsINavBookmarksService.idl 1.42
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.99
mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js 1.21
mozilla/toolkit/components/places/tests/chrome/test_371798.xul 1.5
mozilla/toolkit/components/places/tests/chrome/test_add_livemark.xul 1.4
mozilla/browser/base/content/browser.js 1.793
Attachment #265731 - Attachment description: onFolderChanged removal → onFolderChanged removal (checked in)
Comment on attachment 265745 [details] [diff] [review]
insertItem -> insertBookmark, add title parameter (checked in)

>Index: toolkit/components/places/public/nsINavBookmarksService.idl
>RCS file: /cvsroot/mozilla/toolkit/components/places/public/nsINavBookmarksService.idl,v
>retrieving revision 1.42
>diff -u -p -8 -r1.42 nsINavBookmarksService.idl
>--- toolkit/components/places/public/nsINavBookmarksService.idl	22 May 2007 23:56:50 -0000	1.42
>+++ toolkit/components/places/public/nsINavBookmarksService.idl	23 May 2007 00:01:22 -0000
>@@ -155,17 +155,17 @@ interface nsINavBookmarkObserver : nsISu
> };
> /**
>  * The BookmarksService interface provides methods for managing bookmarked
>  * history items.  Bookmarks consist of a set of user-customizable
>  * folders.  A URI in history can be contained in one or more such folders.
>  */
>-[scriptable, uuid(5e44a4e2-5db6-412a-b153-c732c9c62fd5)]
>+[scriptable, uuid(c3307160-525d-4d3c-9d75-b4f13a9ca05d)]
> interface nsINavBookmarksService : nsISupports
> {
>   /**
>    * The folder ID of the Places root.
>    */
>   readonly attribute long long placesRoot;
>   /**
>@@ -193,28 +193,30 @@ interface nsINavBookmarksService : nsISu
>    */
>   const short DEFAULT_INDEX = -1;
>   const unsigned short TYPE_BOOKMARK = 1;
>   const unsigned short TYPE_FOLDER = 2;
>   const unsigned short TYPE_SEPARATOR = 3;
>   /**
>-   * Inserts a child item into the given folder. If this item already exists in
>+   * Inserts a child bookmark into the given folder. If this item already exists in
>    * the given folder, it will be moved to the new position.
>    *  @param aParentFolder
>    *         The id of the parent folder
>    *  @param aURI
>    *         The URI to insert
>    *  @param aIndex
>    *         The index to insert at, or -1 to append
>+   *  @param aTitle
>+   *         The title for the ne bookmark

>Index: browser/components/places/content/controller.js
>RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
>retrieving revision 1.152
>diff -u -p -8 -r1.152 controller.js
>--- browser/components/places/content/controller.js	22 May 2007 22:03:54 -0000	1.152
>+++ browser/components/places/content/controller.js	23 May 2007 00:01:46 -0000
>@@ -1655,19 +1655,18 @@ PlacesCreateItemTransaction.prototype = 
>   __proto__: PlacesBaseTransaction.prototype,
>   // childItemsTransactions support for the create-folder transaction
>   get container() { return this._container; },
>   set container(val) { return this._container = val; },
>   doTransaction: function PCIT_doTransaction() {
>     var bookmarks = this.utils.bookmarks;
>-    this._id = bookmarks.insertItem(this.container, this._uri, this._index);
>-    if (this._title)
>-      bookmarks.setItemTitle(this._id, this._title);
>+    this._id = bookmarks.insertBookmark(this.container, this._uri, this._index,
>+                                        this._title);

does removing that if() present a problem, is this._title initialized properly elsewhere?
Attachment #265745 - Flags: review?(dietrich) → review+
It's initialized in the factory method for this transaction. Either way, xpconnect will map |undefined| to an empty string for us when passed as a string parameter.
Comment on attachment 265745 [details] [diff] [review]
insertItem -> insertBookmark, add title parameter (checked in)

mozilla/toolkit/components/places/public/nsINavBookmarksService.idl 1.43
mozilla/toolkit/components/places/src/nsLivemarkService.js 1.13
mozilla/toolkit/components/places/src/nsNavBookmarks.cpp 1.96
mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js 1.22
mozilla/toolkit/components/places/tests/chrome/test_371798.xul 1.6
mozilla/toolkit/components/places/tests/unit/test_annotations.js 1.7
mozilla/toolkit/components/places/tests/unit/test_result_sort.js 1.5
mozilla/browser/components/migration/src/nsIEProfileMigrator.cpp 1.58
ozilla/browser/components/migration/src/nsOperaProfileMigrator.cpp 1.63
mozilla/browser/components/migration/src/nsSafariProfileMigrator.cpp 1.40
mozilla/browser/components/places/content/controller.js 1.153
mozilla/browser/components/places/src/nsPlacesImportExportService.cpp 1.19
Attachment #265745 - Attachment description: insertItem -> insertBookmark, add title parameter → insertItem -> insertBookmark, add title parameter (checked in)
We're done here, I think. Please file follow ups for remaining cleanup, if any.
Closed: 14 years ago
Resolution: --- → FIXED
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.

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