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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Places
P2
major
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: bz, Assigned: sdwilsh)

Tracking

({verified1.9.1})

Trunk
mozilla1.9.2a1
verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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."
(Assignee)

Comment 2

10 years ago
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).

Comment 4

10 years ago
Anything changed under the "browser.urlbar." prefbranch?

Comment 5

10 years ago
Oh and "places.frecency." too.
No non-default settings under either of those.

Comment 7

9 years ago
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.
(Assignee)

Comment 8

9 years ago
(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)

Updated

9 years ago
Assignee: nobody → sdwilsh

Comment 15

9 years ago
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.

Comment 16

9 years ago
Created attachment 358110 [details] [diff] [review]
idea v1

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

Comment 17

9 years ago
I think it has to do with how we handle TRANSITION_DOWNLOAD actually.

Comment 18

9 years ago
Created attachment 358139 [details] [diff] [review]
idea v1.1

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

Updated

9 years ago
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
(Assignee)

Comment 20

9 years ago
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.
(Assignee)

Comment 22

9 years ago
Created attachment 358296 [details] [diff] [review]
v2.0

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

Comment 23

9 years ago
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.
(Assignee)

Comment 24

9 years ago
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?
(Assignee)

Updated

9 years ago
Depends on: 412132
(Assignee)

Comment 25

9 years ago
Created attachment 358485 [details] [diff] [review]
v3.0

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

Comment 26

9 years ago
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).
(Assignee)

Comment 27

9 years ago
(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.
(Assignee)

Comment 28

9 years ago
Created attachment 358504 [details] [diff] [review]
v3.1

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

Comment 30

9 years ago
Comment on attachment 358504 [details] [diff] [review]
v3.1

something is still wrong with this.  Not ready yet...
Attachment #358504 - Flags: review?(mak77)
(Assignee)

Comment 31

9 years ago
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
(Assignee)

Comment 33

9 years ago
(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.
(Assignee)

Comment 34

9 years ago
(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.
(Assignee)

Comment 35

9 years ago
Created attachment 358922 [details] [diff] [review]
v3.2

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

Updated

9 years ago
Whiteboard: [needs review dietrich][needs review MaK77]
Target Milestone: --- → mozilla1.9.2a1

Updated

9 years ago
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 38

9 years ago
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.
(Assignee)

Comment 41

9 years ago
(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+
(Assignee)

Comment 43

9 years ago
Created attachment 359187 [details] [diff] [review]
v3.3

for checkin
Attachment #358922 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Whiteboard: [has patch][needs review dietrich] → [can land]
(Assignee)

Comment 44

9 years ago
http://hg.mozilla.org/mozilla-central/rev/9eaf7cf915b6
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [can land]
(Assignee)

Comment 45

9 years ago
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)?
(Assignee)

Comment 47

9 years ago
(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
Keywords: fixed1.9.1 → verified1.9.1
No idea if we already have a similar test on Litmus.
Flags: in-litmus?
(Assignee)

Comment 52

9 years ago
Don't need a litmus test - this is covered by automated tests.
Flags: in-testsuite+
Flags: in-litmus?
Flags: in-litmus-
Depends on: 476292
You need to log in before you can comment on or make changes to this bug.