Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
13 years ago
9 years ago

People

(Reporter: Ben Goodger (use ben at mozilla dot org for email), Assigned: Annie Sullivan)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Re-implement Live Bookmarks
Assignee: nobody → annie.sullivan
(Assignee)

Comment 1

13 years ago
Created attachment 205594 [details] [diff] [review]
First pass at livemarks backend

This implements the backend for livemarks.  Things that still need to be done:
* Hook it up to the existing livemark UI
* Set the icon for the livemark folder
* Correctly handle multiple live bookmarks pointing to the same feed
* Handle moving live bookmarks
* Handle the case when the livemark is deleted while it is loading
Attachment #205594 - Flags: review?(bryner)
(Assignee)

Updated

13 years ago
Attachment #205594 - Flags: review?(bryner) → review?(brettw)
(Assignee)

Updated

13 years ago
Attachment #205594 - Flags: superreview?(bugs)

Comment 2

13 years ago
Comment on attachment 205594 [details] [diff] [review]
First pass at livemarks backend

How much of the stream listener/parser did you write? If it was more than a little, you should probably also have somebody like darin check it.

I assume you didn't do much to the parsing code, I just looked at it quickly.

Can you namespace all the annotation names like "livemark/foo"? I'd like extension authors to do this, so we should at least set a good example.

You use the annotation service for locking. This should work fine, but is there a reason you can't store the lock in the LivemarkInfo structure for it? That would be much more efficient.

