Closed Bug 449406 Opened 12 years ago Closed 11 years ago

bookmarks of TRANSITION_DOWNLOAD and TRANSITION_EMBED don't show up in the location bar if they've been visited

Categories

(Toolkit :: Places, defect, P2, major)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: bzbarsky, Assigned: sdwilsh)

References

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 6 obsolete files)

BUILD: Current trunk

STEPS TO REPRODUCE:
1) Bookmarks > Organize Bookmarks
2) Bring up the context menu on "Unsorted Bookmarks" in the pane on the left
3) Select "New Bookmark..."
4) Enter "Phishing slides" as the name
5) Enter "http://www.cs.auckland.ac.nz/~pgut001/pubs/phishing.pdf" as the
   location
6) Select "Add"
7) Close bookmark organizer
8) Type "phish" in the URL bar

EXPECTED RESULTS: match list includes the bookmark you added, since both the
                  URL and the name match "phish". 

ACTUAL RESULTS: match list doesn't include that bookmark (in fact, is empty in
                my case)

I don't know whether this is due to me manually adding the bookmark, due to the page not being in history (which it can never be anyway), or something else.
WFM, so I suspect "something else."
works for me as well.  did you change any of your url prefs?
I have no idea.  If it's not exposed in the prefs dialog I probably didn't change it, but I'd be happy to give you any info you need (values for specific pref names, output of stuff run in JS console, etc).
Anything changed under the "browser.urlbar." prefbranch?
Oh and "places.frecency." too.
No non-default settings under either of those.
I have this problem too. About half of the search terms that gives results in the bookmarks pane, does not give any results in the location bar. And very often i get more results for the same search term in the bookmarks pane than in the location bar. I can't see any logic behind which terms/bookmarks are working or not.
(In reply to comment #0)
> 5) Enter "http://www.cs.auckland.ac.nz/~pgut001/pubs/phishing.pdf" as the
>    location
Do you, by chance, have this bookmarked elsewhere too?
No.

Though now that you raise that, I tried adding it a second time and ran into bug 471352 again.

But I continue to have this bookmark in my "unsorted bookmarks", and neither "phishing" nor "slides" nor "phishing slides" nor "auckland" finds it.  "auckland" finds other pages on the same site that I visited, but NOT this one page.

I wonder... did you guys try clicking on the link before trying to reproduce?  And make sure that you don't have a PDF plug-in installed?
the entry appear when you add it to bookmarks, but as soon as you visit it its frecency is set to 0, and it won't appear in the locationbar.

try these STRs:
1 - context menu on bookmarks toolbar
2 - New Bookmark...
3 - Enter "Phishing slides" as the name
4 - Enter "http://www.cs.auckland.ac.nz/~pgut001/pubs/phishing.pdf" as location
6 - Select "Add"
7 - Type "phish" in the URL bar
WORKS (frecency = -1)
8 - now click on the bookmark and Cancel the download dialog
9 - Type "phish" in the URL bar
DOES NOT WORK (frecency = 0)
10 - clear history
WORKS (frecency = -1)
bug 412219 is related since the problem is in visit_count used in calculateFrecencyInternal. Download and embed do not add to visit_count and have bonus 0, so their calculated frecency is 0 (visit_count = 0 and bonus = 0).
Plus when calling UpdateFrecency we pass aIsBookmark = PR_FALSE even if it's true.
Blocks: 412219
Marco, would I have gotten the same results if I'd visited the PDF before adding the bookmark?  I'm certain I'd done that!

Requesting blocking, though I'm not quite sure this really should block.
Flags: blocking1.9.1?
Adding a bookmark updates the frecency, but the fact a place is bookmarked is not used to calculate frecency if the place has visits. Indeed the aIsBookmarked param is only used if we don't have visits, if we have visits we exit after sampling a certain number of them, without considering if it is bookmarked. So i suppose the answer to comment 12 is yes, because even bookmarking the site later won't change the frecency from 0.

