If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add isLivemark(folderID), get/setSiteURI(folderID), and get/setFeedURI(folderID) to livemark service

RESOLVED FIXED in Firefox 2 alpha2

Status

()

Firefox
Bookmarks & History
--
enhancement
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: Joe Hughes, Assigned: Joe Hughes)

Tracking

({fixed1.8.1})

2.0 Branch
Firefox 2 alpha2
fixed1.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

12 years ago
 
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 2 alpha2
(Assignee)

Updated

12 years ago
Blocks: 330063
(Assignee)

Comment 1

12 years ago
Created attachment 217084 [details] [diff] [review]
Proposed additions to the livemark service interface
Attachment #217084 - Flags: review?(brettw)

Comment 2

12 years ago
Comment on attachment 217084 [details] [diff] [review]
Proposed additions to the livemark service interface

>+  /**
>+   * Determines whether the folder with the given folder ID identifies
>+   * a livemark container.
>+   *
>+   * @param folder    A folder ID
>+   *
>+   * @returns PR_TRUE if the given folder is a livemark folder, or
>+   *          PR_FALSE otherwise
>+   *
>+   * @throws NS_ERROR_INVALID_ARG if the folder ID isn't known
>+   */
>+  PRBool isLivemark(in PRInt64 folder);

s/PRBool/boolean
s/PR_TRUE/true
s/PR_FALSE/false


