livemark feed URIs should show up as "bookmarked" (gold star on) in the url bar and in url bar autocomplete search results

VERIFIED FIXED in Firefox 3 alpha8

Status

()

VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: moco, Assigned: moco)

Tracking

Trunk
Firefox 3 alpha8
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

livebookmark should show up as bookmarked (gold star on) in the url bar and in search results

steps to reproduce

1) type in http://weblogs.mozillazine.org/asa/index.rdf to the url bar
2) bookmark it as a livebookmark
3) type http://weblogs.mozillazine.org/asa/index.rdf again into the url bar

expected results:

1) the star in next to go button is gold
2) the star next to the item in the autocomplete results is gold

actual results:

neither are gold.


see also bug #393887 and bug #394484
did a little debugging, and the problem is that:

in nsNavBookmarks::GetBookmarkIdsForURITArray() we query for all bookmarks, but we look for items with type TYPE_BOOKMARK (1).  livemarks are really folders TYPE_FOLDER (2).  

I think we should leave GetBookmarkIdsForURITArray() alone.

we could either use getItemsWithAnnotation('livemark/feedURI') to find all the items with the 'livemark/feedURI' and then use getItemAnnotation(itemId, 'livemark/feedURI') to see if the annotation is our url.  that seems good for 
PSB_updateState()

for autocomplete, as we would be doing this on every uri that wasn't a bookmark, we need to watch out for perf.

so we could do this query once:

select a.id from moz_anno_attributes a where name = 'livemark/feedURI'

to determine the anno id for livemark

and then for each uri, do:

select item_id from moz_items_annos, moz_bookmarks b where b.id = item_id AND anno_attribute_id = ?1 AND b.type = ?2 and content = ?3 limit 1

where b.type is TYPE_FOLDER.

I'll work on the url bar first, and the url bar autocomplete results second.

(note, I considered a solution using folder_type, but that is going away, see mano's comment in bug #392399)
Assignee: nobody → sspitzer
Created attachment 280022 [details] [diff] [review]
a fix for the url bar case (but not for the url bar autocomplete case)
Attachment #280022 - Flags: review?(dietrich)
Comment on attachment 280022 [details] [diff] [review]
a fix for the url bar case (but not for the url bar autocomplete case)

>+  getMostRecentFolderForFeed:
>+  function PU_getMostRecentFolderForFeed(aFeedSpec) {
>+    var annosvc = this.annotations;
>+    var livemarks = annosvc.getItemsWithAnnotation(LMANNO_FEEDURI, {});
>+    for (var i = 0; i < livemarks.length; i++) {
>+      if (annosvc.getItemAnnotation(livemarks[i], LMANNO_FEEDURI) == aFeedSpec)
>+        return livemarks[i];
>+    }
>+    return -1;
>+  },

we need annotation searches in a bad way. some kind of SPO syntax would be nice (/me hears RDF cackling from it's grave).

is there a reason you passed the spec instead of the URI? passing the URI would be consistent with getMostRecentBookmarkForURI.

r=me otherwise.
Attachment #280022 - Flags: review?(dietrich) → review+
> is there a reason you passed the spec instead of the URI?

in this case, yes, because otherwise I would have done aURI.spec twice, instead of once in the caller.

but maybe I should generalize this guy to be getMostRecentItemForURIByAnnotation(), and pass in aURI and the annotation?

maybe others will need it?
> in this case, yes, because otherwise I would have done aURI.spec twice, instead
> of once in the caller.

sorry, not twice, but either a local variable, or done aURI.spec in the loop.

new patch coming.  decided not to generalize for now.
Status: NEW → ASSIGNED
Created attachment 280027 [details] [diff] [review]
updated patch for url bar, per dietrich
Attachment #280022 - Attachment is obsolete: true
Attachment #280027 - Flags: review+
Created attachment 280028 [details] [diff] [review]
rename to getMostRecentFolderForFeedURI()
Attachment #280027 - Attachment is obsolete: true
Attachment #280028 - Flags: review+
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Firefox 3 M8
can wait for m9
Target Milestone: Firefox 3 M8 → Firefox 3 M9
back to comment #1, a better idea for autocomplete:

1) get all the feeds (by doing select distinct content from moz_items_annos, moz_bookmarks b where b.id = item_id AND anno_attribute_id = 3 AND b.type = 2)
2) save them in a hash, like we do for livemark parents
3) if url in hash, then show as bookmark.