ideally i think not showing downloads in the locationbar is fine, unless they are not bookmarks. Bookmarked downloads should be visible instead.

The biggest problem about that is that everytime we have to check if an uri is bookmarked we need to filter out livemarks children and that can slowdown us quite a bit (since we also update frecency on add visit).
The clean fix would be to remove livemarks children from bookmarks table (Bug 453530 surely not possible for 3.1), pass a correct isBookmarked param to UpdateFrecency (we only should exclude place: queries then) and in case we have a 0 sample from visits but the uri is bookmarked use unvisited bookmarks bonus.
I guess my issue is that I thought starring a page was supposed to make it show up higher in the awesomebar results, and that in fact that was the whole point of the new bookmark setup.  The first time I actually tried to make use of it, it failed (this bug).
Flags: blocking1.9.1? → blocking1.9.1+
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
Assignee: nobody → sdwilsh
Is this just an issue of a bookmark not getting a big enough of a bonus?

There's currently two bookmark related prefs:

places.frecency.bookmarkVisitBonus
places.frecency.unvisitedBookmarkBonus

But those don't give bonuses for just being a bookmark. The former only triggers if you click the bookmark from the bookmark menu (IIRC) and the latter is only for places that are bookmarked but no visits.

One way is to tweak bookmarkVisitBonus to give a bonus to places that are visited and happen to be bookmarks. Similar to places.frecency.typedVisitBonus.
Attached patch idea v1 (obsolete) — Splinter Review
One idea: After checking the visit type for a bonus, additionally check if it's a bookmark and give a minimum of the bookmark visit amount.
I think it has to do with how we handle TRANSITION_DOWNLOAD actually.
Attached patch idea v1.1 (obsolete) — Splinter Review
A quick refactoring of some "is bookmark" code and this seems to fix the problem of visits of only transition download preventing a page from showing up. Tests are probably broken.

1) I visited the pdf link and nothing shows up when I type "phish".
2) I bookmark "phishing" and it shows up when I type "phish".
3) I change the bookmark url to .blah and .blah shows up but not .pdf.
4) Switch back to Firefox 3, I change bookmark url from .blah to .pdf and no .pdf, but there is a .blah. (Separate bug where we dont recalc frecency on changing uri.. but might already be fixed for 3.1?)
Attachment #358110 - Attachment is obsolete: true
see comment 13.
that's bad for performance since we do such a check on every visit, instead we should really have a precompiled statement that will exactly check if an uri is bookmarked, is not a livemark and is not a query, at least until we will remove livemarks children from bookmarks table, then we could directly use isBookmarked and isQuery.

Then i think is not correct to use mBookmarkVisitBonus, because clearing history and setting unvisitedBookmarksBonus to 0 will still show this even if it's an unvisited bookmark.

