Closed
Bug 705043
Opened 13 years ago
Closed 13 years ago
History sidebar: page titles are expanded, but URLs remain compressed
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 11
People
(Reporter: jdm, Assigned: prip)
Details
(Whiteboard: [mentor=jdm][lang=js])
Attachments
(1 file, 2 obsolete files)
4.25 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
Seen on Aurora as well as nightlies, on mac and windows. STR: 1. open history sidebar 2. find list of google searches 3. notice that pages with an actual title will show as much of the title as possible until the edge of the sidebar is reached 4. notice that google searches which just show up as a URL show you unhelpful things like www.google.com/.../waef , no matter how wide the sidebar is.
Reporter | ||
Comment 1•13 years ago
|
||
I'm pretty sure the relevant code is http://mxr.mozilla.org/mozilla-central/source/browser/components/places/src/PlacesUIUtils.jsm#732
Component: General → Places
Product: Firefox → Toolkit
QA Contact: general → places
Updated•13 years ago
|
Component: Places → Bookmarks & History
Product: Toolkit → Firefox
QA Contact: places → bookmarks
Comment 2•13 years ago
|
||
Right, while the "cutting" is still wanted on menuitems and toolbar, it may be avoided on treeviews that can be resized. We may add an optional argument to getBestTitle to disable cutting and use it in this callpoint http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/treeView.js#1342
Reporter | ||
Comment 3•13 years ago
|
||
I'm happy to provide help to anybody who wants to fix this. Comment 2 provides a good solution to do so.
Whiteboard: [mentor=jdm][lang=js]
Assignee | ||
Comment 4•13 years ago
|
||
Reporter | ||
Comment 5•13 years ago
|
||
Comment on attachment 579071 [details] [diff] [review] v1 - please review :) Thank you for the patch! I have a couple style nitpicks, and then it can get a proper review from the code owner :) >Bug 705043 - Please do a review Please give a commit message that describes the change, like "Avoid cutting off Places URI titles unnecessarily." >+ getBestTitle: function PUIU_getBestTitle(aNode,aDoNotCutTitle) { Add a space after the , >+ if(!aDoNotCutTitle) { Add a space before the ( >+ title = host + (fileName ? > (host ? "/" + this.ellipsis + "/" : "") + fileName : > uri.path); Make the next two lines line up with the f in fileName. Thanks again!
Assignee | ||
Comment 6•13 years ago
|
||
Thanks a lot Josh!
Attachment #579071 -
Attachment is obsolete: true
Reporter | ||
Updated•13 years ago
|
Attachment #579355 -
Flags: review?(mak77)
Updated•13 years ago
|
Assignee: nobody → thomas
Status: NEW → ASSIGNED
Comment 7•13 years ago
|
||
Comment on attachment 579355 [details] [diff] [review] Avoid cutting off Places URI titles unnecessarily. Review of attachment 579355 [details] [diff] [review]: ----------------------------------------------------------------- The patch is fine, apart some style nits. The problem to fix though, are tests (thus the r-, since this would fail tests). Searching mxr for getBestTitle I can see 2 tests: http://mxr.mozilla.org/mozilla-central/search?string=getBestTitle - test_treeview_date.xul may be unaffected, it compares only if the title is not empty, so getBestTitle return value should not be checked - browser_views_liveupdate.js instead will likely be affected here http://mxr.mozilla.org/mozilla-central/source/browser/components/places/tests/browser/browser_views_liveupdate.js#339 cellText will use the newly added argument, so you should add a ", true" there. Having a Try server run with this change should confirm all cases have been handled. ::: browser/components/places/content/treeView.js @@ +1338,5 @@ > // Do it here so that callers can still get at the 0 length title > // if they go through the "result" API. > if (PlacesUtils.nodeIsSeparator(node)) > return ""; > + return PlacesUIUtils.getBestTitle(node,true); add whitespace after the comma please ::: browser/components/places/src/PlacesUIUtils.jsm @@ +728,5 @@ > var uri = PlacesUtils._uri(aNode.uri); > var host = uri.host; > var fileName = uri.QueryInterface(Ci.nsIURL).fileName; > // if fileName is empty, use path to distinguish labels > + if (!aDoNotCutTitle) { I find double negations hard to follow and error-prone, may you please invert the if/else so that we have if (aDoNotCutTitle) @@ +734,2 @@ > (host ? "/" + this.ellipsis + "/" : "") + fileName : > uri.path); the 2 last lines should be indented a bit more, they are at a sublevel compared to fileName, not before it.
Attachment #579355 -
Flags: review?(mak77) → review-
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #579355 -
Attachment is obsolete: true
Reporter | ||
Comment 9•13 years ago
|
||
I've sent this patch to the tryserver, so we'll see if any further tests need fixing.
Comment 10•13 years ago
|
||
Try run for 8257ac288de5 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=8257ac288de5 Results (out of 55 total builds): success: 47 warnings: 7 failure: 1 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/josh@joshmatthews.net-8257ac288de5
Comment 11•13 years ago
|
||
Comment on attachment 579897 [details] [diff] [review] Avoid cutting off Places URI titles unnecessarily Review of attachment 579897 [details] [diff] [review]: ----------------------------------------------------------------- Try results look good, thank you!
Attachment #579897 -
Flags: review+
Reporter | ||
Comment 12•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/7983b5d3e8ee
Comment 13•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7983b5d3e8ee Thanks prip, awesome work this week! :-)
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
You need to log in
before you can comment on or make changes to this bug.
Description
•