Last Comment Bug 349225 - Added content handlers are not retained by feeds pane
: Added content handlers are not retained by feeds pane
Status: RESOLVED FIXED
: fixed1.8.1, relnote, testcase
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: 2.0 Branch
: All All
: -- major (vote)
: Firefox 2
Assigned To: Mike Connor [:mconnor]
:
Mentors:
: 352339 (view as bug list)
Depends on: 347146 352845
Blocks:
  Show dependency treegraph
 
Reported: 2006-08-18 13:12 PDT by Ryan Flint [:rflint] (ping via IRC for reviews)
Modified: 2008-08-21 02:19 PDT (History)
8 users (show)
mbeltzner: blocking‑firefox2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (396 bytes, text/html)
2006-08-18 13:14 PDT, Ryan Flint [:rflint] (ping via IRC for reviews)
no flags Details
Patch (5.40 KB, patch)
2006-08-21 18:51 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
mconnor: review+
Details | Diff | Review
Fix exception on startup (2.16 KB, patch)
2006-08-25 11:51 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
mconnor: review-
Details | Diff | Review
l10n ja/ja-JP-mac patch (2.87 KB, patch)
2006-08-25 11:52 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
mconnor: review-
Details | Diff | Review
Move individual reader pref definitions to -l10n.js, add try/catch-report (5.49 KB, patch)
2006-08-28 12:36 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
mconnor: review+
l10n: superreview-
Details | Diff | Review
handle gaps better (1.70 KB, patch)
2006-09-11 21:21 PDT, Mike Connor [:mconnor]
gavin.sharp: review+
mtschrep: approval1.8.1+
Details | Diff | Review
trunk port (2.19 KB, patch)
2006-09-16 01:20 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Review

Description Ryan Flint [:rflint] (ping via IRC for reviews) 2006-08-18 13:12:48 PDT
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) 2006-08-18 13:14:21 PDT
Created attachment 234476 [details]
Testcase
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2006-08-18 23:49:17 PDT
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] (remove +bmo to email) 2006-08-21 16:18:49 PDT
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] (remove +bmo to email) 2006-08-21 18:51:47 PDT
Created attachment 234891 [details] [diff] [review]
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] 2006-08-24 12:10:50 PDT
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] (remove +bmo to email) 2006-08-24 16:00:48 PDT
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 2006-08-25 06:30:08 PDT
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] (remove +bmo to email) 2006-08-25 11:51:34 PDT
Created attachment 235441 [details] [diff] [review]
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] (remove +bmo to email) 2006-08-25 11:52:17 PDT
Created attachment 235442 [details] [diff] [review]
l10n ja/ja-JP-mac patch
Comment 10 Mike Connor [:mconnor] 2006-08-25 21:22:14 PDT
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] (remove +bmo to email) 2006-08-28 12:36:40 PDT
Created attachment 235772 [details] [diff] [review]
Move individual reader pref definitions to -l10n.js, add try/catch-report
Comment 12 Mike Connor [:mconnor] 2006-08-28 14:06:19 PDT
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] 2006-08-31 05:09:44 PDT
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] 2006-08-31 05:11:51 PDT
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] 2006-09-11 01:50:28 PDT
taking
Comment 16 Mike Connor [:mconnor] 2006-09-11 21:21:34 PDT
Created attachment 237899 [details] [diff] [review]
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) 2006-09-12 07:34:12 PDT
*** Bug 352339 has been marked as a duplicate of this bug. ***
Comment 18 Mike Connor [:mconnor] 2006-09-12 20:59:20 PDT
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 2006-09-13 10:45:36 PDT
Comment on attachment 237899 [details] [diff] [review]
handle gaps better

a=schrep for 181drivers
Comment 20 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-09-13 16:18:20 PDT
not fixed on trunk, please leave this open for now.
Comment 21 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-09-16 01:20:07 PDT
Created attachment 238752 [details] [diff] [review]
trunk port

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

Note You need to log in before you can comment on or make changes to this bug.