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

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

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

Comment 1

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

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)

Comment 2

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

Attachment: a fix for the url bar case (but not for the url bar autocomplete case)

Comment 3

[Dietrich Ayala (: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.

Comment 4

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

> 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?

Comment 5

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

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

Comment 6

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

Attachment: updated patch for url bar, per dietrich

Comment 7

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

Attachment: rename to getMostRecentFolderForFeedURI()

Comment 8

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

can wait for m9

Comment 9

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

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

Comment 10

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

Attachment: patch for the url autocomplete, case and some perf improvements

Comment 11

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

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.

Comment 12

[Dietrich Ayala (:dietrich)]

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.

Comment 13

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

Attachment: one query, per dietrich (thanks!)

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 280104 [details] [diff] [review]
one query, per dietrich (thanks!)

r=me, thanks

Comment 15

[Dietrich Ayala (:dietrich)]

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 16

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

Comment on attachment 280028 [details] [diff] [review]
rename to getMostRecentFolderForFeedURI()

seeking approval for this fix for 1.9 (fixes the bug + a perf win.)

Comment 17

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

Comment on attachment 280104 [details] [diff] [review]
one query, per dietrich (thanks!)

seeking approval for the fix (plus the perf win.)

Comment 18

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

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

Comment 19

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

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

Comment 20

[Stephen Donner [:stephend] Not actively reading bugmail]

(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)?

Comment 21

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

> 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  

Comment 22

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Comment 23

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

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.

Comment 24

[Stephen Donner [:stephend] Not actively reading bugmail]

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.

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Comment 26

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

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

Comment 27

[Asaf Romano (gone)]

Seth, when the star is clicked twice, what's being edited - the livemark or a new item under "All Bookmarks"?

Comment 28

[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