>+  /**
>+   * Gets the URI of the website associated with a livemark container.
>+   *
>+   * @param container  The folder ID of a livemark container
>+   *
>+   * @returns nsIURI representing the URI of the website; if the livemark
>+   *          container doesn't have a valid site URI, the spec (string)
>+   *          of the nsIURI object returned will be the empty string.

Is it legal for an nsIURI to have an empty spec? I thing returning NULL here would be better anyway.

>+  /**
>+   * Gets the URI of the syndication feed associated with a livemark container.
>+   *
>+   * @param container  The folder ID of a livemark container
>+   *
>+   * @returns nsIURI representing the URI of the feed; if the livemark
>+   *          container doesn't have a valid feed URI, the spec (string)
>+   *          of the nsIURI object returned will be the empty string.

Same thing here.

r=me with those fixed
Attachment #217084 - Flags: review?(brettw) → review+
(Assignee)

Comment 3

12 years ago
Created attachment 217104 [details] [diff] [review]
Implemented methods, removed unused mBookmarksService
Attachment #217084 - Attachment is obsolete: true
Attachment #217104 - Flags: review?
(Assignee)

Updated

12 years ago
Attachment #217104 - Flags: review? → review?(brettw)

Comment 4

12 years ago
Comment on attachment 217104 [details] [diff] [review]
Implemented methods, removed unused mBookmarksService

>+NS_IMETHODIMP
>+nsLivemarkService::IsLivemark(PRInt64 aFolder, PRBool *aResult)
>+{
>+  nsresult rv;
>+  *aResult = PR_FALSE;
>+
>+  nsCOMPtr<nsIURI> folderURI;
>+  rv = nsNavBookmarks::GetBookmarksService()->GetFolderURI(
>+      aFolder, getter_AddRefs(folderURI));
>+  NS_ENSURE_SUCCESS(rv, rv);

You should do:
  nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
  NS_ENSURE_TRUE(bookmarks, NS_ERROR_FAILURE);
same in other places.

>+NS_IMETHODIMP
>+nsLivemarkService::GetSiteURI(PRInt64 aContainer, nsIURI **aURI)
>+{
>+  nsresult rv;
>+
>+  if (!aURI)
>+    return NS_ERROR_NULL_POINTER;
>+
>+  // First off, make sure we're dealing with a livemark ID.
>+  PRBool isLivemark = PR_FALSE;
>+  rv = IsLivemark(aContainer, &isLivemark);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!isLivemark)
>+    return NS_ERROR_INVALID_ARG;
>+
>+  // Now convert the container ID to a container URI for annotation operations
>+  nsCOMPtr<nsIURI> containerURI;
>+  rv = nsNavBookmarks::GetBookmarksService()->GetFolderURI(
>+      aContainer, getter_AddRefs(containerURI));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // If there's no site URI annotation, return null
>+  PRBool hasSiteURI = PR_FALSE;
>+  rv = mAnnotationService->HasAnnotation(
>+      containerURI, NS_LITERAL_CSTRING(LMANNO_SITEURI), &hasSiteURI);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!hasSiteURI) {
>+    *aURI = nsnull;
>+    return NS_OK;
>+  }

I think you should just try to get the string like you did below and let that fail. HasAnnotation does the exact same SQL, so doing it twice is a waste of time.


>+NS_IMETHODIMP
>+nsLivemarkService::SetSiteURI(PRInt64 aContainer, nsIURI *aSiteURI)


>+NS_IMETHODIMP
>+nsLivemarkService::GetFeedURI(PRInt64
...
>+  // If there's no feed URI annotation, that means this isn't a livemark
>+  PRBool hasSiteURI = PR_FALSE;
>+  rv = mAnnotationService->HasAnnotation(
>+      containerURI, NS_LITERAL_CSTRING(LMANNO_SITEURI), &hasSiteURI);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!hasSiteURI) {
>+    return NS_ERROR_INVALID_ARG;
>+  }

Same as above, you can remove HasAnnotation.


>+NS_IMETHODIMP
>+nsLivemarkService::SetFeedURI(PRInt64 aContainer, nsIURI *aFeedURI)

This function doesn't update the livemark lists that the annotation service keeps (mLivemarks). This will have to be updated for the changes to be reflected in the next updates. You should talk to Annie if you have any questions about this, I don't know the details.
Attachment #217104 - Flags: review?(brettw) → review-
(Assignee)

Comment 5

12 years ago
Created attachment 217224 [details] [diff] [review]
Addressed Brett's comments, fixed a bug I came across in OnContainerMoved
Attachment #217104 - Attachment is obsolete: true
Attachment #217224 - Flags: review?(brettw)

Updated

12 years ago
Attachment #217224 - Flags: review?(brettw) → review+
(Assignee)

Updated

12 years ago
Attachment #217224 - Flags: superreview?(darin)

Comment 6

12 years ago
Comment on attachment 217224 [details] [diff] [review]
Addressed Brett's comments, fixed a bug I came across in OnContainerMoved

>Index: browser/components/places/src/nsLivemarkService.cpp

>+NS_IMETHODIMP
>+nsLivemarkService::GetSiteURI(PRInt64 aContainer, nsIURI **aURI)
>+{
>+  nsresult rv;
>+
>+  if (!aURI)
>+    return NS_ERROR_NULL_POINTER;

In general it is not necessary to null-check retvals because JS
could never pass null, and C++ code should know better.  At least,
that is the prevailing theory for new mozilla code.


>+  PRBool isLivemark = PR_FALSE;
>+  rv = IsLivemark(aContainer, &isLivemark);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!isLivemark)
>+    return NS_ERROR_INVALID_ARG;
>+
>+  // Now convert the container ID to a container URI for annotation operations
>+  nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService();
>+  NS_ENSURE_TRUE(bookmarks, NS_ERROR_FAILURE);
>+
>+  nsCOMPtr<nsIURI> containerURI;
>+  rv = bookmarks->GetFolderURI(aContainer, getter_AddRefs(containerURI));
>+  NS_ENSURE_SUCCESS(rv, rv);

Is GetFolderURI fast?  IsLivemark basically does the same lookup for
the folderURI.  Maybe it doesn't matter.


>+  nsCOMPtr<nsIURI> siteURI;
>+  rv = NS_NewURI(getter_AddRefs(siteURI), NS_ConvertUTF16toUTF8(siteURIString));
>+  *aURI = siteURI.get();
>+  NS_IF_ADDREF(*aURI);

So, in the case where the annotation exists but is malformed (i.e., does
not parse as a URI), then you're wanting to just return null and not
throw an exception?  Maybe the IDL should mention this failure case?


sr=darin
Attachment #217224 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 7

12 years ago
Created attachment 217242 [details] [diff] [review]
Patch as landed (tweaked based on Darin's comments)

Made a couple changes you suggested (null checking and throwing error on bad URI in annotation).  I'm leaving the IsLivemark in for now, on the grounds of favoring  legibility and lack of duplication over premature optimization in this case.

Landed on branch & trunk.
Attachment #217224 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED

Comment 8

12 years ago
does a livemark have a reload function? I need to reload a livemark from an extension and was wondering if it's possible
(Assignee)

Comment 9

12 years ago
Yes, Pam just added that, see bug 329634.
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.