Closed Bug 424216 Opened 17 years ago Closed 17 years ago

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

Categories

(Firefox :: Address Bar, defect, P2)

defect

Tracking

()

RESOLVED FIXED
Firefox 3

People

(Reporter: zeniko, Assigned: Mardak)

References

Details

(Keywords: polish, Whiteboard: [fixes bug 425056])

Attachments

(1 file, 1 obsolete file)

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.
Flags: blocking-firefox3?
Summary: filename instead of title displayed for bookmarks → displaying filename/path instead of title for (unvisited?) bookmarks
Ed, any ideas? I can confirm with a fresh profile.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Target Milestone: --- → Firefox 3
We only show bookmark titles if we match it. Should we always prefer to show bookmarks if we have a bookmark title?
(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)
Assignee: nobody → edilee
Blocks: 425056
Whiteboard: [has patch][need dietrich review][fixes bug 425056]
Attached patch v1 (obsolete) (deleted) — Splinter Review
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.
Attachment #311647 - Flags: review?(dietrich)
Depends on: 418257
Whiteboard: [has patch][need dietrich review][fixes bug 425056] → [has patch][need dietrich review][need bug 418257][fixes bug 425056]
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.
(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.
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 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.
Attachment #311647 - Flags: review?(dietrich) → review+
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
Blocks: 418257
Status: NEW → RESOLVED
Closed: 17 years ago
No longer depends on: 418257
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch][need dietrich review][need bug 418257][fixes bug 425056] → [fixes bug 425056]
Attached patch v1.1 (deleted) — Splinter Review
As checked in -- moved patch ahead of bug 418257.
Attachment #311647 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: