Closed Bug 349225 Opened 18 years ago Closed 18 years ago

Added content handlers are not retained by feeds pane


(Firefox :: Settings UI, defect)

2.0 Branch
Not set



Firefox 2


(Reporter: rflint, Assigned: mconnor)



(Keywords: fixed1.8.1, relnote, testcase)


(3 files, 4 obsolete files)

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

No non-default content handlers are shown even though they're still in prefs.js.
Flags: blocking-firefox2?
Keywords: testcase
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.
Flags: blocking-firefox2? → blocking-firefox2+
Assignee: nobody → jwalden+bmo
This is fallout from bug 347146.

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.
Depends on: 347146
Attached patch Patch (obsolete) — Splinter Review
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).
Attachment #234891 - Flags: review?(mconnor)
Comment on attachment 234891 [details] [diff] [review]

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

(you and gavin, such a pair!)
Attachment #234891 - Flags: review?(mconnor) → review+
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.
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
Attached patch Fix exception on startup (obsolete) — Splinter Review
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 -- <>)
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.
Attachment #235441 - Flags: superreview?(l10n)
Attachment #235441 - Flags: review?(mconnor)
Attached patch l10n ja/ja-JP-mac patch (obsolete) — Splinter Review
Attachment #235442 - Flags: review?(l10n)
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.
Attachment #235441 - Flags: superreview?(l10n)
Attachment #235441 - Flags: review?(mconnor)
Attachment #235441 - Flags: review-
Attachment #235442 - Flags: review?(l10n) → review-
Whiteboard: [needs new patch]
Attachment #235441 - Attachment is obsolete: true
Attachment #235442 - Attachment is obsolete: true
Attachment #235772 - Flags: superreview?(l10n)
Attachment #235772 - Flags: review?(mconnor)
Whiteboard: [needs new patch]
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.

Attachment #235772 - Flags: review?(mconnor) → review+
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 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 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.
Attachment #235772 - Flags: superreview?(l10n) → superreview-
Assignee: jwalden+bmo → mconnor
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.
Attachment #234891 - Attachment is obsolete: true
Attachment #235772 - Attachment is obsolete: true
Attachment #237899 - Flags: review?(
Whiteboard: [has patch][needs review gavin]
Attachment #237899 - Flags: review?( → review+
*** Bug 352339 has been marked as a duplicate of this bug. ***
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
Attachment #237899 - Flags: approval1.8.1?
Whiteboard: [has patch][needs review gavin] → [has patch][needs approval]
Comment on attachment 237899 [details] [diff] [review]
handle gaps better

a=schrep for 181drivers
Attachment #237899 - Flags: approval1.8.1? → approval1.8.1+
Whiteboard: [has patch][needs approval] → [checkin needed (1.8 branch)]
Closed: 18 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [checkin needed (1.8 branch)]
not fixed on trunk, please leave this open for now.
Resolution: FIXED → ---
Depends on: 352845
Attached patch trunk portSplinter Review
mozilla/browser/components/feeds/src/WebContentConverter.js 1.16
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.