History sidebar: page titles are expanded, but URLs remain compressed

RESOLVED FIXED in Firefox 11



Bookmarks & History
6 years ago
6 years ago


(Reporter: jdm, Assigned: prip)


Firefox 11

Firefox Tracking Flags

(Not tracked)


(Whiteboard: [mentor=jdm][lang=js])


(1 attachment, 2 obsolete attachments)



6 years ago
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 www.google.com/.../waef , no matter how wide the sidebar is.

Comment 1

6 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
Component: Places → Bookmarks & History
Product: Toolkit → Firefox
QA Contact: places → bookmarks
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

Comment 3

6 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]

Comment 4

6 years ago
Created attachment 579071 [details] [diff] [review]
v1 - please review :)

Comment 5

6 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!

Comment 6

6 years ago
Created attachment 579355 [details] [diff] [review]
Avoid cutting off Places URI titles unnecessarily.

Thanks a lot Josh!
Attachment #579071 - Attachment is obsolete: true


6 years ago
Attachment #579355 - Flags: review?(mak77)
Assignee: nobody → thomas
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-

Comment 8

6 years ago
Created attachment 579897 [details] [diff] [review]
Avoid cutting off Places URI titles unnecessarily
Attachment #579355 - Attachment is obsolete: true

Comment 9

6 years ago
I've sent this patch to the tryserver, so we'll see if any further tests need fixing.

Comment 10

6 years ago
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 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+

Comment 12

6 years ago

Thanks prip, awesome work this week! :-)
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
You need to log in before you can comment on or make changes to this bug.