379211 - Use item IDs for annotating bookmark/folder items rather than place: URIs

Status: RESOLVED FIXED

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

Description

[Asaf Romano (gone)]

Comment 1

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

over irc, asaf writes:

<Mano> don't bother working on the annos bug yet
<Mano> at least until i finish refactoring annosvc

Comment 2

[Asaf Romano (gone)]

Attachment: wip

Comment 3

[Asaf Romano (gone)]

Attachment: wip

Comment 4

[Asaf Romano (gone)]

Attachment: applies to trunk

Comment 5

[Asaf Romano (gone)]

Attachment: with tests

Comment 6

[Asaf Romano (gone)]

Attachment: more...

Comment 7

[Asaf Romano (gone)]

Attachment: more...

Comment 8

[Asaf Romano (gone)]

Attachment: more, not less. and not even busted!

Comment 9

[Asaf Romano (gone)]

Attachment: more...

Comment 10

[Asaf Romano (gone)]

Attachment: more...

Comment 11

[tabmix.onemen]

> Index: toolkit/components/places/src/nsLivemarkService.js
> ===================================================================
>....
>....
> -    this._pushLivemark(folders[0], livemarks[i], feedURI);
> +    this._pushLivemark(folders[0], feedURI);

folders is undefined...
  +    this._pushLivemark(livemarks[i], feedURI);


> Index: browser/components/places/content/controller.js
> ===================================================================
>....
>....
> -        // For bookmark-items also include the bookmark-specific annotations
> -        if ("bookmark" in nodeData) {
> -          var placeURI = PlacesUtils.bookmarks.getItemURI(node.bookmarkId);
> -          names = PlacesUtils.annotations.getPageAnnotationNames(placeURI, {});
> +        // For items also include the item-specific annotations
> +        if ("bookmark" in nodeData || "folder" in nodeData) {
> +          names = PlacesUtils.annotations
> +                             .getItemAnnotationNames(node.bookmarkId, {});

"folder" nodes don't have bookmarkId yet.

Comment 12

[Asaf Romano (gone)]

Thanks oneman.one (is that your name? ;) ), esp. for catching the error in the livemarks service(!).

The later is known, the current patch is all wrong without the bookmarkId & folderId unification which is half-done by now.

Comment 13

[Asaf Romano (gone)]

Attachment: ready for some sort of prime time

Comment 14

[tabmix.onemen]

>Index: toolkit/components/places/src/nsLivemarkService.js
>===================================================================
>....
>....

-    this._pushLivemark(folders[0], livemarks[i], feedURI);
+    this._pushLivemark(livemarks[0], feedURI);
   }
 }

almost there :)

+    this._pushLivemark(livemarks[i], feedURI);

Comment 15

[Asaf Romano (gone)]

Attachment: don't break legacy-bookmarks

and other minor fixes.

Comment 16

[Dietrich Ayala (:dietrich)]

Comment on attachment 264287 [details] [diff] [review]
don't break legacy-bookmarks

r+, looks good, much cleaner for bookmark/folder annotations. thanks also for combining the folder and bookmark ids, and the rest of the cleanup work (PR types etc).

is there a reason you didn't remove mFolderId from nsNavHistoryFolderResultNode in nsNavHistoryResult.h?

the error i pasted into irc (and most of my other comments :/) were fixed by this updated patch.

>   /**
>    * Notify this observer that a bookmark's information has changed.  This
>    * will be called whenever any attributes like "title" are changed.
>    * 
>-   * @param bookmarkId The id of the bookmark that was changed.
>-   * @param bookmark The URI of the bookmark which changed.
>-   * @param property The property which changed.
>+   * @param qBookmarkId
>+   *        The id of the bookmark that was changed.

nit: s/qBookmarkId/aBookmarkId/

>+   * @param aProperty
>+   *        The property which changed.
>+   * @param aIsAnnotationProperty
>+   *        Is aProperty the name of an item annotation
>    *
>    * property = "cleartime": (history was deleted, there is no last visit date):
>    *                         value = none
>    * property = "title": value = new title
>    * property = "favicon": value = new "moz-anno" URL of favicon image
>-   * property = "uri": value = empty, get the value of the changed bookmark URI
>-   *                           from the |bookmark| parameter.
>+   * property = "uri": value = new uri spec
>+   * aIsAnnotationProperty = true: value = empty string
>    */
>-  void onItemChanged(in PRInt64 bookmarkId, in nsIURI bookmark, in ACString property,
>-                     in AString value);
>+  void onItemChanged(in long long aBookmarkId, in ACString aProperty,
>+                     in boolean aIsAnnotation, in AUTF8String aValue);

nit: aIsAnnotationProperty docs should be consistent w/ the name in the api.

