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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 11

People

(Reporter: jdm, Assigned: prip)

Details

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

Attachments

(1 file, 2 obsolete files)

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.
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
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]
Attached patch v1 - please review :) (obsolete) — Splinter Review
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!
Thanks a lot Josh!
Attachment #579071 - Attachment is obsolete: true
Attachment #579355 - Flags: review?(mak77)
Assignee: nobody → thomas
Status: NEW → ASSIGNED
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-
I've sent this patch to the tryserver, so we'll see if any further tests need fixing.
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 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+
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.

Attachment

General

Created:
Updated:
Size: