Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 705043 - History sidebar: page titles are expanded, but URLs remain compressed
: History sidebar: page titles are expanded, but URLs remain compressed
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 11
Assigned To: Thomas Prip Vestergaard [:prip]
: Marco Bonardo [::mak]
Depends on:
  Show dependency treegraph
Reported: 2011-11-23 21:02 PST by Josh Matthews [:jdm]
Modified: 2012-02-01 13:57 PST (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 - please review :) (2.73 KB, patch)
2011-12-05 08:11 PST, Thomas Prip Vestergaard [:prip]
no flags Details | Diff | Splinter Review
Avoid cutting off Places URI titles unnecessarily. (2.77 KB, patch)
2011-12-06 10:10 PST, Thomas Prip Vestergaard [:prip]
mak77: review-
Details | Diff | Splinter Review
Avoid cutting off Places URI titles unnecessarily (4.25 KB, patch)
2011-12-07 16:44 PST, Thomas Prip Vestergaard [:prip]
mak77: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2011-11-23 21:02:50 PST
Seen on Aurora as well as nightlies, on mac and windows.

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 , no matter how wide the sidebar is.
Comment 1 Josh Matthews [:jdm] 2011-11-23 21:10:14 PST
I'm pretty sure the relevant code is
Comment 2 Marco Bonardo [::mak] 2011-11-24 02:05:23 PST
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
Comment 3 Josh Matthews [:jdm] 2011-11-24 07:13:35 PST
I'm happy to provide help to anybody who wants to fix this. Comment 2 provides a good solution to do so.
Comment 4 Thomas Prip Vestergaard [:prip] 2011-12-05 08:11:56 PST
Created attachment 579071 [details] [diff] [review]
v1 - please review :)
Comment 5 Josh Matthews [:jdm] 2011-12-05 08:28:56 PST
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!
Comment 6 Thomas Prip Vestergaard [:prip] 2011-12-06 10:10:14 PST
Created attachment 579355 [details] [diff] [review]
Avoid cutting off Places URI titles unnecessarily.

Thanks a lot Josh!
Comment 7 Marco Bonardo [::mak] 2011-12-06 16:35:33 PST
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:

- 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 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 =;
>          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.
Comment 8 Thomas Prip Vestergaard [:prip] 2011-12-07 16:44:42 PST
Created attachment 579897 [details] [diff] [review]
Avoid cutting off Places URI titles unnecessarily
Comment 9 Josh Matthews [:jdm] 2011-12-07 19:26:11 PST
I've sent this patch to the tryserver, so we'll see if any further tests need fixing.
Comment 10 Mozilla RelEng Bot 2011-12-08 00:20:42 PST
Try run for 8257ac288de5 is complete.
Detailed breakdown of the results available here:
Results (out of 55 total builds):
    success: 47
    warnings: 7
    failure: 1
Builds (or logs if builds failed) available at:
Comment 11 Marco Bonardo [::mak] 2011-12-09 05:25:00 PST
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!
Comment 13 Ed Morley [:emorley] 2011-12-10 20:35:07 PST

Thanks prip, awesome work this week! :-)

Note You need to log in before you can comment on or make changes to this bug.