>+  nsLivemarkLoadListener(nsNavBookmarks *aBMSVC,
>+                         nsIAnnotationService *aAnnotationService,
>+                         PRInt64 aFolderId,
>+                         nsIURI *aFeedURI)
>+    : mBMSVC(aBMSVC), mAnnotationService(aAnnotationService),
>+      mFolderId(aFolderId), mFeedURI(aFeedURI), mAborted(PR_FALSE)
>+  {
>+    NS_IF_ADDREF(mBMSVC);
>+    NS_IF_ADDREF(mAnnotationService);

You should probably use assertions to verify the bookmark and annotation services are not NULL instead of doing IF_ADDREF. It looks like the rest of the code will fail if these are NULL. If looks like you just use nsIAnnotationService and not a concrete pointer, so you can even use a COM pointer for that.


>+  rv = mAnnotationService->SetAnnotationInt64(mFeedURI,
>+                                              NS_LITERAL_CSTRING("livemark_expiration"),
>+                                              exptime, 0,
>+                                              nsAnnotationService::EXPIRE_NEVER);

Here and elsewhere you use a literal string when you have made a nice #define in the bookmark service header file. You should use your #define. (See the note on your defines below before you do this).

>+  rv = mAnnotationService->GetAnnotationInt64(mLivemarks[aLivemarkIndex].feedURI,
>+                                              NS_LITERAL_CSTRING("livemark_expiration"),
>+                                              &exprTime);
>+  if (rv == NS_OK) {

use "if (NS_SUCCEEDED(rv))"


> nsNavBookmarks.h:
>+#include "nsAnnotationService.h"

I don't think you use the concrete class, so just include the interface header file nsIAnnotationService.h. If you need to for some other reason, that's fine.


>+// Constants for livemark annotations
>+#define LMANNO_FEEDURI     NS_LITERAL_CSTRING("livemark_feedURI")
>+#define LMANNO_SITEURI     NS_LITERAL_CSTRING("livemark_siteURI")
>+#define LMANNO_LOCK        NS_LITERAL_CSTRING("livemark_lock")
>+#define LMANNO_EXPIRATION  NS_LITERAL_CSTRING("livemark_expiration")

Its probably better to not use the NS_LITERAL_CSTRING here and just #define the string. That way you can use the #define if you actually need a literal. For normal use, you then say NS_LITERAL_CSTRING(LMANNO_LOCK) when you want to use it.
Attachment #205594 - Flags: review?(brettw) → review+
A couple of questions:

- can you do a diff of the feed handler with the old one, so I can see the actual changes you've made in that file (attach a separate patch for that)
- let's discuss the integration of this into the core bookmarks service. 
(Assignee)

Comment 4

13 years ago
Created attachment 205661 [details] [diff] [review]
A diff between the bookmarks nsNavBookmarks.cpp and the places one

I intend to make the changes Brett suggested, but they're not done yet in this patch.
If TryParseAsSimpleRSS first is an optimization (I assumed the RDF parser returned quick and cheap if it didn't find rdf:RDF as the document root, but maybe not), wouldn't it be worthwhile to return early from TryParseAsSimpleRSS if (nname.Equals(NS_LITERAL_STRING("RDF")))? Right now, you'll look at every single element in an RSS 1.0 feed, seeing whether the localname is "rss" or "feed" (and ignoring namespaces, which is a tiny bit risky).

Comment 6

13 years ago
FYI: I have two parsing patches to the old feed handler that still apply here. bug262222 has fix for xml:base support in Atom and bug302133 fixes Atom title parsing. bug302133 contains a method called 'FindTextInChildNodes' that is preferable to FindTextInNode. The current method will err when there are child elements or interspersed CDATA blocks like this:

http://franklinmint.fm/2005/12/12/rsstest.xml
(Assignee)

Comment 7

13 years ago
(In reply to comment #5)
> If TryParseAsSimpleRSS first is an optimization (I assumed the RDF parser
> returned quick and cheap if it didn't find rdf:RDF as the document root, but
> maybe not), wouldn't it be worthwhile to return early from TryParseAsSimpleRSS
> if (nname.Equals(NS_LITERAL_STRING("RDF")))? Right now, you'll look at every
> single element in an RSS 1.0 feed, seeing whether the localname is "rss" or
> "feed" (and ignoring namespaces, which is a tiny bit risky).

I was thinking that there are hardly any RDF feeds left on the web now, so it seems silly to check for RDF first, but you're right, it's not very slow and it's safer that way.  I changed it back.  Thanks.
(Assignee)

Comment 8

13 years ago
(In reply to comment #6)
> FYI: I have two parsing patches to the old feed handler that still apply here.
> bug262222 has fix for xml:base support in Atom and bug302133 fixes Atom title
> parsing. bug302133 contains a method called 'FindTextInChildNodes' that is
> preferable to FindTextInNode. The current method will err when there are child
> elements or interspersed CDATA blocks like this:
> 
> http://franklinmint.fm/2005/12/12/rsstest.xml

Thanks, Robert.  I'll be happy to merge your patches into the new feed handler when everything has been reviewed.  It shouldn't be very different than the old one.
(Assignee)

Comment 9

13 years ago
Created attachment 205713 [details] [diff] [review]
Implements livemarks as a separate service, per Ben's request

This patch addresses Brett's and Phil's comments, and implements livemarks as a separate service per Ben's suggestion.

A problem that is introduced by this approach is that we need to go through all the UI code and add an additional check for nodeIsLivemark() in most of the places we check for nodeIsFolder() to make sure that we always change livemarks through the livemarks service.  (The previous implementation allowed the UI code to treat livemarks as folders for most purposes).
Attachment #205594 - Attachment is obsolete: true
Attachment #205661 - Attachment is obsolete: true
Attachment #205713 - Flags: review?(bugs)
Attachment #205594 - Flags: superreview?(bugs)
I need to think a little further about this. It's going in the right direction but not quite there yet. In the mean time - you add a property getter in the controller for the livemark service but seem to use it nowhere. What's this for?
(Assignee)

Comment 11

13 years ago
(In reply to comment #10)
> I need to think a little further about this. It's going in the right direction
> but not quite there yet. In the mean time - you add a property getter in the
> controller for the livemark service but seem to use it nowhere. What's this
> for?

The property getter was there because I was working on adding hooks for livemarks into all the javascript, but it got pretty ugly (i.e. when deleting a bookmark, check if it's a livemark and call the livemark service to delete it) so I took out the hooks but left in the getter by accident.  Sorry about that.

As for a better direction, Brian and I came up with a design where all containers like livemarks inherit from an nsIBookmarksContainerService.  nsINavBookmarkService gets a new CreateContainer() function which is a wrapper for CreateFolder().  CreateContainer() creates a folder and sets its container type in the moz_bookmark_folders table.

The container type is the contract id of the service, i.e. "@mozilla.org/browser/livemark-service;1".  Then when folders are moved, deleted, etc, the bookmarks service checks the container type of the folder, and if it's set, it notifies the service.

nsINavHistoryResultNode has a member variable for the container type, so the controller can check it to notify container services when containers are shown or hidden.

How does that sound?  I've started work on implementing it, and it's going pretty smoothly so far.
(Assignee)

Comment 12

13 years ago
Created attachment 205920 [details] [diff] [review]
Implements new bookmarks container service, and livemarks

This patch implements the nsIBookmarksContainer service, and an nsLivemarkService on top of it.  It has most of the functionality of livemarks.  Here's the major things that are missing:
-Need to set the right icon for livemarks
-Need to be able to import livemarks
-Building the context menu for a container should query if the service that created it needs to add any options
-It's hooked into the urlbar, so people can add livemarks, but most of the UI is missing.
-Eventually container services should be able to get notification when child items are about to be moved or deleted.
Attachment #205713 - Attachment is obsolete: true
Attachment #205920 - Flags: review?(bugs)
Attachment #205713 - Flags: review?(bugs)
Comment on attachment 205920 [details] [diff] [review]
Implements new bookmarks container service, and livemarks

Just a lot of nits and naming issues, nothing major:

>+/**
>+ * The Bookmarks Container Service interface provides a base class
>+ * for services that want to provide containers for bookmarks.
>+ * Some examples of possible services are the livemarks service and
>+ * the filesystem.  Containers are implemented as bookmarked folders.
>+ */
>+
>+[scriptable, uuid(45bf2020-9683-498c-9638-f08130c4151d)]
>+interface nsIBookmarksContainerService : nsISupports

Can I get you to rename this to nsIBookmarksContainer for now? Someone that implements this is not necessarily a singleton object and the name implies it. 

>+nsIRDFResource       *kLMRDF_type;
>+nsIRDFResource       *kLMRSS09_channel;
> ...

It looks like all of these are used in nsBookmarksFeedHandler.cpp. Can you stick a comment above saying that's where they're used because although they're initialized in this file they're not actually used. If we ever remove RDF completely (ha!) then we'll want to change the parser to parse RDF like it parses any other regular XML, and we don't want to forget to remove this detritus. 

>+  rv = bundleService->CreateBundle(
>+      "chrome://browser/locale/places/places.properties",

#define this URI somewhere near the top of the file, to make it easier for people to change path names if they ever change (they do, once in a while). 

>+class nsLivemarkService : nsILivemarkService

Do you mean : public nsILivemarkService ?

>+// THE ORIGINAL LOCATION OF THIS CODE IS
>+// /browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp
>+

You added this comment to nsNavBookmarks.cpp ... ?

Have Brian look over the rest of the Bookmarks changes...
Attachment #205920 - Flags: review?(bugs) → review+
Comment on attachment 205920 [details] [diff] [review]
Implements new bookmarks container service, and livemarks

>--- browser/components/places/src/nsNavHistoryResult.cpp	12 Dec 2005 21:03:04 -0000	1.19
>+++ browser/components/places/src/nsNavHistoryResult.cpp	15 Dec 2005 02:00:31 -0000
>@@ -136,16 +136,23 @@ NS_IMETHODIMP nsNavHistoryResultNode::Ge
> 
> /* attribute string title; */
> NS_IMETHODIMP nsNavHistoryResultNode::GetTitle(nsAString& aTitle)
> {
>   aTitle = mTitle;
>   return NS_OK;
> }
> 
>+/* attribute string folderType; */
>+NS_IMETHODIMP nsNavHistoryResultNode::GetFolderType(nsAString& aFolderType)
>+{
>+  aFolderType.Assign(NS_LITERAL_STRING(""));

Use:

aFolderType.Truncate(0);

Looks fine otherwise.
Attachment #205920 - Flags: superreview+
(Assignee)

Updated

13 years ago
Status: NEW → RESOLVED
Last Resolved: 13 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.

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.