372508 - should compact the bookmark apis

Status: VERIFIED FIXED

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

Description

[Dietrich Ayala (:dietrich)]

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 moz_bookmark.id property.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: wip patch for folder/item id unification

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: id unification fix v1

this patch makes folder ids use moz_bookmarks.id instead of moz_bookmarks_folders.id, 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.

Comment 3

[Asaf Romano (gone)]

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.

Comment 4

[Dietrich Ayala (:dietrich)]

separated folder id unification out into bug 379986, assigning this to mano per irc.

Comment 5

[Asaf Romano (gone)]

The schema changes for this bug were done in bug 379986, this bug by itself does not block turning on places-bookmarks.

Comment 6

[Asaf Romano (gone)]

Attachment: wip

Comment 7

[Asaf Romano (gone)]

Attachment: part 1 (checked in)

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.

Comment 8

[(not reading, please use [e-mail] instead)]

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"

Comment 9

[Asaf Romano (gone)]

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!).

Comment 10

[(not reading, please use [e-mail] instead)]

thanks for the quick answers.

What about the "excludeQueries=1" part?  What did you remove that?

Comment 11

[Asaf Romano (gone)]

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

Comment 12

[Asaf Romano (gone)]

(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.

Comment 13

[Asaf Romano (gone)]

Attachment: part 2 - folderId removal (checked in)

Comment 14

[(not reading, please use [e-mail] instead)]

Comment on attachment 264606 [details] [diff] [review]
part 2 - folderId removal (checked in)

r=sspitzer

Comment 15

[Asaf Romano (gone)]

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

Comment 16

[Asaf Romano (gone)]

Attachment: onFolderChanged removal (checked in)

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 265731 [details] [diff] [review]
onFolderChanged removal (checked in)

r=me, thanks.

Comment 18

[Asaf Romano (gone)]

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

Comment 19

[Asaf Romano (gone)]

Attachment: insertItem -> insertBookmark, add title parameter (checked in)

Comment 20

[Dietrich Ayala (:dietrich)]

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

s/ne/new/
                                                        nsnull,
>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?

Comment 21

[Asaf Romano (gone)]

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 22

[Asaf Romano (gone)]

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

Comment 23

[Asaf Romano (gone)]

We're done here, I think. Please file follow ups for remaining cleanup, if any.

Comment 24

[Gervase Markham [:gerv]]

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