Plus we call updateFrecency in other places, especially in nsNavBookmarksService, with the wrong value, so we need a precompiled statement there too for now, and we have to fix all calls or some other action could again reset the frecency to 0 and cause an intermittent view/hide behavior.
Summary: URL bar not finding some bookmarks → bookmarks of TRANSITION_DOWNLOAD and TRANSITION_EMBED don't show up in the location bar if they've been visited
Writing this down for my own sanity:
After auditing every call site of UpdateFrecency, the only call site where it is not called with the right value of isBookmarked is in nsNavHistory::AddVisit.
comment 20 is wrong, for example see bug 455315, probably the same in every place where we remove a bookmark but we don't check if the uri has more than one bookmark (removeFolderChildren?).
Also updateFrecency is an entering point for CalculateFrecencyInternal that has the bug, we should also check other CalculateFrecencyInternal callers.
Attached patch v2.0 (obsolete) — Splinter Review
This is a lot simpler than the previous ideas, and changes things up a bit (and isn't fully correct given comment 21)

Note that this changes behavior -CanAddURI now returns false for query uri's (place:).  I don't see why we would ever want to add a visit for a place uri, and it simplifies some of our logic.

This only ends up doing one query to the database during AddVisit, and uses the bookmark hash table otherwise.
Attachment #358139 - Attachment is obsolete: true
Marco pointed out on irc that this patch isn't right either since this could be the child of a livemark.  I think it might be worthwhile to not even add livemark children to the bookmarks hash, but I need to investigate that more still too.

Also, the next patch needs to address the issue of duplicate bookmarks.
So, if we don't add livemark children to the bookmarks hash, we simplify a number of codepaths.  There is a side effect here though - IsBookmarked will return false for livemark children now.  Dietrich, do you think this is problematic?
Depends on: 412132
Attached patch v3.0 (obsolete) — Splinter Review
Not there yet, but this builds on the work from bug 412132.  I need to make sure I've addressed previous comments, and write some unit tests still.
Attachment #358296 - Attachment is obsolete: true
Things I need to write a test for:
(1) autocomplete returns a download and embed uri that is bookmarked and visited
(2) visits are not added for uris for place:

If anyone can think of something else, please let me know.

(In reply to comment #21)
> Also updateFrecency is an entering point for CalculateFrecencyInternal that has
> the bug, we should also check other CalculateFrecencyInternal callers.
There are two call sites:
(1) UpdateFrecency
(2) CalculateFrecency

CalculateFrecency does a proper bookmark check (which I think we actually want to use our method now and a new patch will have that).
(In reply to comment #26)
> CalculateFrecency does a proper bookmark check (which I think we actually want
> to use our method now and a new patch will have that).
Additional benefit is that we can get rid of mDBGetBookmarkParentsForPlace on nsNavHistory, which will offset the cost of us adding the new statement in nsNavBookmarks.  This may actually speed up frecency calculations.
Attached patch v3.1 (obsolete) — Splinter Review
Not sure if I can get the tests done today, so let's get this code looked at...
Attachment #358485 - Attachment is obsolete: true
Attachment #358504 - Flags: review?(mak77)
Attachment #358504 - Flags: review?(dietrich)
Comment on attachment 358504 [details] [diff] [review]
v3.1

>diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp
>--- a/toolkit/components/places/src/nsNavBookmarks.cpp
>+++ b/toolkit/components/places/src/nsNavBookmarks.cpp
>@@ -360,16 +360,32 @@ nsNavBookmarks::InitStatements()
>   // mDBIsBookmarkedInDatabase
>   // Just select position since it's just an int32 and may be faster.
>   // We don't actually care about the data, just whether there is any.
>   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>       "SELECT position FROM moz_bookmarks WHERE fk = ?1 AND type = ?2"),
>     getter_AddRefs(mDBIsBookmarkedInDatabase));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  // mDBIsRealBookmark
>+  // Checks to make sure a place_id is a bookmark, and isn't a livemark.
>+  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+      "SELECT id "
>+      "FROM moz_bookmarks "
>+      "WHERE fk = ?1 AND "
>+            "type = ?2 AND "
>+            "parent NOT IN ("
>+              "SELECT a.item_id "
>+              "FROM moz_items_annos a "
>+              "JOIN moz_anno_attributes n ON a.anno_attribute_id = n.id "
>+              "WHERE n.name = ?3"
>+            ")"),
>+    getter_AddRefs(mDBIsRealBookmark));
>+  NS_ENSURE_SUCCESS(rv, rv);

add " LIMIT 1"?

