Livemarks service does not show children with empty titles

VERIFIED FIXED

Status

()

Firefox
Bookmarks & History
--
minor
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Simon Wagner, Assigned: Simon Wagner)

Tracking

({testcase})

Trunk
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.0.1) Gecko/2008072820 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; de; rv:1.9.0.1) Gecko/2008072820 Firefox/3.0.1

If a feed is added to the bookmarks via Dynamic Bookmarks, entries that have an empty title tag (e.g. <title />) won't show up.

Reproducible: Always

Steps to Reproduce:
1. Create a feed file with an empty title tag (example http://judith-in-ecuador.blogspot.com/feeds/posts/default)
2. add this feed to the bookmarks
3. open the bookmark
Actual Results:  
entries with empty title tag won't be shown. However entries with a missing title tag will be shown with the publishing date as title.

Expected Results:  
The entries should be shown nevertheless, maybe with the URI of the link of the entry (as this is also the behaviour if an HTML site with a missing <title> tag is added to the bookmarks).
Showing them with the publishing date is a bit confusing, maybe we should also replace this behaviour with showing the URI.
(Assignee)

Comment 1

9 years ago
Created attachment 336194 [details]
Feed test case

Feed test case with one entry with a title tag <title>, one with empty title tag <title /> and one with a missing title tag (no <title> at all).

Note: You have to use the MIME-Type application/atom+xml to open this test case. Opening this file from your local filesystem will just open the file as an XML file, not as a Feed.
Use netcat to create a local HTTP server and send the file with "Content-Type: application/atom+xml"
(Assignee)

Comment 2

9 years ago
It seems the problem is the following code: http://mxr.mozilla.org/firefox/source/toolkit/components/places/src/nsLivemarkService.js#553 .

I am not a Mozilla Developer and I didn't debug the code, but from what I see, I guess that  entry.title.plainText() will return null if the title tag is empty. Therefore in the next step the continue statement will be executed and the entry will be skipped.

So instead of
let title = entry.title ? entry.title.plainText() : entry.updated;

it should be
let title = (entry.title != null &&  entry.title.plainText() != null) ? entry.title.plainText() : entry.updated;

To show the URI and not the date, you could use the following line:
let title = (entry.title != null &&  entry.title.plainText() != null) ? entry.title.plainText() : href;

As I said, I am not a developer here, so this code could be wrong. I just read the source and thought that these lines would fix the problem, so sorry for any wrong advices :)
(Assignee)

Comment 3

9 years ago
sorry i posted the wrong line, its of course 
http://mxr.mozilla.org/firefox/source/toolkit/components/places/src/nsLivemarkService.js#559
(Assignee)

Updated

9 years ago
Version: unspecified → Trunk
(Assignee)

Comment 4

9 years ago
Created attachment 336664 [details] [diff] [review]
Manual created patch to fix the bug

Patch added, to fix the problem and to change the displayed text to the link URI.
However, the patch is not tested and I even don't know if the format is really correct, because I created it from hand (don't want to install git at the moment ;) ).
Note also, that I think that the URI should be HTML escaped. That must also be done!
Hoping the bug will get fixed now :).
Attachment #336664 - Flags: review?
(Assignee)

Updated

9 years ago
Keywords: helpwanted, testcase
(Assignee)

Updated

9 years ago
Component: Bookmarks & History → Places
Created attachment 336856 [details] [diff] [review]
patch

Thank you!

well i think it makes sense displaying part of the url instead of the updated time for one simple reason: i'd like to know where that item will bring me when clicked.
We are doing the same for bookmarks publishing host+fileName (or path if filename is empty), for this reason we could simply add an empty titled feed since we accept empty titled bookmarks / nodes and getBestTitle util will fix the showed title for us. i'm attaching this kind of change.

Dietrich, we had ui-review+ for bookmarks, do we need a new one for this?
Attachment #336664 - Attachment is obsolete: true
Attachment #336856 - Flags: review?(dietrich)
Attachment #336664 - Flags: review?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → wagner.sim88
Severity: normal → minor
Keywords: helpwanted
Summary: Dynamic Bookmarks don't show entries with empty titles → Livemarks service does not show children with empty titles
Status: NEW → ASSIGNED
Attachment #336856 - Flags: review?(dietrich) → review+
Comment on attachment 336856 [details] [diff] [review]
patch

r=me. ui-r not necessary, since we're just making this consistent w/ pre-existing practice.
(Assignee)

Comment 7

9 years ago
Thanks for taking care of the bug.

But there is something in your patch that I don't understand in your patch:
As I noted above, entry.title.plainText() will return null, if the title tag is empty (with empty I mean <title />). 
That's the reason why the bookmark was thrown away because
if(!title)
   continue;
would throw away the entry.
So your patch would create an entry with a null title.
Is that OK?

Well, even if it's OK, I would still add a second test and replace the null value with an empty string. But that's just my opinion, I don't like null values, expect to indicate an error.
http://hg.mozilla.org/mozilla-central/rev/71479935c4af

(In reply to comment #7)
> So your patch would create an entry with a null title.
> Is that OK?

Yes we have bookmarks without titles, and placesutils will take care of that
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre
Status: RESOLVED → VERIFIED
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
You need to log in before you can comment on or make changes to this bug.