341972 - Live bookmark parsing needs to update livemark/siteURI

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Phil Ringnalda (:philor)]

Feed parsing needs to check the channel/link or feed/link against the current value of livemark/siteURI, and update it if it's different, so that the "Open (feed name)" item won't continue opening whatever random page was used to add the feed in some ancient build, and will pick up changes in the link (say, when a feed is redirected for six months, and then the redirect is shut off, so we'll at least have the siteURI from the last successful parse).

Comment 1

[Robert Sayre]

Phil, I think I new what this bug was when you reported it, but I don't anymore. Can you give me some steps to reproduce?

Comment 2

[Phil Ringnalda (:philor)]

STR:

1. In 1.5 or earlier, go to http://www.mozilla.org/ and add the feed for mozillazine.
2. Use that profile in a current trunk build, and click the 'Open "Whatever Bad Name You Got"' menuitem at the bottom of the livemark folder.

Expected:

Go to http://www.mozillazine.org/

Actual:

Go to http://www.mozilla.org/

The other, more common and long range but harder to STR use case would be to add a livemark at http://www.example.com/myblog/wordpress/, and six months later have the 'Open "My Blog"' menuitem still pointing at that, rather than shifting to what's now in the feed, http://myblog.example.com/. We don't really trust redirects enough to change the feed URL during the three months that it was redirecting, but we can at least trust the site URL in the redirected feed enough to have the new site URL there when our user finally notices that the feed isn't loading because the redirect has been shut off.

Comment 3

[Phil Ringnalda (:philor)]

Dead simple, once bug 353434 lands and handleResult can get it from the result.doc

Comment 4

[Phil Ringnalda (:philor)]

Attachment: Fix v.1

I'm growing increasingly unfond of my method of testing livemarks: for absolutely no good reason, setting the siteURI when it was null to start with apparently takes longer, so I had to set the timeout between when onEndUpdateBatch fires and when it's actually done and can be tested even longer, to not randomly wind up testing before the actual change happened.

But, we need to do this even more than we did when I filed it, since now we're passing in the preview page URI (and thus the feed URI) as the siteURI when you add a livemark without bypassing preview, so the one new livemark feature for Fx3 is "a menuitem which pointlessly sends you back to where you can add the feed as a livemark, except you already did that."

It's a shame that the default livemark is going to lose its link to http://en-US.fxfeeds.mozilla.com/en-US/firefox/livebookmarks/ on the first feed load (and maybe we should just take the HREF out of bookmarks.html), but not enough of a shame to make all other feeds suffer for it.

Comment 5

[Phil Ringnalda (:philor)]

Blocking? rationale in comment 4 - without this or a change to the preview page so it at least passes in the random page where you happened to add the feed, the only user-visible change in livemarks for Fx3 is useless.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 295046 [details] [diff] [review]
Fix v.1

r=me for the code change and added tests. however, why are you removing the old test?

Comment 7

[Phil Ringnalda (:philor)]

That's not "the old test," it's "the dead test" - Mano had r+ to remove it in bug 393924, where he made it not work anymore, but decided to leave it and just take it out of the makefile for some reason, probably guessing correctly that I would need it as a starter to figure out how to do tests that don't depend on observing the annotation service (or, he forgot it when he was checking in). Now that we'll have a couple of working tests, I don't think we need that broken one ghosting around anymore.

Comment 8

[Phil Ringnalda (:philor)]

toolkit/components/places/src/nsLivemarkService.js 1.33
toolkit/components/places/tests/chrome/Makefile.in 1.7
toolkit/components/places/tests/chrome/test_341972a.xul 1.1
toolkit/components/places/tests/chrome/test_341972b.xul 1.1
toolkit/components/places/tests/chrome/test_add_livemark.xul delete

Comment 9

[Phil Ringnalda (:philor)]

Had to disable test_341972b.xul, the one that tests going from no site URI to the one in the feed, because it was timing out on the tinderboxes. No clue why it takes longer (it passes fine for me, though as I said in comment 4 it needed a longer timeout before calling the test function to be sure the site URI updates before the test runs). Luckily, in user terms it only tests the behavior when you added a livemark in Fx 2 from Bookmarks - Organize Bookmarks - File - New Live Bookmark, which was probably very rare, and it doesn't seem to be an actual user-visible problem, just a test problem.

Comment 10

[Phil Ringnalda (:philor)]

Oy. The test passes with what I use, --autorun. It passes with the rather odd pair of --console-level=INFO and --close-when-done but not --autorun. It fails with --autorun plus either or both of --console-level=INFO and --close-when-done (the tinderboxes use all three), by calling lmsvc.getSiteURI(gLivemarkId) before it has actually been set. Thinking about why --autorun --close-when-done would cause setting the siteURI to not yet be done two seconds after onEndUpdateBatch fires is making my head hurt real bad.

Comment 11

[Phil Ringnalda (:philor)]

Attachment: Tests, take 2

Apparently I wasn't quite as done with the dead test as I thought: after much head-bashing trying to figure out why params passed to the JS HTTP server made a difference to how quickly an annotation was set, but only if it wasn't set before, I finally realized that I didn't care, because I was testing the hard way, and since I was looking for a change in an annotation, I should just be listening for a change in an annotation.

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 295603 [details] [diff] [review]
Tests, take 2

yes, much simpler, thanks. r=me.

Comment 13

[Phil Ringnalda (:philor)]

Grr. Checked in, and then once again, second one disabled - despite the fact that I ran it twenty times straight with the same commandline the tinderboxes use without a problem, it timed out on both of the ones that run chrome tests. If anyone has the slightest idea what I can do about that, I'm all ears.

Comment 14

[Gervase Markham [:gerv]]

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