349225 - Added content handlers are not retained by feeds pane

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

[Ryan Flint [:rflint] (ping via IRC for reviews)]

STR:
1. Click on the button in the attached testcase and add 5 new content handlers
2. Go to the feeds pref pane and verify that they were added sucessfully
3. Restart Firefox and return to the feeds pane

Results:
No non-default content handlers are shown even though they're still in prefs.js.

Comment 1

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Testcase

Comment 2

[Jeff Walden [:Waldo]]

I lay heavy odds on this being a problem with WebContentConverter.registerContentHandler, but I haven't spent time really reading the code to tell for sure; I'll look more on Monday.

Comment 3

[Jeff Walden [:Waldo]]

This is fallout from bug 347146.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/app/profile/firefox.js&rev=1.152&mark=504-512#500

We added a bunch of feed preferences, but since we didn't give them values, the WebContentConverter.js initialization from preferences short-circuits early (it reads prefs of the form "browser.contenthandlers.types.<###>.<property>" starting from 0 until it hits an error), and since new handlers are added to the first available slot starting from 0 (which for us happens to be 6), they're never reached.

I'm evaluating options, and a patch is forthcoming.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Patch

Hint on finding the infinite loop: nsIPrefBranch.resetBranch() isn't implemented anywhere in the tree.

Once this bug is closed, I'll file a followup to reimplement resetHandlersForType. Given that it's not currently used, I think removing the buggy implementation is a better use of time right now than reimplementing it (although I do believe it should be implemented for v2, a task which I'll probably assign to myself).

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 234891 [details] [diff] [review]
Patch

looks good, but random whitespace cleanup is annoying to filter out doing reviews :P

(you and gavin, such a pair!)

Comment 6

[Jeff Walden [:Waldo]]

So, on application of this to branch I noticed an exception thrown at startup.  On further investigation I think this /might/ happen on trunk, too, for reasons which somehow escaped me during my testing.  (I haven't yet been able to test a fully clean trunk build to determine if this is true.)  Once I figure out what's causing the problems on branch (and maybe trunk, if I screwed up something with the trunk patch), I'll have a branch patch posted.

Comment 7

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060825 Minefield/3.0a1 ID:2006082504 [cairo]

[in error console]
Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIPrefBranch.getComplexValue]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///C:/Program%20Files/Firefox30/components/WebContentConverter.js :: anonymous :: line 525"  data: no]
Source File: file:///C:/Program%20Files/Firefox30/components/WebContentConverter.js
Line: 525

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Fix exception on startup

I *think* the reason I didn't see this during testing was that I left the try/catch in place during testing and removed it afterward.  I definitely removed the try/catch later in my testing cycle, but I could have sworn I removed it *before* a final test.  :-\

The exception occurs because we try to read the first pref that doesn't exist from the string bundle, it doesn't exist, and the exception's thrown.  The try/catch silently swallowed it (bad!), and thus removal of the try/catch caused the exception to stop further processing.

Catching the exception is a solution, but I think it's the wrong one.  We shouldn't have preferences set which only *look* entirely non-bogus, and if localizers want them (and, I note, so far only ja wants them -- <http://lxr.mozilla.org/l10n-mozilla1.8/search?string=er.contentHandlers.types.3>)
they should set the preferences in firefox-l10n.js.  They are already required to get approval for having more than three feed readers, so requiring approval of a firefox-l10n.js change should not be a burden.

I'm posting an l10n patch in a sec to add the necessary lines to ja/ja-JP-mac on trunk.  Assuming this plan meets with approval, someone else is going to have to check it in, because I don't have (or really need) l10n repository access.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: l10n ja/ja-JP-mac patch

Comment 10

[Mike Connor [:mconnor]]

Comment on attachment 235441 [details] [diff] [review]
Fix exception on startup

This isn't necessarily the right fix.  We shouldn't split this across two locations.  It'd be better to put this all in firefox-l10n.js and comment out the last three pref sets for en-US

This means we won't fail if we have 0/1/2 feed readers in a locale, and that we handle this in a single location.

That said, as discussed on IRC, we still need to catch the exception (this exception shouldn't be fatal) and use reportError so it still shows up in console.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Move individual reader pref definitions to -l10n.js, add try/catch-report

Comment 12

[Mike Connor [:mconnor]]

Comment on attachment 235772 [details] [diff] [review]
Move individual reader pref definitions to -l10n.js, add try/catch-report

This looks like a good thing.

Axel?

Comment 13

[Axel Hecht [:Pike]]

It doesn't look good to me, sadly.

We have nothing but manual testing for this right now, and making localizers add stuff to firefox-l10n.js is risky business. They're just too tempted to add any other kind of funkyness in there.

This is yet another bug that shows that localized string prefs are bad for enumerated prefs, something I tried to fix in bug 322962, which got stuck.

I suggest that you just don't use localized prefs but use regular prefs and hardcode falling back to region.properties if there is not user-set pref.
I have no idea on how to do something with type, frankly, as I recall Ben having something against making that default to maybe.feed. Though that may just be the right thing to do for 2.0.

Comment 14

[Axel Hecht [:Pike]]

Comment on attachment 235772 [details] [diff] [review]
Move individual reader pref definitions to -l10n.js, add try/catch-report

This has l10n impact, and we got lots of requests for various numbers of feeds pending. This is really late-l10n, thus sr-. Though I'm not officially a superreviewer, of course.

Comment 15

[Mike Connor [:mconnor]]

taking

Comment 16

[Mike Connor [:mconnor]]

Attachment: handle gaps better

basically, the problem is that we throw at the four entry, which is blank.  We need to handle empty/broken sets gracefully, and keep going until we get to a prefbranch with no children.  There's some cases that this doesn't handle perfectly, but we'll have to implement management for web handlers in general for Fx3.

This should be pretty safe and regression-free.

Comment 17

[Phil Ringnalda (:philor)]

*** Bug 352339 has been marked as a duplicate of this bug. ***

Comment 18

[Mike Connor [:mconnor]]

Comment on attachment 237899 [details] [diff] [review]
handle gaps better

turns out, branch and trunk have forked here, since some of these patches didn't make branch, so we'll have to sort that out next week when we backport the feed tweaks that are branch-only right now

Comment 19

[Mike Schroepfer]

Comment on attachment 237899 [details] [diff] [review]
handle gaps better

a=schrep for 181drivers

Comment 20

[Asaf Romano (gone)]

not fixed on trunk, please leave this open for now.

Comment 21

[Asaf Romano (gone)]

Attachment: trunk port

mozilla/browser/components/feeds/src/WebContentConverter.js 1.16