this should be pretty fast as we query once, and the do a constant hash lookup once per url.

patch coming
Created attachment 280034 [details] [diff] [review]
patch for the url autocomplete, case and some perf improvements
Attachment #280034 - Flags: review?(dietrich)
moving back to m8, because of the perf improvements, which are:

1)  don't regenerate the livemark hashes if we are searching the previous results.

+  // if we are searching through our previous results,
+  // we don't need to regenerate these hash tables
+  if (!searchPrevious) {

2) after searching previous results, if we have any matches, tell listener asap.

+    // if we found some results, announce them now instead of waiting
+    // to do the first db search.
Target Milestone: Firefox 3 M9 → Firefox 3 M8
Comment on attachment 280034 [details] [diff] [review]
patch for the url autocomplete, case and some perf improvements

> nsresult
> nsNavHistory::CreateAutoCompleteQueries()
> {
>   nsCString sql = NS_LITERAL_CSTRING(
>     "SELECT annos.item_id FROM moz_anno_attributes attrs "
>-    "JOIN moz_items_annos annos WHERE attrs.name = \"" LMANNO_FEEDURI "\" "
>-    "AND attrs.id = annos.anno_attribute_id;");
>+    "JOIN moz_items_annos annos ON attrs.id = annos.anno_attribute_id "
>+    "WHERE attrs.name = \"" LMANNO_FEEDURI "\";");
>   nsresult rv = mDBConn->CreateStatement(sql, getter_AddRefs(mLivemarkFeedsQuery));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   sql = NS_LITERAL_CSTRING(
>+    "SELECT DISTINCT annos.content FROM moz_anno_attributes attrs "
>+    "JOIN moz_items_annos annos ON attrs.id = annos.anno_attribute_id "
>+    "JOIN moz_bookmarks b ON b.id = annos.item_id "
>+    "WHERE attrs.name = \"" LMANNO_FEEDURI "\" "
>+    "AND b.type = ");
>+  sql.AppendInt(nsNavBookmarks::TYPE_FOLDER);
>+  rv = mDBConn->CreateStatement(sql, getter_AddRefs(mLivemarkFeedURIsQuery));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+

hrm. i think you could merge these queries. you could combine the queries into a single query that selects the item id and the feed URI, and remove DISTINCT. you can make the feed URI list distinct post-query when you insert into the hash. (i bet that a majority of the time the results would be naturally distinct anyways)

also, about the folder type restriction - there's no place in the core code that ever marks a non-folder with the feed URI annotation. maybe i'm not paranoid enough, but it seems overkill to check that in the query. also: if you do include the folder check, it should be in both queries.
Attachment #280034 - Flags: review?(dietrich)
Created attachment 280104 [details] [diff] [review]
one query, per dietrich (thanks!)
Attachment #280034 - Attachment is obsolete: true
Attachment #280104 - Flags: review?(dietrich)
Comment on attachment 280104 [details] [diff] [review]
one query, per dietrich (thanks!)

r=me, thanks
Attachment #280104 - Flags: review?(dietrich) → review+
Comment on attachment 280104 [details] [diff] [review]
one query, per dietrich (thanks!)



>+    mozStorageStatementScoper scope(mLivemarkFeedsQuery);
>+    PRBool hasMore = PR_FALSE;
>+    PRBool dummy;
>+    while (NS_SUCCEEDED(mLivemarkFeedsQuery->ExecuteStep(&hasMore)) && hasMore) {

per irc:

[2:13pm] sspitzerMsgMe: oops, removing the "dummy" declaration
[2:13pm] sspitzerMsgMe: that was from when I was going to check for duplicate uris
[2:14pm] sspitzerMsgMe: locally, I've removed:      PRBool dummy;
Comment on attachment 280028 [details] [diff] [review]
rename to getMostRecentFolderForFeedURI()

seeking approval for this fix for 1.9 (fixes the bug + a perf win.)
Attachment #280028 - Flags: approval1.9?
Comment on attachment 280104 [details] [diff] [review]
one query, per dietrich (thanks!)

seeking approval for the fix (plus the perf win.)
Attachment #280104 - Flags: approval1.9?

Updated

11 years ago
Attachment #280104 - Flags: approval1.9? → approval1.9+

Updated

11 years ago
Attachment #280028 - Flags: approval1.9? → approval1.9+
fixed.

Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHisto
ry.h
new revision: 1.98; previous revision: 1.97
done
Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <
--  nsNavHistoryAutoComplete.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in browser/base/content/browser-places.js;
/cvsroot/mozilla/browser/base/content/browser-places.js,v  <--  browser-places.j
s
new revision: 1.49; previous revision: 1.48
done
Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.63; previous revision: 1.62
done
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Summary: livebookmark should show up as bookmarked (gold star on) in the url bar and in search results → livemark feed URIs should show up as "bookmarked" (gold star on) in the url bar and in url bar autocomplete search results
Flags: in-litmus?
Flags: blocking-firefox3?
note to my friends in QA, when verifying this, keep an eye out for regression to bug #393887.  (I've tested it before checking in, but the fix is to related code.)
Depends on: 393887
(In reply to comment #0)

<snip>
 
> 1) type in http://weblogs.mozillazine.org/asa/index.rdf to the url bar
> 2) bookmark it as a livebookmark
> 3) type http://weblogs.mozillazine.org/asa/index.rdf again into the url bar
> 
> expected results:
> 
> 1) the star in next to go button is gold
> 2) the star next to the item in the autocomplete results is gold