>+// nsAnnotationService::RemoveAnnotationInternal
>+//
>+//    We don't remove anything from the moz_anno_attributes table. If we delete
>+//    the last item of a given name, that item really should go away. It will
>+//    get cleaned up on the next shutdown.
>+//
>+//    XXX NOT REALLY, see nsNavHistoryExpire::ExpireAnnotationsParanoid

is there a bug for this?

>   // insert a bookmark 
>   var newId = bmsvc.insertItem(testRoot, uri("http://google.com/"), bmsvc.DEFAULT_INDEX);
>   do_check_eq(observer._itemAddedId, newId);
>-  do_check_eq(observer._itemAdded.spec, "http://google.com/");
>   do_check_eq(observer._itemAddedFolder, testRoot);
>   do_check_eq(observer._itemAddedIndex, testStartIndex);

should rather check getBookmarkURI instead of just removing the test.

> PlacesRemoveItemTransaction.prototype = {
>   __proto__: PlacesBaseTransaction.prototype, 
>   
>   doTransaction: function PRIT_doTransaction() {
>     this.LOG("Remove Item: " + this._uri.spec + " from: " + this._oldContainer + "," + this._oldIndex);
>     this._title = this.bookmarks.getItemTitle(this._id);
>-    this._placeURI = this.bookmarks.getItemURI(this._id);
>-    this._annotations = this.utils.getAnnotationsForURI(this._placeURI);
>-    PlacesUtils.annotations.removePageAnnotations(this._placeURI);
>+    this._annotations = this.utils.getAnnotationsForItem(this._id);
>+    PlacesUtils.annotations.removeItemAnnotations(this._id);

hm, i know this is from a previous patch, but why the mixed use of the PlacesUtils global and this.utils?

Comment 17

[Asaf Romano (gone)]

Because I keep forgetting we cannot rely on window being the global script objecgt in transactions... this line though should go away, we're now removing the annos in removeItem.

Comment 18

[Asaf Romano (gone)]

> >+//    XXX NOT REALLY, see nsNavHistoryExpire::ExpireAnnotationsParanoid

> is there a bug for this?

Bug 319455, kinda.

Comment 19

[Asaf Romano (gone)]

Attachment: for checkin

Comment 20

[Asaf Romano (gone)]

mozilla/toolkit/components/places/public/nsIAnnotationService.idl 1.14
mozilla/toolkit/components/places/public/nsINavBookmarksService.idl 1.37
mozilla/toolkit/components/places/public/nsINavHistoryService.idl 1.57
mozilla/toolkit/components/places/src/nsAnnoProtocolHandler.cpp 1.5
mozilla/toolkit/components/places/src/nsAnnotationService.cpp 1.21
mozilla/toolkit/components/places/src/nsAnnotationService.h 1.11
mozilla/toolkit/components/places/src/nsLivemarkService.js 1.12
mozilla/toolkit/components/places/src/nsNavBookmarks.cpp 1.85
mozilla/toolkit/components/places/src/nsNavBookmarks.h 1.40
mozilla/toolkit/components/places/src/nsNavHistory.cpp 1.122
mozilla/toolkit/components/places/src/nsNavHistory.h 1.77
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.92
mozilla/toolkit/components/places/src/nsNavHistoryResult.h 1.34
mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js 1.10
mozilla/toolkit/components/places/tests/chrome/test_371798.xul 1.3
mozilla/toolkit/components/places/tests/chrome/test_add_livemark.xul 1.2
mozilla/toolkit/components/places/tests/unit/test_annotations.js 1.3
mozilla/toolkit/components/places/tests/unit/test_history.js 1.4
mozilla/toolkit/components/places/tests/unit/test_result_sort.js 1.2
mozilla/browser/base/content/browser.js 1.781
mozilla/browser/components/microsummaries/public/Makefile.in 1.2
mozilla/browser/components/microsummaries/public/nsIMicrosummaryService.idl 1.9
mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js 1.63
mozilla/browser/components/places/content/bookmarkProperties.js 1.44
mozilla/browser/components/places/content/controller.js 1.146
mozilla/browser/components/places/content/moveBookmarks.js 1.3
mozilla/browser/components/places/content/toolbar.xml 1.80
mozilla/browser/components/places/content/utils.js 1.34
mozilla/browser/components/places/src/nsPlacesImportExportService.cpp 1.2
mozilla/browser/components/places/tests/unit/test_bookmarks_html.js 1.2

Comment 21

[pal-moz]

regression?
crash (app error), and many bookmarks-*.html files are created.
* is 1,2,3,.....

Comment 22

[Asaf Romano (gone)]

you've a range? I would rather bet that's bug 379986 or anything that forces import, etc.

Comment 23

[pal-moz]

no problem with 050904,
problem with 051004.

same problem with both new/existing profile.

Comment 24

[Asaf Romano (gone)]

File a bug please (generally a better solution btw).

Comment 25

[pal-moz]

this morning (in Japan), try the same step as last night.
but no crash so far.
I've no idea.

Comment 27

[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