424216 - displaying filename/path instead of title for (unvisited?) bookmarks

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect, P2)

Description

[Simon Bünzli]

Steps to Reproduce:
1. Enter moz into the address bar

Actual result:
Several cryptic suggestions, such as "/en-US/firefox/about/".

Expected result:
Bookmark titles for the URLs (e.g. "About Us" for the above suggestion).

Requesting blocking, as these results will be the first contact with the new awesomebar for many users.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Ed, any ideas? I can confirm with a fresh profile.

Comment 2

[Ed Lee :Mardak]

We only show bookmark titles if we match it. Should we always prefer to show bookmarks if we have a bookmark title?

Comment 3

[Simon Bünzli]

(In reply to comment #2)
IMO we should show the bookmark title, unless the following three conditions are _all_ true:
1. the bookmark's title doesn't match
2. the history entry's title matches
3. we've got an actual history title to begin with (and not the made up ones we're currently displaying)

Comment 4

[Ed Lee :Mardak]

Attachment: v1

Make sure we use either the bookmark or page title -- prefer the bookmark if we have it.

This patch is after bug 418257 so that dietrich doesn't need to re-r? the patch in that bug.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 311647 [details] [diff] [review]
v1


> 
>-      PRBool useBookmark = PR_FALSE;
>+      // Always prefer the bookmark title unless it's empty
>+      nsAutoString title =
>+        entryBookmarkTitle.IsEmpty() ? entryTitle : entryBookmarkTitle;
>+

doesn't this mean that the page title is never evaluated for match, if there's a non-empty bookmark title?

per simon's comment: if the page title matched, and the bookmark title didn't, we should show the page title.

Comment 6

[Ed Lee :Mardak]

(In reply to comment #5)
> >+      // Always prefer the bookmark title unless it's empty
> >+      nsAutoString title =
> >+        entryBookmarkTitle.IsEmpty() ? entryTitle : entryBookmarkTitle;
> doesn't this mean that the page title is never evaluated for match, if there's
> a non-empty bookmark title?
Right. However, most of the time the bookmark title is the same as the page title. And we don't know if the page title is just the placeholder title that we have for unvisited pages. This avoids running into bug 425056 because we only try matching either the page title or bookmark title -- not both at the same time.

Comment 7

[Ed Lee :Mardak]

SELECT h.url, b.title, h.title
FROM moz_bookmarks b
JOIN moz_places h ON h.id = b.fk
WHERE b.title != h.title AND b.keyword_id IS NULL AND h.frecency != 0

From my own history/bookmarks, I have 5 pages that don't have the same bookmark title as page title.

2 pages have blank titles
data:text/html,... | Zapfino | 
http://courses.ece.uiuc.edu/ece411/ | ECE 411 |

2 pages have redirected titles that aren't useful for me:
https://build.mozilla.org/sendchange.cgi | MozillaTry Buildbot Test Page | Patch uploaded successfully
http://uiuc.edu/ | University of Illinois at Urbana-Champaign | CSL Network Login

1 page is the default page (unvisited bookmark) because it can't be visited
javascript:prompt(...) | Postify | prompt(...)

All those pages I would only want to look at the bookmark title I gave to the page anyway.

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 311647 [details] [diff] [review]
v1

i agree that the intersection of bookmarked-but-page-title-matches-better is likely pretty small, so worth making this fix at that expense. it'll shakeout in the nightlies it's a problem.

Comment 9

[Ed Lee :Mardak]

Checking in toolkit/components/places/src/nsNavHistoryAutoComplete.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryAutoComplete.cpp,v  <--  nsNavHistoryAutoComplete.cpp
new revision: 1.54; previous revision: 1.53
done

Comment 10

[Ed Lee :Mardak]

Attachment: v1.1

As checked in -- moved patch ahead of bug 418257.