Is 1) really expected?  My understanding is that the "gold star icon" only shows up during page views (once the URL has loaded); if that's intended, this part of the fix isn't working, as even autocompleting the full string "http://weblogs.mozillazine.org/asa/index.rdf" doesn't turn it gold.

Once I press enter, of course, that shows up; was "press enter" implicit in the STR 3)?
> 1) really expected?

Expected?  I'm not sure I follow.  Do you mean, common?

> this part of the fix isn't working

the star should not be gold until you've either:

a)  clicked "subscribe now" and made it a livebookmark
or
b)  clicked on the star in the url bar and bookmarked the raw feed  

(In reply to comment #21)
> > 1) really expected?
> 
> Expected?  I'm not sure I follow.  Do you mean, common?

No, I meant, "really an expected result?"

> > this part of the fix isn't working
> 
> the star should not be gold until you've either:
> 
> a)  clicked "subscribe now" and made it a livebookmark
> or
> b)  clicked on the star in the url bar and bookmarked the raw feed  

I'm really asking if only on autocomplete (with the URL already bookmarked), the gold star should appear, or if it requires the user to press "Enter" ("return" on Macs) to actually *load* the URL.

In your "Steps to Reproduce," above, I assumed that you implied an "Enter" keypress for 1), but didn't want to make that same assumption for 3).

Should I be?  If so--if loading that URL is tantamount to it being re-starred in the URL bar--then this is verified fixed; if not--i.e. you really mean "just trigger autocomplete--then this is NOT fixed.

Sorry for the maximum verbosity; I just want to be absolutely clear when I verify.
stephen, now I get it.  thanks for clarifying.

I left out a lot of steps, I should have written:

0)  create a new profile
1)  type in http://weblogs.mozillazine.org/asa/index.rdf to the url bar and then hit enter.
2)  before you click "subscribe now" the star is not gold.
3)  click "subscribe now" to make it a livebookmark.  now the star is gold in the url bar.
4)  create a new tab, and type "http://weblogs.mozillazine.org/a" into the url bar (but don't type enter)
5)  http://weblogs.mozillazine.org/asa/index.rdf should be in the results, and it should have a gold star.
6)  select http://weblogs.mozillazine.org/asa/index.rdf in the results, hit enter.
7)  once http://weblogs.mozillazine.org/asa/index.rdf has loaded, you should get the gold star.
Seth, thanks!  The explicit callout to steps # 6/7 is what I was after (and assuming, but didn't want to assume when verifying/writing the testcase).

http://litmus.mozilla.org/show_test.cgi?id=4682

in-litmus+

Verified FIXED using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007091204 Minefield/3.0a8pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a8pre) Gecko/2007091204 Minefield/3.0a8pre

and

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007091204 Minefield/3.0a8pre

Sorry for the mixup.
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
Copy and paste from a VM is hard, apparently (meh):

That second Mac user-string should be Windows XP:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007091204 Minefield/3.0a8pre
> Sorry for the mixup.

no mixup on your part.  I keep forgetting that you can't read my mind.  

thanks again for following up and asking for clarification.
Seth, when the star is clicked twice, what's being edited - the livemark or a new item under "All Bookmarks"?

Updated

11 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Depends on: 425520
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.