>+PRBool
>+nsNavBookmarks::IsRealBookmark(PRInt64 aPlaceId)
>+{
>+  // Fast path is to check the hash table first.  If it is in the hash table,
>+  // then verify that it is a real bookmark.
>+  PRInt64 bookmarkId;
>+  PRBool isBookmark = mBookmarksHash.Get(aPlaceId, &bookmarkId);
>+  if (!isBookmark)
>+    return PR_FALSE;
>+
>+  {
>+    mozStorageStatementScoper scope(mDBIsRealBookmark);
>+
>+    (void)mDBIsRealBookmark->BindInt64Parameter(0, aPlaceId);
>+    (void)mDBIsRealBookmark->BindInt32Parameter(1, TYPE_BOOKMARK);
>+    (void)mDBIsRealBookmark->BindUTF8StringParameter(
>+      2, NS_LITERAL_CSTRING(LMANNO_FEEDURI)
>+    );

nit: check rv. probably paranoid, but is consistent w/ the rest of Places.

>@@ -2755,17 +2749,19 @@ nsNavHistory::AddVisit(nsIURI* aURI, PRT
> 
>   rv = InternalAddVisit(pageID, referringVisitID, aSessionID, aTime,
>                         aTransitionType, aVisitID);
>   transaction.Commit();
> 
>   // Update frecency (*after* the visit info is in the db)
>   // Swallow errors here, since if we've gotten this far, it's more
>   // important to notify the observers below.
>-  (void)UpdateFrecency(pageID, PR_FALSE);
>+  // We cannot get any query URI's here, because CanAddURI would say no.
>+  nsNavBookmarks *bs = nsNavBookmarks::GetBookmarksService();
>+  (void)UpdateFrecency(pageID, bs->IsRealBookmark(pageID));

nit: the comment is not clear. what are you saying, and why there?

patch is looking ok so far.
Attachment #358504 - Flags: review?(dietrich)
Comment on attachment 358504 [details] [diff] [review]
v3.1

something is still wrong with this.  Not ready yet...
Attachment #358504 - Flags: review?(mak77)
In addition to what I've already attached, we need to modify how we calculate frecency too.  I can get my testcase to pass if I make the bonus for TRANSITION_EMBED be:
bonus = aIsBookmarked ? mBookmarkVisitBonus : mEmbedVisitBonus;
and TRANSITION_DOWNLOAD be:
bonus = aIsBookmarked ? mBookmarkVisitBonus : mDownloadVisitBonus;

The problem is that I'm not so sure that's the right thing to do here.  Thoughts?
Status: NEW → ASSIGNED
[11:58] <dietrich> sdwilsh: yes. if the url is bookmarked, it should get the bookmark bonus, and we should ignore it's download/embeddedness
(In reply to comment #29)
> nit: check rv. probably paranoid, but is consistent w/ the rest of Places.
The only reason why I didn't is because this function returns a boolean, so propagating error codes doesn't make sense.  If the binding fails, we'll get nothing returned, so we go to the false case anyway.
(In reply to comment #26)
> Things I need to write a test for:
> (1) autocomplete returns a download and embed uri that is bookmarked and
> visited
> (2) visits are not added for uris for place:
(2) is no longer valid - don't want to block place: uri's from being added to history.
Attached patch v3.2 (obsolete) — Splinter Review
Passes all unit tests, and adds a test to ensure that we don't regress this.
Attachment #358504 - Attachment is obsolete: true
Attachment #358922 - Flags: review?(mak77)
Attachment #358922 - Flags: review?(dietrich)
Whiteboard: [needs review dietrich][needs review MaK77]
Target Milestone: --- → mozilla1.9.2a1
Attachment #358922 - Flags: review?(mak77) → review+
Comment on attachment 358922 [details] [diff] [review]
v3.2

looks good and i like the approach.
I think you should also provide a test to check that if those pages are not bookmarked but visited (embed download) they don't appear, and that if they are bookmarked without visits they appear (unless we already have one test covering that).
this is not going to fix other calls to updateFrecency in nsNavBookmarks isn't it? I mean bug 455315, those fixes should be quite easy now, so maybe you could add them to avoid really minor cases in which the frecency could still be zeroed(). Do you want us to fix that apart to provide a specific test?
Comment on attachment 358922 [details] [diff] [review]
v3.2

>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>@@ -7084,16 +7079,26 @@ nsNavHistory::CalculateFrecencyInternal(
>+      // If this is a bookmark, regardless of the transition type, we always
>+      // need to add the bookmark visit bonus.
>+      if (aIsBookmarked &&
>+          visitType != nsINavHistoryService::TRANSITION_BOOKMARK)
>+        bonus += mBookmarkVisitBonus;
nit: Always add the bookmark visit bonus if we haven't yet for the bookmark.

Or something like that..

Also, do we need to tweak the value of bookmark visit bonus? Now every time you visit a link to a bookmarked page, it'll get 150 more frecency for 250 total. That's more than double the current link bonus.

Then why "penalize" bookmark transitions? Clicking a bookmark gives only 150 frecency while clicking a link to a page that happens to be bookmarked gives 250.

Perhaps just if (aIsBookmarked) bonus += mBookmarkVisitBonus
I agree with Edward, i fear users could complain that their searches are too bounded to bookmarks (only returning bookmarks). maybe the bonus should be 50 now, so it would add a bit, but not too much.
Whiteboard: [needs review dietrich][needs review MaK77] → [has patch][needs review dietrich]
Blocks: 455315
(In reply to comment #37)
> this is not going to fix other calls to updateFrecency in nsNavBookmarks isn't
> it? I mean bug 455315, those fixes should be quite easy now, so maybe you could
> add them to avoid really minor cases in which the frecency could still be
> zeroed(). Do you want us to fix that apart to provide a specific test?

fine for a followup, marked it as such.

(In reply to comment #38)
...
(In reply to comment #39)
...

agreed, just adding the bookmarkVisitBonus would score bookmark transitions too high. a bump of 50 across the board sounds reasonable. it's also consistent with the current bonus difference between linkVisitBonus and bookmarkVisitBonus.
(In reply to comment #36)
> (From update of attachment 358922 [details] [diff] [review])
> looks good and i like the approach.
> I think you should also provide a test to check that if those pages are not
> bookmarked but visited (embed download) they don't appear, and that if they are
> bookmarked without visits they appear (unless we already have one test covering
> that).
If they are bookmarked with no visits, they don't have any transition types, so they wouldn't be treated as download/embed anyway.  Added the other test though.
Comment on attachment 358922 [details] [diff] [review]
v3.2

r=me w/ the changes from irc
Attachment #358922 - Flags: review?(dietrich) → review+
Attached patch v3.3Splinter Review
for checkin
Attachment #358922 - Attachment is obsolete: true
Whiteboard: [has patch][needs review dietrich] → [can land]
http://hg.mozilla.org/mozilla-central/rev/9eaf7cf915b6
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
This cannot land on branch until bug 412132 gets approval (or if it doesn't, I need to cook up a new patch for branch)
OK, I have good news and bad news.  The good news is that steps to reproduce from comment 10 now work.

The bad news is that in my existing profile with my existing bookmark things still fail.  Searching on "phish" or "auckland" finds various other stuff but NOT that bookmark.

Is that expected (in the sense that the patch affects writes to the DB but not reads from it or something)?
(In reply to comment #46)
> The bad news is that in my existing profile with my existing bookmark things
> still fail.  Searching on "phish" or "auckland" finds various other stuff but
> NOT that bookmark.
> 
> Is that expected (in the sense that the patch affects writes to the DB but not
> reads from it or something)?
Once you visit the bookmark again, it should start showing up.  We don't recalculate the frecency until you visit it, so it won't fix existing issues.
AHA!  Verified fixed.  Just visiting the bookmark and canceling the dialog makes it happy.  Thank you for aiding my efforts to educate the folks who thing security dialogs are a good idea!
Status: RESOLVED → VERIFIED
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090130 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090130030519
No idea if we already have a similar test on Litmus.
Flags: in-litmus?
Don't need a litmus test - this is covered by automated tests.
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.