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)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 3
People
(Reporter: zeniko, Assigned: Mardak)
References
Details
(Keywords: polish, Whiteboard: [fixes bug 425056])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Updated•17 years ago
|
Summary: filename instead of title displayed for bookmarks → displaying filename/path instead of title for (unvisited?) bookmarks
Comment 1•17 years ago
|
||
Ed, any ideas? I can confirm with a fresh profile.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Target Milestone: --- → Firefox 3
Assignee | ||
Comment 2•17 years ago
|
||
We only show bookmark titles if we match it. Should we always prefer to show bookmarks if we have a bookmark title?
Reporter | ||
Comment 3•17 years ago
|
||
(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)
Updated•17 years ago
|
Assignee: nobody → edilee
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][need dietrich review][fixes bug 425056]
Assignee | ||
Comment 4•17 years ago
|
||
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)
Assignee | ||
Updated•17 years ago
|
Depends on: 418257
Whiteboard: [has patch][need dietrich review][fixes bug 425056] → [has patch][need dietrich review][need bug 418257][fixes bug 425056]
Comment 5•17 years ago
|
||
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.
Assignee | ||
Comment 6•17 years ago
|
||
(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.
Assignee | ||
Comment 7•17 years ago
|
||
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•17 years ago
|
||
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+
Assignee | ||
Comment 9•17 years ago
|
||
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
Assignee | ||
Comment 10•17 years ago
|
||
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.
Description
•