438429 - Meta bug to fix several RSS Summary/Web Page bugs

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect, P4)

Description

[alta88]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008060803 Thunderbird/3.0a2pre XPCOMViewer/0.9.5 ID:2008060803

this WIP patch is to fix the below rss summary/web page view bugs.  i've tested it on dozens of feeds, but not all.

Bug 309696 – No way to open RSS item's webpage via keyboard 
Bug 259018 – "show article summary instead of loading the web page" only affects new items 
Bug 312819 – rss feeds changes from showing article summary into loading web page after 1-2 days 
Bug 325424 – RSS always shows "Article Summary" regardless of Article Summary checkbox in feed properties
Bug 333237 – RSS message pane behavior 
Bug 353302 – RSS summary selections not universal option 

Notes:
1. data for both rss summary and web page access is stored in the message body, allowing toggling, yet body will be smaller.
2. message body size should now be cut in half or more on former web page only feeds, unnecessary double storing.
3. iframe src is loaded only when web page mode is selected.
4. remove inline css, not the way to do it, some comments in Bug 363154 #c28
5. pref added for preferred mode, with View - Show Rss Summary checkbox.
6. pref added for opening an rss item (anything with content-base in header actually) in new window or inline, on threadpane double click/enter, set with Message - Open Rss/News Message radio prefs.
7. an alert is shown when trying to select an unavailable mode for old feed items.

TODO:
1. get per folder pref fz:quickMode from feeds.rdf before using global pref.  ideally, Tb should allow adding folder based properties
and support inheritance from parent folders in a simple mechanism, but that is beyond this scope.
2. localization, .dtd entries.
3. need to figure out how to reset some vars in a new message window if changed in main 3pane.
4. handle html/plain text issue for rss context (Bug 253853).
5. do something with old format items?



Reproducible: Always

Comment 1

[alta88]

Attachment: first cut patch

please advise re review request as there are some \mailnews files touched.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

alta88, I've just given your account editbugs and canconfirm privs, so you can now file bugs as CONFIRMED when you wish.  Additionally, I've re-assigned this bug to you.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 324534 [details] [diff] [review]
first cut patch

r- because of all the commented out stuff.  Just remove those lines; we can always use CVS if need to see them or put them bug.

Standard8 has newly minted Thunderbird-peer status, so I'd suggest you request review on the next revision of this patch from him in order to help him break in his review privs. :)

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Also, I dunno if this is worth doing or not (and if it is, I don't know if it's worth doing now or in another bug later), but e4x seems like it would be cleaner than all that regexp replacement stuff.

Comment 5

[alta88]

(In reply to comment #3)
> (From update of attachment 324534 [details] [diff] [review])
> r- because of all the commented out stuff.  Just remove those lines; we can
> always use CVS if need to see them or put them bug.
> 
> Standard8 has newly minted Thunderbird-peer status, so I'd suggest you request
> review on the next revision of this patch from him in order to help him break
> in his review privs. :)
> 

ok.  yeah, final cut won't have any of that, though easier as a WIP; maybe premature to ask for review at this point.. 

(In reply to comment #4)
> Also, I dunno if this is worth doing or not (and if it is, I don't know if it's
> worth doing now or in another bug later), but e4x seems like it would be
> cleaner than all that regexp replacement stuff.
> 

not sure either, the template consts/replace method works but seems odd.  i'd lean to getting the major stuff in and baked, then follow up if worth it.

Comment 6

[alta88]

Attachment: patch for review

> TODO:
> 1. get per folder pref fz:quickMode from feeds.rdf before using global pref. 

done.  it would sure be nice if Bug 418551 gets in, perhaps we can get rid of pieces of rss .rdf dependence.. 

> 2. localization, .dtd entries.

done.

> 3. need to figure out how to reset some vars in a new message window if changed
> in main 3pane.

done.

> 4. handle html/plain text issue for rss context (Bug 253853).

i'll see what can be done separately from this patch; any patch would be added to that bug.

> 5. do something with old format items?
> 

guidance needed. old format feeds aren't affected, the same lousy behavior as before will continue.  one thing which may be done is to rewrite the body automatically, but this would be a separate bug.

Comment 7

[alta88]

myk, what purpose do these serve?  if none, i'd like to take them out.

X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
X-Mozilla-Keys:

Comment 8

[alta88]

Attachment: patch for review

some cleanup, fixed missing debug function, code/comment formatting.

TODO:
1. nicify and localize old format feed alert.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #7)
> myk, what purpose do these serve?  if none, i'd like to take them out.
> 
> X-Mozilla-Status: 0001
> X-Mozilla-Status2: 00000000
> X-Mozilla-Keys:

Hmm, it's been a while, and my memory is hazy, but I believe they enable the correct read/unread status for new stories.  But perhaps that's only when importing stories into IMAP folders, which Thunderbird doesn't do (although one could set up a message filter to do it, so maybe it matters then).

I think bienvenu might have suggested it to me at one point to solve a problem I was having (which I think was that read/unread problem).  cc:ing him to see if he remembers anything about it.

Comment 10

[David :Bienvenu]

Not IMAP, but rather local folders, which is all TB does currently for RSS feeds. If the .msf file is deleted or rebuilt, we regenerate the unread status (among other things) from the x-mozilla-status headers. Similarly, we save the tags in the x-mozilla-keys, so they can survie the .msf file getting rebuilt.

Comment 11

[alta88]

ah thanks.  i realize Status: is used for things like expunged but not compacted, so that one i get.  it's the other two, which seem unused for rss if looking at the .h defs, but i won't muck with it now unless someone confirms it won't affect offsets or the like into the message body.

Comment 12

[David :Bienvenu]

I think it would be easier if you don't muck with them as long as they're not hurting anything.

Comment 13

[Ben Bucksch (:BenB)]

Re sanitizer, see bug 253853 comment 11.

Comment 14

[alta88]

in implementing separate rendering prefs for mail and rss, it turns out sanitizer  makes it hard to swap between iframe and Summary div in plaintext mode, since iframe is just gone.

i'm beginning to think the whole iframe method is unnecessary.  is there any reason not to do: 
gMessageBrowser.loadURI(contentBase.headerValue, null, null); 
to display the web page for a feed??

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #14)
> i'm beginning to think the whole iframe method is unnecessary.  is there any
> reason not to do: 
> gMessageBrowser.loadURI(contentBase.headerValue, null, null); 
> to display the web page for a feed??

I suggested that when we first integrated Forumzilla into Thunderbird (the iframe was just a hack to make remote content display work when adding feed messages from an extension), but mscott didn't want to do it, although he didn't say why.

I think it's a great idea, and we should do it.

Comment 16

[David Ascher (:davida)]

Happy to see this work moving along.  accepting as wanted-tb3, and a2 as requested.

I think it'd be good to get ui-review on the patch as well (i haven't tried it, but looking at it, there's a fair bit of chrome diffs).  It might be easier to get that by attaching screenshots.

Comment 17

[alta88]

(In reply to comment #15)
> (In reply to comment #14)
> > i'm beginning to think the whole iframe method is unnecessary.  is there any
> > reason not to do: 
> > gMessageBrowser.loadURI(contentBase.headerValue, null, null); 
> > to display the web page for a feed??
> 
> I suggested that when we first integrated Forumzilla into Thunderbird (the
> iframe was just a hack to make remote content display work when adding feed
> messages from an extension), but mscott didn't want to do it, although he
> didn't say why.
> 
> I think it's a great idea, and we should do it.
> 

great, that will make things *much* easier..

Comment 18

[alta88]

(In reply to comment #16)
> Happy to see this work moving along.  accepting as wanted-tb3, and a2 as
> requested.
> 
> I think it'd be good to get ui-review on the patch as well (i haven't tried it,
> but looking at it, there's a fair bit of chrome diffs).  It might be easier to
> get that by attaching screenshots.
> 

thanks.  will do re ui-review, the xul changes are solely for menupopup options though.  

Comment 19

[alta88]

Attachment: reworked patch

not sure who does UI so karsten gets the pick..

Comment 20

[alta88]

Notes:

1. Full HTML, Simple HTML and Plaintext have been added as separate prefs for rss Summary message body (anything with content-base).  This was implemented by switching the prefs before sending to mime cpp processing, which rereads them every message load. Rss Web Page is not included in these options. 
2. Web Page is loadURI now.
3. Backward compatibility: old feed format items which are Summary only will also support viewing/toggling the web page. Items which are old form Summary, yet are chosen to use global View Web Page (rather than the feed's folder pref) will cause a flash of loading the Summary before loading the Page.  This isn't a prior usage option, so a small price. Old items which default to show Web Page do not have a Summary available; an alert is shown if attempting to toggle or otherwise show Summary for these. 
4. Alert is localized in \messenger.properties.
5. Feed items can be moved/copied to any folder and still maintain their rss-ness since this is a property of each message's content-base header and not the folder. quickMode property is gotten from the item's original folder, if available.

ben.bucksch - the sanitizer tags list hasn't been added as a separate pref for rss - easy to do if anyone feels this is necessary...

Comment 21

[Karsten Düsterloh]

Comment on attachment 326119 [details] [diff] [review]
reworked patch

I'm not in the position to decide on Thunderbird UI, thus passing on to Bryan.

But, as a sidenote:

>Index: mailnews.js
>===================================================================
>+// rss message body preferences
>+// 0 - global no, load web page
>+// 1 - global yes, load summary
>+// 2 - use individual folder setting; if no setting default to 1
>+pref("mail.show.rss.summary", 1);
...

All RSS is part of /mail/extensions and thus not available to other /mailnews consumers like SeaMonkey, so all these RSS prefs should IMO go into all-thunderbird.js instead.

Comment 22

[alta88]

Attachment: tweaked to remove touching any \mailnews code

Comment 23

[alta88]

Attachment: FINAL cut patch for review

final cleanup, removal of debugs, string changes, tweaks.

Comment 24

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #23)
> Created an attachment (id=326595) [details]
> FINAL cut patch for review
> 
> final cleanup, removal of debugs, string changes, tweaks.

Could you please attach screenshots of the visual changes?  thanks!

Comment 25

[alta88]

there aren't any visual or layout changes to 3pane or any rss dialog etc. other than the addition of 2 new menubar options (does this count?):

1) View -> Rss Message Body As -> (radio group w/ Original HTML, Simple HTML, Plain Text), separator, (radio group w/ Always show Web Page, Always show Summary, Use Feed Properties Setting)
2) Message -> Open Rss/News Message -> (radio group w/ Web Page in New Window, Summary in New Window, Web Page in Message Pane)

if you need a screenshot of this, let me know.  but that's all there is to it..  (accesskeys have obviously been included.)

Comment 26

[Mark Banner (:standard8)]

Comment on attachment 326595 [details] [diff] [review]
FINAL cut patch for review

The majority of this patch looks like changes to wrapping/whitespace. We don't normally do this "randomly" because it confuses blame when looking at the code later on. I realise you have probably done a lot of work on this, but I think I must ask you to take those changes out, especially as I'm not familiar with this code.

Also could you separate it into two patches maybe? One with the debug to debugRSS function changes, and the other with the actual changes to fix the bugs.

Comment 27

[alta88]

Attachment: per request

i've removed the formatting from files (3) with no code change and reverted to debug, hopefully this will be easier.  i'll submit a separate patch for those and i'd really appreciate if it could go in as the code is already hard enough to work with, doesn't follow cleanup i've seen done around here, and as dmose says above, there's always cvs for tracking..

for some reason all of the .css file is repeated in the diff for only a small change.  the meat of this patch are the new functions at the bottom of mailWindowOverlay.js.

Comment 28

[Mark Banner (:standard8)]

(In reply to comment #27)
> Created an attachment (id=327126) [details]
> per request
> 
> 
> i've removed the formatting from files (3) with no code change and reverted to
> debug, hopefully this will be easier.  i'll submit a separate patch for those
> and i'd really appreciate if it could go in as the code is already hard enough
> to work with, doesn't follow cleanup i've seen done around here, and as dmose
> says above, there's always cvs for tracking..

Dan was on about the fact that we didn't need the removed out-lines, not whitespace/line-length changes. The problem with just whitespace (or line-length) changes is that they do make it very hard to track down where changes where introduced and hence why (especially when looking at blame, see link below), which can be very useful for not regressing bugs. There is also a lot of history on these files.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mail/extensions/newsblog/content/FeedItem.js&rev=1.43

I don't mind some limited tidy up when a bug/patch is touching a particular function, but unless something is completely moving, I won't accept random changes.

> for some reason all of the .css file is repeated in the diff for only a small
> change.  the meat of this patch are the new functions at the bottom of
> mailWindowOverlay.js.
> 
I'm guessing that the line endings have changed from unix style (correct) to DOS style (wrong) and for some reason patch hasn't picked it up correctly.

A diff -w would be useful if that does show the single line changed. As it stands, that part of the patch doesn't apply on my latest trunk build.

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 327126 [details] [diff] [review]
per request

(In reply to comment #0)
> Bug 309696 – No way to open RSS item's webpage via keyboard

I've tested this on mac, and unless the fix is in the css changes (which didn't apply), then this hasn't been fixed.

> Bug 259018 – "show article summary instead of loading the web page" only
> affects new items 
> Bug 325424 – RSS always shows "Article Summary" regardless of Article Summary
> checkbox in feed properties
> Bug 353302 – RSS summary selections not universal option 

So now we have:

1) Tools -> Accounts, select rss account, "By Default, show the article summary instead of loading the web page".
2) View -> RSS Message Body As -> "Always show Web Page", "Always show Summary", "Use Feed Properties Setting".

This is confusing they all seem to be related but their actions don't seem to relate to each other in any way, and I'm not sure what effect the account setting actually has now - it certainly doesn't seem to change anything.

I also don't like the way we now have two menu options with Original HTML/Simple HTML/Plain HTML in them, and that they exist all the time. Please attach a screenshot of the View menu with the "RSS Message Body As" sub-menu being displayed, for Bryan's review.

Comment 30

[alta88]

(In reply to comment #29)
> (From update of attachment 327126 [details] [diff] [review])
> (In reply to comment #0)
> > Bug 309696 – No way to open RSS item's webpage via keyboard
> 
> I've tested this on mac, and unless the fix is in the css changes (which didn't
> apply), then this hasn't been fixed.

yes, without the css piece nothing will work.

> 
> > Bug 259018 – "show article summary instead of loading the web page" only
> > affects new items 
> > Bug 325424 – RSS always shows "Article Summary" regardless of Article Summary
> > checkbox in feed properties
> > Bug 353302 – RSS summary selections not universal option 
> 
> So now we have:
> 
> 1) Tools -> Accounts, select rss account, "By Default, show the article summary
> instead of loading the web page".
> 2) View -> RSS Message Body As -> "Always show Web Page", "Always show
> Summary", "Use Feed Properties Setting".
> 
> This is confusing they all seem to be related but their actions don't seem to
> relate to each other in any way, and I'm not sure what effect the account
> setting actually has now - it certainly doesn't seem to change anything.
> 

it works like this: the Always show.. menuitems apply globally to any and all rss messages post download (as determined by the presence of the content-base header) and override the Subscribe -> Edit feed -> Feed properties dialog folder-specific option.  this is true also regardless of the folder of the message, as it's important properties belong to the message, as organization of messages may be mixed within a folder,  having all of pop/imap, rss, newsgroup, and future types (irc etc).

(i would like to do a follow on enh to add a property atom to subjectCol so visual indication of rss can be styled, or add the type of rss feed in a content-description header, or even a new Type column.)

if Use Feed Properties is selected, then the Feed Properties setting per folder setting is used.  if a message has been moved to, say, local folders, the original folder is in the item's record, and a lookup is done in feeds.rdf for the pref.  this can now be changed, per feed, post download - one of the major enhancements here.

the account settings option is simply to determine whether the Feed Properties 'show article summary' checkbox is ticked, when in the dialog to add a new feed.  has no effect after that on downloaded items.

> I also don't like the way we now have two menu options with Original
> HTML/Simple HTML/Plain HTML in them, and that they exist all the time. Please
> attach a screenshot of the View menu with the "RSS Message Body As" sub-menu
> being displayed, for Bryan's review.
> 

do you have an alternate proposal?  the whole point of Bug 253853 is to provide rss specific options for viewing the message body; i've merely followed the existing convention.  note also, the code is very careful to differentiate when changing those options for both rss/non-rss while viewing a non-rss/rss message, and when switching back and forth between rss/non-rss.  again, i do not assume an rss item lives only in an rss account/folder.  screenshot and css patch fix to follow.

Comment 31

[alta88]

Attachment: sync w/latest messageBody.css, line endings

Comment 32

[alta88]

Attachment: View menu screenshot

Comment 33

[alta88]

Attachment: Message menu screenshot

only other visual change here.  also, imo the rss account option 'By default, show article summary..' may be removed; it was only important when summary/web page was an either/or choice - we now have both and it's very easy to switch, per folder, to what is preferred.

Comment 34

[Magnus Melin [:mkmelin]]

Why don't just add the feed options (and I don't particularly like using "rss", as it can be atom just as well) to the view message body menu when an rss message is showing. 

Also, the "Always" is not always possible as plenty of feeds lack summary. In fact, the items can lack link URL too (at least according to the rss2 spec), but it's not common. 

Comment 35

[Magnus Melin [:mkmelin]]

Forgot to mention: using "RSS/News" to describe feeds is totally confusing. news=nntp

Comment 36

[alta88]

(In reply to comment #34)
> Why don't just add the feed options (and I don't particularly like using "rss",
> as it can be atom just as well) to the view message body menu when an rss
> message is showing. 
> 

ok, if bryan also wants the menuitem hidden when not in that type of message, it's easy to do, and i'll put it in.

> Also, the "Always" is not always possible as plenty of feeds lack summary. In
> fact, the items can lack link URL too (at least according to the rss2 spec),
> but it's not common. 
> 

if there's no summary, title/description is used (current behavior).  it also means 'not web page'.  this is a global option, if it doesn't make sense, the user will choose the right one for that feed.

(In reply to comment #35)
> Forgot to mention: using "RSS/News" to describe feeds is totally confusing.
> news=nntp
> 

how about making an alternative suggestion?  it's just strings..  anyway, imo 'rss' means 'of either rss flavor or atom flavor protocol feed', and not even that much to a non tech user.

Comment 37

[Mark Banner (:standard8)]

(In reply to comment #36)
> (In reply to comment #34)
> > Why don't just add the feed options (and I don't particularly like using "rss",
> > as it can be atom just as well) to the view message body menu when an rss
> > message is showing. 
> > 
> 
> ok, if bryan also wants the menuitem hidden when not in that type of message,
> it's easy to do, and i'll put it in.

You don't necessarily have to just show/hide it on that type of message, you could actually replace what is there, so from the user perspective, its just the same menu option, but in the rss case we have a few extra options.

Bryan should be more available again now so hopefully he'll be able to pick this up again soon and give us some advice.

> (In reply to comment #35)
> > Forgot to mention: using "RSS/News" to describe feeds is totally confusing.
> > news=nntp
> > 
> 
> how about making an alternative suggestion?  it's just strings..  anyway, imo
> 'rss' means 'of either rss flavor or atom flavor protocol feed', and not even
> that much to a non tech user.

I think maybe Feed(s) or something, but again Bryan may have a better idea.

Comment 38

[Magnus Melin [:mkmelin]]

(In reply to comment #36)
> how about making an alternative suggestion?  it's just strings..  anyway, imo
> 'rss' means 'of either rss flavor or atom flavor protocol feed', and not even
> that much to a non tech user.

I meant that the "/News" part was confusing as we're not talking nntp here. But yeah Feeds, "RSS/Atom feed" or something...

Comment 39

[alta88]

(In reply to comment #37)
> I think maybe Feed(s) or something, but again Bryan may have a better idea.
> 

(In reply to comment #38)
> I meant that the "/News" part was confusing as we're not talking nntp here. But
> yeah Feeds, "RSS/Atom feed" or something...
> 

those string choices are harder than the actual code fixes it seems ;)
i would note that in account central, the string is "RSS News and Blogs" and the account folder name defaults to "News and Blogs", which is why i went with that.  i sort of categorize nntp=newsgroup (per a/c) and rss=news.  perhaps a unified revamp of what we call 'RSS' is needed..

agree, i like 'RSS Feeds account' for a/c and RSS Message for specific items elsewhere.  (blogs is so, yesterday..)

Comment 40

[Myk Melez [:myk] [@mykmelez]]

I suggest simply "Feeds".  Despite heavy use of the acronym, I suspect most internet users still don't know what RSS is (nor should they have to).

"Feeds" isn't entirely obvious or non-technical either (Google Reader's welcome page doesn't use either term; it talks about subscribing to websites), but it is used more often than RSS, since most mentions of RSS include the word "feed" as well, while "feed" is used without "RSS" in a number of cases (f.e. on feedburner.com).

Comment 41

[Dan Mosedale (:dmosedale, :dmose)]

Feeds sounds better than the alternatives to me as well.

Comment 42

[alta88]

Attachment: strings and menus changes

ok, menu strings are now:
View -> Feed Message Body As
Message -> Open Feed Message

and they replace the existing non-feed message menuitems only if a folder has server.type == rss or the message is a feed (regardless of its folder).

Comment 43

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 328517 [details] [diff] [review]
strings and menus changes

Looks ok though something about feels a little clunky, I'm fine with it if everyone else is.  I guess all the cleanup work was done for me.

Comment 44

[David Ascher (:davida)]

Has this patch landed?

Comment 45

[Magnus Melin [:mkmelin]]

Nope, it's not reviewed yet. 
I had a (very) quick look through it. 

Some things to fix:
 - you have *a lot* of trailing spaces in the code
 - please line things like
var mimeEncoder = Components.classes["@mozilla.org/messenger/mimeconverter;1"]
                            .getService(Components.interfaces.nsIMimeConverter);

  with like this (with the .classes and .getService) aligned.

As per comment 34, i don't think "Always" should be used in the menus. I suggest you make it (if a feed is selected)

Feed Message Body As > Original HTML
                       Simple HTML
                       Plain text
                       -------------
                       Web Page
                       Summary
                       Default Format

... though changing the three first (HTML and plaintext) doesn't seem to do much? Should they be there at all?

Or is that only for feeds that carry full content (not just summary)? What should I choose to get that?

Comment 46

[Magnus Melin [:mkmelin]]

That's meant to show
  .classes["@mozilla.org/messenger/mimeconverter;1"]
  .getService

Oh, and remove the code you have commented out (out from your own code).

Comment 47

[Ben Bucksch (:BenB)]

Obviously, the HTML options only make a difference for rich HTML content, if that's what you're asking.

Comment 48

[alta88]

(In reply to comment #45)
> Nope, it's not reviewed yet. 

quite honestly, it needs to be said that letting patches languish like this is a great way to alienate contributors.

> I had a (very) quick look through it. 
> 
> Some things to fix:
>  - you have *a lot* of trailing spaces in the code
>  - please line things like
> var mimeEncoder = Components.classes["@mozilla.org/messenger/mimeconverter;1"]
>                            
> .getService(Components.interfaces.nsIMimeConverter);
> 
>   with like this (with the .classes and .getService) aligned.

ok.
> 
> As per comment 34, i don't think "Always" should be used in the menus. I
> suggest you make it (if a feed is selected)
> 
> Feed Message Body As > Original HTML
>                        Simple HTML
>                        Plain text
>                        -------------
>                        Web Page
>                        Summary
>                        Default Format
> 
> ... though changing the three first (HTML and plaintext) doesn't seem to do
> much? Should they be there at all?
> 

ok.
> Or is that only for feeds that carry full content (not just summary)? What
> should I choose to get that?
> 

this part of the patch is only for Summary, since it is constructed and can be considered a message like pop/imap etc.  the patch is updated to not show those menuitems if the feed is showing Web Page.  to handle a Web Page, the solution should quite obviously be a verbatim cut/paste of the code/gui used by Fx for the same type of content handling, and would be a separate bug.

Comment 49

[alta88]

Attachment: address comment #46 items

Comment 50

[Mark Banner (:standard8)]

Comment on attachment 335418 [details] [diff] [review]
address comment #46 items

Sorry for the delay in getting to this.

I saw you updated it recently, just in case you don't already know: we have now moved to a Mercurial repository, so cvs is out of date (although currently the patch still applies). See http://developer.mozilla.org/en/docs/comm-central for more information.


>Index: extensions/newsblog/content/FeedItem.js
>===================================================================
>RCS file: /cvsroot/mozilla/mail/extensions/newsblog/content/FeedItem.js,v
>retrieving revision 1.43
>diff -u -r1.43 FeedItem.js
>--- extensions/newsblog/content/FeedItem.js	30 May 2008 13:33:49 -0000	1.43
>+++ extensions/newsblog/content/FeedItem.js	25 Aug 2008 20:21:09 -0000
>@@ -49,71 +49,35 @@
> <html>\n\
>   <head>\n\
>     <title>%TITLE%</title>\n\
>-    <base href=\"%BASE%\">\n\
>-    <style type=\"text/css\">\n\
>-      %STYLE%\n\
>-    </style>\n\
>+    <base href=\"%BASE%\" >\n\

nit: No space before the > please (same in the other places just below it).

>-      // if the RDF was bogus, do nothing. rethrow if it's some other problem
>-      if(!((e instanceof Components.interfaces.nsIXPCException)
>-      && (e.result==Components.results.NS_ERROR_NO_INTERFACE)))
>+      // If the RDF was bogus, do nothing. Rethrow if it's some other problem
>+      if(!((e instanceof Components.interfaces.nsIXPCException) &&
>+          (e.result==Components.results.NS_ERROR_NO_INTERFACE)))

As you're touching this, there should be a space after the if, the (e should be aligned with the second bracket after the !

>Index: locales/en-US/chrome/messenger/messenger.properties
>===================================================================
>RCS file: /cvsroot/mozilla/mail/locales/en-US/chrome/messenger/messenger.properties,v
>retrieving revision 1.59

>+feedNoSummaryAlert=Old format feeds not stored with Summary do not have it available.

I'm not quite sure about the wording of this at the moment, but I can't think of anything better either.

>Index: base/content/mailWindowOverlay.xul
>===================================================================
>RCS file: /cvsroot/mozilla/mail/base/content/mailWindowOverlay.xul,v
>retrieving revision 1.253
>diff -u -r1.253 mailWindowOverlay.xul
>--- base/content/mailWindowOverlay.xul	8 Jul 2008 20:55:22 -0000	1.253
>+++ base/content/mailWindowOverlay.xul	25 Aug 2008 20:21:08 -0000
>@@ -1160,15 +1160,71 @@
>+          <menu id="viewBodyMenu" 
>+                accesskey="&bodyMenu.accesskey;" label="&bodyMenu.label;">
>+            <menupopup id="viewBodyPopMenu" 
>+                       onpopupshowing="InitViewBodyMenu()">
>+              <menuitem id="bodyAllowHTML" 
>+                        type="radio" 
>+                        name="bodyPlaintextVsHTMLPref" 
>+                        label="&bodyAllowHTML.label;"
>+                        accesskey="&bodyAllowHTML.accesskey;" 
>+                        oncommand="MsgBodyRenderPrefs(false, 0, 0)"/>

Please don't do these one-per line. Please do them multiple per line, keeping to a reasonable line length (e.g. 80 chars).

Also a lot of these lines have unnecessary white space at the end of them.

>@@ -187,6 +189,33 @@
>   // ... and also the separator
>   document.getElementById("viewMenuAfterTaskbarSeparator").hidden = !viewsToolbarButton;
> 
>+  // Initialize the Message Body menuitem
>+  var viewBodyMenu = document.getElementById('viewBodyMenu');
>+  if (viewBodyMenu)
>+    viewBodyMenu.setAttribute("hidden", isFeed);

It is better to remove the attribute if you are no longer wanting it to be hidden.

>+
>+  // Initialize the Show Feed Summary menu
>+  var viewFeedSummary = document.getElementById('viewFeedSummary');
>+  if (viewFeedSummary) {
>+    if (document.documentElement.getAttribute('windowtype') != "mail:3pane")
>+      viewFeedSummary.setAttribute("hidden", !gShowFeedSummary);
>+    else
>+      viewFeedSummary.setAttribute("hidden", !isFeed);
>+  }

The same applies here and elsewhere in this patch.

>+  var viewRssMenuItemIds = ["bodyFeedGlobalWebPage",
>+                            "bodyFeedGlobalSummary",
>+                            "bodyFeedPerFolderPref"];
>+  var checked = gPrefBranch.getIntPref("rss.show.summary");
>+  var viewRssItemToCheck = document.getElementById(viewRssMenuItemIds[checked]);
>+  if (viewRssItemToCheck)
>+    viewRssItemToCheck.setAttribute("checked", true);
>+  if (document.documentElement.getAttribute('windowtype') != "mail:3pane") {
>+    document.getElementById("viewFeedSummarySeparator").setAttribute("hidden", true);
>+    document.getElementById("bodyFeedGlobalWebPage").setAttribute("hidden", true);
>+    document.getElementById("bodyFeedGlobalSummary").setAttribute("hidden", true);
>+    document.getElementById("bodyFeedPerFolderPref").setAttribute("hidden", true);
>+  }

Please add a blank line before the second if statement to help readability.

>+  // Initialize the Open Message menuitem
>+  var openMessageMenu = document.getElementById('openMessageWindowMenuitem');
>+  if(document.documentElement.getAttribute('windowtype') == "mail:3pane")
>+    if (openMessageMenu)
>+      openMessageMenu.setAttribute("hidden", isFeed);

All these if statements should have a blank space after the if.

Why are you not checking openMessageMenu first, or even combining these into one if statement?

>+
>+  // Initialize the Open Feed Message handler menu
>+  var openRssMenuPopup = document.getElementById("menu_openFeedMessage");
>+  var openRssMenu = document.getElementById("openFeedMessage");
>+  if(openRssMenuPopup)
>+    openRssMenuPopup.childNodes[gHandleContentBaseMessage]
>+                    .setAttribute("checked", true);
>+  if(openRssMenu)
>+    openRssMenu.setAttribute("hidden", !isFeed);
>+  if(document.documentElement.getAttribute('windowtype') != "mail:3pane")

This is the second time you've got this in a matter of lines, why not cache it?

>@@ -458,6 +524,16 @@
>   return (/^imap-message:/.test(messageUri));
> }
> 
>+function IsFeedMessageOrFolder()
>+{
>+  if (gMsgFolderSelected)
>+    if (gMsgFolderSelected.server.type == 'rss')
>+      return true;
>+  if (GetFirstSelectedMessage() && 'content-base' in currentHeaderData)
>+    return true;
>+  return false;
>+}

This function doesn't return what its name implies. Maybe just IsFeedItem() would be better.

> // passing in the view, so this will work for search and the thread pane
> function MsgOpenSelectedMessages()
> {
>+  // Toggle message body (rss summary) and content-base url in message
>+  // pane per pref, otherwise open summary or web page in new window.
>+  if('content-base' in currentHeaderData && gHandleContentBaseMessage == 2) {

Space after the if please

>@@ -1936,32 +2019,68 @@
>     return true;
> }
> 
>-function MsgBodyAllowHTML()
>+//Global var array for msgbody render prefs feed/non feed,
>+//[0] false = default non feed mode, true = feed mode, 3 prefs follow
>+var gMsgBodyRenderPrefs = [false,
>+                           gPrefBranch.getBoolPref("mailnews.display.prefer_plaintext"),
>+                           gPrefBranch.getIntPref("mailnews.display.html_as"),
>+                           gPrefBranch.getIntPref("mailnews.display.disallow_mime_handlers")]
>+

This code, and the general preference setting code seems inconsistent and difficult. I also don't like the way you are attempting to re-use the mailnews.* prefs - for starters if I set reading email to one mode, go onto RSS feeds, set them to a different mode and then quit, when I go back into it and load my email, it shows it in the wrong mode.

I think the solution would be to make changes in libmime, I'm not sure the best way to go about that, I'm hoping David (Bienvenu) who is cc'ed on this bug might be able to help.

>+//How to load message with content-base url on enter in threadpane
>+var gHandleContentBaseMessage = gPrefBranch.getIntPref("rss.show.content-base");

I don't think this needs to be a global variable, there's only a couple of times you use it.

>+//Current state: load web page if 0, show summary if 1
>+var gShowFeedSummary;
>+var gShowFeedSummaryToggle = false;

Again, I think these could be formed into functions.

>@@ -2895,3 +3018,175 @@
>   else
>     window.openDialog(chromeURL, "", "chrome,resizable,status,centerscreen,dialog=no", args);
> }
>+
>+// Set prefs before mime processing
>+function SetMsgBodyContentMode()
>+{
>+  var contentBase = currentHeaderData["content-base"];
>+
>+  // Not an rss message
>+  if(!contentBase) {

Why not use isFeed() here?

>+    // Now in a non rss mode message; if flag was set for rss mode need
>+    // to set the message body view prefs back to default. Otherwise
>+    // not changing modes, so just return without resetting prefs.
>+    if (!gMsgBodyRenderPrefs[0] == false) {

This should just be if (gMsgBodyRenderPrefs[0])

>+// Switch between message body (feed summary) and content-base url in
>+// the Message Pane, called in MsgOpenSelectedMessages
>+function FeedSetContentViewToggle()
>+{
>+  gShowFeedSummaryToggle = true;
>+  var val = gShowFeedSummary ? 0 : 1;
>+  FeedSetContentView(val);

Drop the intermediate variable val please.

>+// Check message format
>+function FeedCheckContentFormat()
>+{
>+  var contentBase = currentHeaderData["content-base"];
>+  var contentWindowDoc = window.top._content.document;
>+
>+  // Not an rss message
>+  if (!contentBase)
>+    return false;
>+
>+  // For pre Tb3 format..
>+  var rssIframe = contentWindowDoc.getElementById('_mailrssiframe');
>+  if (rssIframe) {
>+    if (gShowFeedSummaryToggle ||
>+        pref.getIntPref("rss.show.summary") == 1) {
>+      alert(gMessengerBundle.getString("feedNoSummaryAlert"));

You should not be using alert. Please use the prompt service. See http://mxr.mozilla.org/comm-central/search?string=nsIPromptService for many examples.

Alternately, how about just displaying the message in the message pane (like we do when in offline mode for imap/news messages when they haven't been downloaded for offline use)? That way we can avoid an annoying prompt.

>+      gShowFeedSummaryToggle = false;
>+    }
>+    return false;
>+  }
>+  else
>+    return true;

You don't need the else here, as the previous part of the if will always return.

>+}
>\ No newline at end of file

Please fix this.

>Index: app/profile/all-thunderbird.js

>+//pref("rss.display.html_sanitizer.allowed_tags", "html head title body p br div(lang,title) h1 h2 h3 h4 h5 h6 ul(type,compact) ol(type,compact,start) li(type,value) dl dt dd blockquote(type,cite) pre noscript noframes strong em sub sup span(lang,title) acronym(title) abbr(title) del(title,cite,datetime) ins(title,cite,datetime) q(cite) a(href,name,title) img(alt,title,longdesc,src) base(href) area(alt) applet(alt) object(alt) var samp dfn address kbd code cite s strike tt b i table(align) caption tr(align,valign) td(rowspan,colspan,align,valign) th(rowspan,colspan,align,valign)");

Why is this pref commented out? Is it used? If not, please don't include it.

Comment 51

[Magnus Melin [:mkmelin]]

(In reply to comment #50)
> >+  // Initialize the Message Body menuitem
> >+  var viewBodyMenu = document.getElementById('viewBodyMenu');
> >+  if (viewBodyMenu)
> >+    viewBodyMenu.setAttribute("hidden", isFeed);
> 
> It is better to remove the attribute if you are no longer wanting it to be
> hidden.

Though IIRC i read using "viewBodyMenu.hidden = "directly is even better.

Comment 52

[Ben Bucksch (:BenB)]

> feedNoSummaryAlert=
> Old format feeds not stored with Summary do not have it available.

"This is a news feed in an old format, which does not store the Summary of the news item. Therefore it cannot be displayed."
?

Comment 53

[David :Bienvenu]

Since Ben is cc'ed, I'll ask him what he thinks of changing the libmime sanitization stuff as follows - instead of having the rss code poke the mailnews prefs and then restore them, either make libmime look at the message hdr getting loaded to see if it's an rss or mail message, and look at the appropriate pref then. Or, since I'd rather core libmime not know about rss, we could change something early on in the process, on the very edge of libmime, like bridge_new_new_uri, to look at the uri, look at the appropriate prefs, and then set some member vars on mime_stream_data->options that libmime can check later instead of checking the prefs in the middle of libmime. We already do a lot of stuff like that in bridge_new_new_uri.

Comment 54

[alta88]

the minor items are no issue, but i will be traveling for 3 weeks and won't get to them for a while.  as for the message body render pref stuff, yes it was specifically done that way to avoid touching the libmime code.  if there's a better method, it should be used, and would be better implemented by someone who's more familiar with that cpp code..  also, a decision should be made one way or another whether the allowed_tags pref should exist for rss, i can see the value for some people but without a gui, that might be very few people.

Comment 55

[Mark Banner (:standard8)]

David (Bienvenu), could you file a bug on the libmime changes and/or poke Ben to see what he thinks.

Comment 56

[Ben Bucksch (:BenB)]

- oncommand="MsgBodyAllowHTML()"/>
+ oncommand="MsgBodyRenderPrefs(false, 0, 0)"/>

Please leave the old system. I think there should be as little logic in the XUL file as possible, that's why I have one function per menu item and then do the definition of what it means, which flags it sets, in the JS file.

I think you add a new menu item for the RSS case anyways, and hide the "Message Body" menu. So, please live that code unchanged.

I agree with bienvenu that it would be better to have independent prefs for RSS and mail. That's why the sanitizer takes the prefs as input params and doesn't read prefs itself, so that the caller can decide and different uses can have different prefs. What I did not expect and only learn now was that RSS news items are stored and process as RFC822 msgs and passed through libmime (I don't think that's a good diea, for the record). That's the reason why they current share the same pref and what you want to change. I agree with bienvenu that the best approach would be to split out the pref reading code in libmime, not the UI. Then you can have two sets of menu items, one for mail and one for RSS, as in your patch, but unlike your patch, they are independent. (There would be a small bit of code duplication, but I would not care too much about it.)

Comment 57

[Ben Bucksch (:BenB)]

FYI, when I say "sanitizer" above, I mean the code in content/. libmime is merely a caller of it.

Comment 58

[David :Bienvenu]

So I think what I suggested in #53 as to the placement of the pref reading code is OK with Ben.

Comment 59

[alta88]

(In reply to comment #50)
> (From update of attachment 335418 [details] [diff] [review])
> Sorry for the delay in getting to this.
> 
> I saw you updated it recently, just in case you don't already know: we have now
> moved to a Mercurial repository, so cvs is out of date (although currently the
> patch still applies). See http://developer.mozilla.org/en/docs/comm-central for
> more information.
> 

yes, my dev environment is now moved over to hg.
> 
> >Index: extensions/newsblog/content/FeedItem.js
> >===================================================================
> >RCS file: /cvsroot/mozilla/mail/extensions/newsblog/content/FeedItem.js,v
> >retrieving revision 1.43
> >diff -u -r1.43 FeedItem.js
> >--- extensions/newsblog/content/FeedItem.js	30 May 2008 13:33:49 -0000	1.43
> >+++ extensions/newsblog/content/FeedItem.js	25 Aug 2008 20:21:09 -0000
> >@@ -49,71 +49,35 @@
> > <html>\n\
> >   <head>\n\
> >     <title>%TITLE%</title>\n\
> >-    <base href=\"%BASE%\">\n\
> >-    <style type=\"text/css\">\n\
> >-      %STYLE%\n\
> >-    </style>\n\
> >+    <base href=\"%BASE%\" >\n\
> 
> nit: No space before the > please (same in the other places just below it).
> 

ok.
> >-      // if the RDF was bogus, do nothing. rethrow if it's some other problem
> >-      if(!((e instanceof Components.interfaces.nsIXPCException)
> >-      && (e.result==Components.results.NS_ERROR_NO_INTERFACE)))
> >+      // If the RDF was bogus, do nothing. Rethrow if it's some other problem
> >+      if(!((e instanceof Components.interfaces.nsIXPCException) &&
> >+          (e.result==Components.results.NS_ERROR_NO_INTERFACE)))
> 
> As you're touching this, there should be a space after the if, the (e should be
> aligned with the second bracket after the !
> 

ok.
> >Index: locales/en-US/chrome/messenger/messenger.properties
> >===================================================================
> >RCS file: /cvsroot/mozilla/mail/locales/en-US/chrome/messenger/messenger.properties,v
> >retrieving revision 1.59
> 
> >+feedNoSummaryAlert=Old format feeds not stored with Summary do not have it available.
> 
> I'm not quite sure about the wording of this at the moment, but I can't think
> of anything better either.
> 

wording per c52.
> >Index: base/content/mailWindowOverlay.xul
> >===================================================================
> >RCS file: /cvsroot/mozilla/mail/base/content/mailWindowOverlay.xul,v
> >retrieving revision 1.253
> >diff -u -r1.253 mailWindowOverlay.xul
> >--- base/content/mailWindowOverlay.xul	8 Jul 2008 20:55:22 -0000	1.253
> >+++ base/content/mailWindowOverlay.xul	25 Aug 2008 20:21:08 -0000
> >@@ -1160,15 +1160,71 @@
> >+          <menu id="viewBodyMenu" 
> >+                accesskey="&bodyMenu.accesskey;" label="&bodyMenu.label;">
> >+            <menupopup id="viewBodyPopMenu" 
> >+                       onpopupshowing="InitViewBodyMenu()">
> >+              <menuitem id="bodyAllowHTML" 
> >+                        type="radio" 
> >+                        name="bodyPlaintextVsHTMLPref" 
> >+                        label="&bodyAllowHTML.label;"
> >+                        accesskey="&bodyAllowHTML.accesskey;" 
> >+                        oncommand="MsgBodyRenderPrefs(false, 0, 0)"/>
> 
> Please don't do these one-per line. Please do them multiple per line, keeping
> to a reasonable line length (e.g. 80 chars).
> 

i'm sure you don't want working on this code to be uncomfortable for people, so
this should be reconsidered.  half the xul is 1 line the other isn't.  what's the policy, project (mozilla) wide, there surely must be one on this and braces and whitespace etc etc.  what's worse, there isn't even current consistency; a diff on the latest changeset to this file shows someone checked in a change to do nothing more than change many attrs per line to 1 per.
> Also a lot of these lines have unnecessary white space at the end of them.
> 

ok for my lines.  the whole file is full of this though.
> >@@ -187,6 +189,33 @@
> >   // ... and also the separator
> >   document.getElementById("viewMenuAfterTaskbarSeparator").hidden = !viewsToolbarButton;
> > 
> >+  // Initialize the Message Body menuitem
> >+  var viewBodyMenu = document.getElementById('viewBodyMenu');
> >+  if (viewBodyMenu)
> >+    viewBodyMenu.setAttribute("hidden", isFeed);
> 
> It is better to remove the attribute if you are no longer wanting it to be
> hidden.
> 

ok, per c51.
> >+
> >+  // Initialize the Show Feed Summary menu
> >+  var viewFeedSummary = document.getElementById('viewFeedSummary');
> >+  if (viewFeedSummary) {
> >+    if (document.documentElement.getAttribute('windowtype') != "mail:3pane")
> >+      viewFeedSummary.setAttribute("hidden", !gShowFeedSummary);
> >+    else
> >+      viewFeedSummary.setAttribute("hidden", !isFeed);
> >+  }
> 
> The same applies here and elsewhere in this patch.
> 

ok.
> >+  var viewRssMenuItemIds = ["bodyFeedGlobalWebPage",
> >+                            "bodyFeedGlobalSummary",
> >+                            "bodyFeedPerFolderPref"];
> >+  var checked = gPrefBranch.getIntPref("rss.show.summary");
> >+  var viewRssItemToCheck = document.getElementById(viewRssMenuItemIds[checked]);
> >+  if (viewRssItemToCheck)
> >+    viewRssItemToCheck.setAttribute("checked", true);
> >+  if (document.documentElement.getAttribute('windowtype') != "mail:3pane") {
> >+    document.getElementById("viewFeedSummarySeparator").setAttribute("hidden", true);
> >+    document.getElementById("bodyFeedGlobalWebPage").setAttribute("hidden", true);
> >+    document.getElementById("bodyFeedGlobalSummary").setAttribute("hidden", true);
> >+    document.getElementById("bodyFeedPerFolderPref").setAttribute("hidden", true);
> >+  }
> 
> Please add a blank line before the second if statement to help readability.
> 

ok.
> >+  // Initialize the Open Message menuitem
> >+  var openMessageMenu = document.getElementById('openMessageWindowMenuitem');
> >+  if(document.documentElement.getAttribute('windowtype') == "mail:3pane")
> >+    if (openMessageMenu)
> >+      openMessageMenu.setAttribute("hidden", isFeed);
> 
> All these if statements should have a blank space after the if.
> 
> Why are you not checking openMessageMenu first, or even combining these into
> one if statement?
> 

ok.
> >+
> >+  // Initialize the Open Feed Message handler menu
> >+  var openRssMenuPopup = document.getElementById("menu_openFeedMessage");
> >+  var openRssMenu = document.getElementById("openFeedMessage");
> >+  if(openRssMenuPopup)
> >+    openRssMenuPopup.childNodes[gHandleContentBaseMessage]
> >+                    .setAttribute("checked", true);
> >+  if(openRssMenu)
> >+    openRssMenu.setAttribute("hidden", !isFeed);
> >+  if(document.documentElement.getAttribute('windowtype') != "mail:3pane")
> 
> This is the second time you've got this in a matter of lines, why not cache it?
> 

ok.
> >@@ -458,6 +524,16 @@
> >   return (/^imap-message:/.test(messageUri));
> > }
> > 
> >+function IsFeedMessageOrFolder()
> >+{
> >+  if (gMsgFolderSelected)
> >+    if (gMsgFolderSelected.server.type == 'rss')
> >+      return true;
> >+  if (GetFirstSelectedMessage() && 'content-base' in currentHeaderData)
> >+    return true;
> >+  return false;
> >+}
> 
> This function doesn't return what its name implies. Maybe just IsFeedItem()
> would be better.
> 

ok.
> > // passing in the view, so this will work for search and the thread pane
> > function MsgOpenSelectedMessages()
> > {
> >+  // Toggle message body (rss summary) and content-base url in message
> >+  // pane per pref, otherwise open summary or web page in new window.
> >+  if('content-base' in currentHeaderData && gHandleContentBaseMessage == 2) {
> 
> Space after the if please
> 

ok.
> >@@ -1936,32 +2019,68 @@
> >     return true;
> > }
> > 
> >-function MsgBodyAllowHTML()
> >+//Global var array for msgbody render prefs feed/non feed,
> >+//[0] false = default non feed mode, true = feed mode, 3 prefs follow
> >+var gMsgBodyRenderPrefs = [false,
> >+                           gPrefBranch.getBoolPref("mailnews.display.prefer_plaintext"),
> >+                           gPrefBranch.getIntPref("mailnews.display.html_as"),
> >+                           gPrefBranch.getIntPref("mailnews.display.disallow_mime_handlers")]
> >+
> 
> This code, and the general preference setting code seems inconsistent and
> difficult. I also don't like the way you are attempting to re-use the
> mailnews.* prefs - for starters if I set reading email to one mode, go onto RSS
> feeds, set them to a different mode and then quit, when I go back into it and
> load my email, it shows it in the wrong mode.
> 
> I think the solution would be to make changes in libmime, I'm not sure the best
> way to go about that, I'm hoping David (Bienvenu) who is cc'ed on this bug
> might be able to help.
> 

ok.  it was tricky, and an attempt to avoid libmime, this code is now removed.  filed bug 458606.  someone else will need to pick it up though.
> >+//How to load message with content-base url on enter in threadpane
> >+var gHandleContentBaseMessage = gPrefBranch.getIntPref("rss.show.content-base");
> 
> I don't think this needs to be a global variable, there's only a couple of
> times you use it.
> 

are prefs cached?  if not then a disk read is to be avoided for frequent gets. in this case, it's been turned into a function given frequency of use and coded to only be accessed for rss msgs.
> >+//Current state: load web page if 0, show summary if 1
> >+var gShowFeedSummary;
> >+var gShowFeedSummaryToggle = false;
> 
> Again, I think these could be formed into functions.
> 

these are not so easy as they are state holders, i'll need to see how it can be rewritten.  
> >@@ -2895,3 +3018,175 @@
> >   else
> >     window.openDialog(chromeURL, "", "chrome,resizable,status,centerscreen,dialog=no", args);
> > }
> >+
> >+// Set prefs before mime processing
> >+function SetMsgBodyContentMode()
> >+{
> >+  var contentBase = currentHeaderData["content-base"];
> >+
> >+  // Not an rss message
> >+  if(!contentBase) {
> 
> Why not use isFeed() here?
> 

ok.
> >+    // Now in a non rss mode message; if flag was set for rss mode need
> >+    // to set the message body view prefs back to default. Otherwise
> >+    // not changing modes, so just return without resetting prefs.
> >+    if (!gMsgBodyRenderPrefs[0] == false) {
> 
> This should just be if (gMsgBodyRenderPrefs[0])
> 

ok.
> >+// Switch between message body (feed summary) and content-base url in
> >+// the Message Pane, called in MsgOpenSelectedMessages
> >+function FeedSetContentViewToggle()
> >+{
> >+  gShowFeedSummaryToggle = true;
> >+  var val = gShowFeedSummary ? 0 : 1;
> >+  FeedSetContentView(val);
> 
> Drop the intermediate variable val please.
> 

ok.
> >+// Check message format
> >+function FeedCheckContentFormat()
> >+{
> >+  var contentBase = currentHeaderData["content-base"];
> >+  var contentWindowDoc = window.top._content.document;
> >+
> >+  // Not an rss message
> >+  if (!contentBase)
> >+    return false;
> >+
> >+  // For pre Tb3 format..
> >+  var rssIframe = contentWindowDoc.getElementById('_mailrssiframe');
> >+  if (rssIframe) {
> >+    if (gShowFeedSummaryToggle ||
> >+        pref.getIntPref("rss.show.summary") == 1) {
> >+      alert(gMessengerBundle.getString("feedNoSummaryAlert"));
> 
> You should not be using alert. Please use the prompt service. See
> http://mxr.mozilla.org/comm-central/search?string=nsIPromptService for many
> examples.
> 

ok.
> Alternately, how about just displaying the message in the message pane (like we
> do when in offline mode for imap/news messages when they haven't been
> downloaded for offline use)? That way we can avoid an annoying prompt.
> 

no, it would then take 2 clicks to get back to the message content, while dismissing a dialog is 1.
> >+      gShowFeedSummaryToggle = false;
> >+    }
> >+    return false;
> >+  }
> >+  else
> >+    return true;
> 
> You don't need the else here, as the previous part of the if will always
> return.
> 

ok.
> >+}
> >\ No newline at end of file
> 
> Please fix this.
> 

ok.
> >Index: app/profile/all-thunderbird.js
> 
> >+//pref("rss.display.html_sanitizer.allowed_tags", "html head title body p br div(lang,title) h1 h2 h3 h4 h5 h6 ul(type,compact) ol(type,compact,start) li(type,value) dl dt dd blockquote(type,cite) pre noscript noframes strong em sub sup span(lang,title) acronym(title) abbr(title) del(title,cite,datetime) ins(title,cite,datetime) q(cite) a(href,name,title) img(alt,title,longdesc,src) base(href) area(alt) applet(alt) object(alt) var samp dfn address kbd code cite s strike tt b i table(align) caption tr(align,valign) td(rowspan,colspan,align,valign) th(rowspan,colspan,align,valign)");
> 
> Why is this pref commented out? Is it used? If not, please don't include it.

back in for libmime to use.

new patch upcoming later.

Comment 60

[alta88]

(In reply to comment #56)
> - oncommand="MsgBodyAllowHTML()"/>
> + oncommand="MsgBodyRenderPrefs(false, 0, 0)"/>
> 
> Please leave the old system. I think there should be as little logic in the XUL
> file as possible, that's why I have one function per menu item and then do the
> definition of what it means, which flags it sets, in the JS file.
> 
> I think you add a new menu item for the RSS case anyways, and hide the "Message
> Body" menu. So, please live that code unchanged.
> 

sorry, but that is simply not best practice code design, be it OO or assembler.  and triple the code for no gain is wrong. is there a design doc you can point to where having function args in xul is bad practice or is this a personal pref?  the thing i did improperly is not having well-named constants, but i don't think things like this matter around here.

anyway, whatever, i've left your code alone.

Comment 61

[alta88]

Attachment: revised patch per comments

Comment 62

[Ben Bucksch (:BenB)]

> sorry, but that is simply not best practice code design

Well, I beg to differ. I think the new code is far more complicated and for me much harder to read than it as before.

> triple the code

The new code is longer than before, so the increase would not be much, no.

> is there a design doc you can point
> to where having function args in xul is bad practice

There's no way I can understand your code without looking up the parameters in the XUL file, which means I have to search this out.

Even if you made a wrapper function, it would still mean that the code is more complex than before.

Anyways, that's a clear r- as far as I am concerned.

Comment 63

[alta88]

(In reply to comment #62)
> > sorry, but that is simply not best practice code design
> 
> Well, I beg to differ. I think the new code is far more complicated and for me
> much harder to read than it as before.
> 

this is too nonspecific a comment to be useful.  i'm sure you could say this about much of tb code if you wanted.  in general, the tricky code to avoid libmime has been removed, as decided above.  nor does it address what 'best practice' is.
> > triple the code
> 
> The new code is longer than before, so the increase would not be much, no.
> 

not sure if you understand or have looked at the new patch.  the issue at hand is that you use 3 functions to set the 3 render mode prefs where i use 1.  that's triple.
> > is there a design doc you can point
> > to where having function args in xul is bad practice
> 
> There's no way I can understand your code without looking up the parameters in
> the XUL file, which means I have to search this out.
> 

yes, and modular, reusable, 1/3 the size code will trump this *every* time.  bigger code for millions of users rather than a dev looking up/understanding where his functions are called makes no sense to me.
> Even if you made a wrapper function, it would still mean that the code is more
> complex than before.
> 
> Anyways, that's a clear r- as far as I am concerned.

if so on this point, then you're free to take up the patch and do it anyway you like.

as an aside, i hope you know the difference between
AllowHTML_menuitem.setAttribute("checked", AllowHTML_checked ? "true" : "false");
and
AllowHTML_menuitem.setAttribute("checked", AllowHTML_checked ? true : false);
and that you don't mind that i've changed it.

Comment 64

[Mark Banner (:standard8)]

Comment on attachment 341823 [details] [diff] [review]
revised patch per comments

Sorry for not getting to this earlier.

>@@ -180,16 +182,45 @@ function view_init()
>   }
> 
>   // hide the views menu item if the user doesn't have the views toolbar button visible
>   var viewsToolbarButton = document.getElementById("mailviews-container");
>   document.getElementById('viewMessageViewMenu').hidden = !viewsToolbarButton;
> 
>   // ... and also the separator
>   document.getElementById("viewMenuAfterTaskbarSeparator").hidden = !viewsToolbarButton;
>+
>+  // Initialize the Message Body menuitem
>+  var viewBodyMenu = document.getElementById('viewBodyMenu');
>+  if (viewBodyMenu)
>+    viewBodyMenu.hidden = isFeed;
>+
>+  // Initialize the Show Feed Summary menu
>+  var viewFeedSummary = document.getElementById('viewFeedSummary');
>+  var winType = document.documentElement.getAttribute('windowtype');
>+  if (viewFeedSummary) {
>+    if (winType != "mail:3pane")
>+      viewFeedSummary.hidden = !gShowFeedSummary;
>+    else
>+      viewFeedSummary.hidden = !isFeed;
>+  }
>+  var viewRssMenuItemIds = ["bodyFeedGlobalWebPage",
>+                            "bodyFeedGlobalSummary",
>+                            "bodyFeedPerFolderPref"];
>+  var checked = gPrefBranch.getIntPref("rss.show.summary");
>+  var viewRssItemToCheck = document.getElementById(viewRssMenuItemIds[checked]);
>+  if (viewRssItemToCheck)
>+    viewRssItemToCheck.setAttribute("checked", true);

I don't see why you're checking that the various document elements are valid. What might cause them to not exist? I notice you do this at various times throughout the patch.

>+  var isFeed = IsFeedItem();
>+  var defaultIDs = ["bodyAllowHTML",
>+                    "bodySanitized",
>+                    "bodyAsPlaintext"];
>+  var rssIDs = ["bodyFeedSummaryAllowHTML",
>+                "bodyFeedSummarySanitized",
>+                "bodyFeedSummaryAsPlaintext"];

These can be const arrays.

>+  var menuIDs = isFeed ? rssIDs : defaultIDs;
>   try
>   {
>-    prefer_plaintext = pref.getBoolPref("mailnews.display.prefer_plaintext");
>-    html_as = pref.getIntPref("mailnews.display.html_as");
>-    disallow_classes = pref.getIntPref("mailnews.display.disallow_mime_handlers");
>+    // Get prefs
>+    prefer_plaintext = isFeed ?
>+        pref.getBoolPref("rss.display.prefer_plaintext") :
>+        pref.getBoolPref("mailnews.display.prefer_plaintext");
>+    html_as = isFeed ?
>+        pref.getIntPref("rss.display.html_as") :
>+        pref.getIntPref("mailnews.display.html_as");
>+    disallow_classes = isFeed ?
>+        pref.getIntPref("rss.display.disallow_mime_handlers") :
>+        pref.getIntPref("mailnews.display.disallow_mime_handlers");

Given you've got 3 individual checks here, I think separating them into an if..else statement would be reasonable.

>+function IsFeedItem()
>+{
>+  if (gMsgFolderSelected)
>+    if (gMsgFolderSelected.server.type == 'rss')
>+      return true;
>+  if (GetFirstSelectedMessage() && 'content-base' in currentHeaderData)
>+    return true;

This can just be one if statement, or two if you want (but not three). In either case, please insert a blank line after the "return true;" statements.

>+// Check message format
>+function FeedCheckContentFormat()

It is not clear to me what this function is doing when I look at it stand-alone. This feels more like a "Do I need to redisplay feed item?" function, but I'm not sure that is totally right.

>+{
>+  var contentWindowDoc = window.top._content.document;
>+
>+  // Not an rss message
>+  if (!IsFeedItem())
>+    return false;
>+
>+  // For pre Tb3 format..
>+  var rssIframe = contentWindowDoc.getElementById('_mailrssiframe');

Please explain this in the comments a little bit more - something along the lines of, "For Thunderbird 2 feed messages were displayed in an iframe.

Which actually makes me think why are we doing this check - is this due to the way the feeds have been stored on the disk?

>+  if (rssIframe) {
>+    if (gShowFeedSummaryToggle ||
>+        pref.getIntPref("rss.show.summary") == 1) {
>+      var titleMsg = gMessengerBundle.getString("feedNoSummaryTitle");
>+      var dialogMsg = gMessengerBundle.getString("feedNoSummaryAlert");
>+      var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
>+                                    .getService(Components.interfaces.nsIPromptService);
>+      promptService.confirm(window, titleMsg, dialogMsg);

confirm has to be wrong - you're not handling the result.

>-      newSubject = mimeEncoder.encodeMimePartIIStr(this.mUnicodeConverter.ConvertFromUnicode(aSubject), false, aCharset, 9, 72);
>+      newSubject = mimeEncoder.encodeMimePartIIStr(this.mUnicodeConverter
>+                              .ConvertFromUnicode(aSubject), false, aCharset, 9, 72);

Incorrect indentation, the . should line up with the . of this.mUnicodeConverter

>+// Generic get feed property, based on child value. Assumes 1 unique
>+// child value with 1 unique parent, valid for feeds.rdf structure.
>+function getParentTargetForChildResource(childResource, parentTarget, server)
>+{
>+  var ds = getSubscriptionsDS(server);
>+  var childRes = rdf.GetResource(childResource);
>+  var parent = null;
>+
>+  var arcsIn = ds.ArcLabelsIn(childRes);
>+  while (arcsIn.hasMoreElements()){
>+    var arc = arcsIn.getNext();
>+    arc = arc.QueryInterface(Components.interfaces.nsIRDFResource);
>+    if (arc instanceof Components.interfaces.nsIRDFResource){

The query interface and the instanceof will do the same thing, except that QueryInterface will throw if arc isn't an instance of nsIRDFResource.

>+  if (parent) {
>+    var resource = rdf.GetResource(parent.Value);
>+    return ds.GetTarget(resource, parentTarget, true);
>+  }
>+  else
>+    return null;

You don't need the else are the return before it will mean you'll never hit it if (parent) is true.

Comment 65

[alta88]

(In reply to comment #64)
> (From update of attachment 341823 [details] [diff] [review])
> Sorry for not getting to this earlier.
> 
> >@@ -180,16 +182,45 @@ function view_init()
> >   }
> > 
> >   // hide the views menu item if the user doesn't have the views toolbar button visible
> >   var viewsToolbarButton = document.getElementById("mailviews-container");
> >   document.getElementById('viewMessageViewMenu').hidden = !viewsToolbarButton;
> > 
> >   // ... and also the separator
> >   document.getElementById("viewMenuAfterTaskbarSeparator").hidden = !viewsToolbarButton;
> >+
> >+  // Initialize the Message Body menuitem
> >+  var viewBodyMenu = document.getElementById('viewBodyMenu');
> >+  if (viewBodyMenu)
> >+    viewBodyMenu.hidden = isFeed;
> >+
> >+  // Initialize the Show Feed Summary menu
> >+  var viewFeedSummary = document.getElementById('viewFeedSummary');
> >+  var winType = document.documentElement.getAttribute('windowtype');
> >+  if (viewFeedSummary) {
> >+    if (winType != "mail:3pane")
> >+      viewFeedSummary.hidden = !gShowFeedSummary;
> >+    else
> >+      viewFeedSummary.hidden = !isFeed;
> >+  }
> >+  var viewRssMenuItemIds = ["bodyFeedGlobalWebPage",
> >+                            "bodyFeedGlobalSummary",
> >+                            "bodyFeedPerFolderPref"];
> >+  var checked = gPrefBranch.getIntPref("rss.show.summary");
> >+  var viewRssItemToCheck = document.getElementById(viewRssMenuItemIds[checked]);
> >+  if (viewRssItemToCheck)
> >+    viewRssItemToCheck.setAttribute("checked", true);
> 
> I don't see why you're checking that the various document elements are valid.
> What might cause them to not exist? I notice you do this at various times
> throughout the patch.

removed.  seems like the custom though..

> 
> >+  var isFeed = IsFeedItem();
> >+  var defaultIDs = ["bodyAllowHTML",
> >+                    "bodySanitized",
> >+                    "bodyAsPlaintext"];
> >+  var rssIDs = ["bodyFeedSummaryAllowHTML",
> >+                "bodyFeedSummarySanitized",
> >+                "bodyFeedSummaryAsPlaintext"];
> 
> These can be const arrays.
> 

ok.

> >+  var menuIDs = isFeed ? rssIDs : defaultIDs;
> >   try
> >   {
> >-    prefer_plaintext = pref.getBoolPref("mailnews.display.prefer_plaintext");
> >-    html_as = pref.getIntPref("mailnews.display.html_as");
> >-    disallow_classes = pref.getIntPref("mailnews.display.disallow_mime_handlers");
> >+    // Get prefs
> >+    prefer_plaintext = isFeed ?
> >+        pref.getBoolPref("rss.display.prefer_plaintext") :
> >+        pref.getBoolPref("mailnews.display.prefer_plaintext");
> >+    html_as = isFeed ?
> >+        pref.getIntPref("rss.display.html_as") :
> >+        pref.getIntPref("mailnews.display.html_as");
> >+    disallow_classes = isFeed ?
> >+        pref.getIntPref("rss.display.disallow_mime_handlers") :
> >+        pref.getIntPref("mailnews.display.disallow_mime_handlers");
> 
> Given you've got 3 individual checks here, I think separating them into an
> if..else statement would be reasonable.
> 

ok.

> >+function IsFeedItem()
> >+{
> >+  if (gMsgFolderSelected)
> >+    if (gMsgFolderSelected.server.type == 'rss')
> >+      return true;
> >+  if (GetFirstSelectedMessage() && 'content-base' in currentHeaderData)
> >+    return true;
> 
> This can just be one if statement, or two if you want (but not three). In
> either case, please insert a blank line after the "return true;" statements.
> 

no.  can't combine first two since if no folder getting its property will throw.  can't combine last two for new windows opens to work.

> >+// Check message format
> >+function FeedCheckContentFormat()
> 
> It is not clear to me what this function is doing when I look at it
> stand-alone. This feels more like a "Do I need to redisplay feed item?"
> function, but I'm not sure that is totally right.
> 

no. simply, if not a feed, can't toggle so bail.  if an old feed within an iframe, can't toggle so show message and bail.  otherwise ok to toggle.  i'm not clear on the problem here.

> >+{
> >+  var contentWindowDoc = window.top._content.document;
> >+
> >+  // Not an rss message
> >+  if (!IsFeedItem())
> >+    return false;
> >+
> >+  // For pre Tb3 format..
> >+  var rssIframe = contentWindowDoc.getElementById('_mailrssiframe');
> 
> Please explain this in the comments a little bit more - something along the
> lines of, "For Thunderbird 2 feed messages were displayed in an iframe.
> 

ok.

> Which actually makes me think why are we doing this check - is this due to the
> way the feeds have been stored on the disk?
>

yes, if Show summary is not checked then the message was constructed with an <iframe src='url'> within the <body>. if Show summary is checked, it's constructed as summary content within a <body>.

now, it's possible to reconstruct the old message body with the iframe innerHtml text to get the summary out.  if you think this is worth it, i'll do it in a follow up bug.
 
> >+  if (rssIframe) {
> >+    if (gShowFeedSummaryToggle ||
> >+        pref.getIntPref("rss.show.summary") == 1) {
> >+      var titleMsg = gMessengerBundle.getString("feedNoSummaryTitle");
> >+      var dialogMsg = gMessengerBundle.getString("feedNoSummaryAlert");
> >+      var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
> >+                                    .getService(Components.interfaces.nsIPromptService);
> >+      promptService.confirm(window, titleMsg, dialogMsg);
> 
> confirm has to be wrong - you're not handling the result.
> 

should have been alert, fixed.

> >-      newSubject = mimeEncoder.encodeMimePartIIStr(this.mUnicodeConverter.ConvertFromUnicode(aSubject), false, aCharset, 9, 72);
> >+      newSubject = mimeEncoder.encodeMimePartIIStr(this.mUnicodeConverter
> >+                              .ConvertFromUnicode(aSubject), false, aCharset, 9, 72);
> 
> Incorrect indentation, the . should line up with the . of
> this.mUnicodeConverter

rewrapped.
> 
> >+// Generic get feed property, based on child value. Assumes 1 unique
> >+// child value with 1 unique parent, valid for feeds.rdf structure.
> >+function getParentTargetForChildResource(childResource, parentTarget, server)
> >+{
> >+  var ds = getSubscriptionsDS(server);
> >+  var childRes = rdf.GetResource(childResource);
> >+  var parent = null;
> >+
> >+  var arcsIn = ds.ArcLabelsIn(childRes);
> >+  while (arcsIn.hasMoreElements()){
> >+    var arc = arcsIn.getNext();
> >+    arc = arc.QueryInterface(Components.interfaces.nsIRDFResource);
> >+    if (arc instanceof Components.interfaces.nsIRDFResource){
> 
> The query interface and the instanceof will do the same thing, except that
> QueryInterface will throw if arc isn't an instance of nsIRDFResource.
> 

no need to throw, QI removed.

> >+  if (parent) {
> >+    var resource = rdf.GetResource(parent.Value);
> >+    return ds.GetTarget(resource, parentTarget, true);
> >+  }
> >+  else
> >+    return null;
> 
> You don't need the else are the return before it will mean you'll never hit it
> if (parent) is true.

ok.

Comment 66

[alta88]

Attachment: per comments

Comment 67

[alta88]

> >+  if (gMsgFolderSelected)
> >+    if (gMsgFolderSelected.server.type == 'rss')

Ben has pointed out to me the second won't evaluate if combined, and that's what the doc says.  could have sworn i got undefineds when doing this elsewhere..  so it'll be fixed.

Comment 68

[Mark Banner (:standard8)]

Comment on attachment 346283 [details] [diff] [review]
per comments

Thanks for the update, the interdiffs are looking a lot better. Unfortunately the patch is bitrotted, due to SeaMonkey now picking up newsblog as well.

Could you refresh the patch, add the equivalent changes to the SeaMonkey locale & theme files, and post a new version that I can give a quick check on before I do the review?

We'll see if we can land this as soon as the tree reopens after the beta 1 freeze.

Comment 69

[alta88]

Attachment: new patch

Comment 70

[David :Bienvenu]

moving to b2

Comment 71

[Mark Banner (:standard8)]

Comment on attachment 348624 [details] [diff] [review]
new patch

We're almost there I think, there is a little bit of bitrot in mail/themes/qute, but that was easy to fix when I applied the patch.

>+  const defaultIDs = ["bodyAllowHTML",
>+                    "bodySanitized",
>+                    "bodyAsPlaintext"];
>+  const rssIDs = ["bodyFeedSummaryAllowHTML",
>+                "bodyFeedSummarySanitized",
>+                "bodyFeedSummaryAsPlaintext"];

nit: please line the all the starting quotes up so that the are directly in line.

>+    // Get prefs
>+    if (isFeed) {
>+      prefer_plaintext = pref.getBoolPref("rss.display.prefer_plaintext");
>+      html_as = pref.getIntPref("rss.display.html_as");
>+      disallow_classes = pref.getIntPref("rss.display.disallow_mime_handlers");
>+    }
>+    else {
>     prefer_plaintext = pref.getBoolPref("mailnews.display.prefer_plaintext");
>     html_as = pref.getIntPref("mailnews.display.html_as");
>     disallow_classes = pref.getIntPref("mailnews.display.disallow_mime_handlers");
>+    }

nit: you need to adjust the indentation of the lines you haven't changed.

>diff -r f15f33e40beb suite/locales/en-US/chrome/mailnews/messenger.dtd

If you're changing these files, you need to change the equivalent of the mail/ files that you've done the patch for (and the equivalent files are in mailnews) otherwise there's not much point in doing the additions because SeaMonkey won't pick them up.

r- because we should either do the equivalent changes or not modify the SeaMonkey locale files (if that's possible, I've got a feeling we should be doing the equivalent changes), but the rest of the patch is looking good now.

Comment 72

[Magnus Melin [:mkmelin]]

alta88: not interested in getting the final patch in? Doesn't sound like a lot of review comments to address.

Comment 73

[Wayne Mery (:wsmwk)]

new owner needed.  this thunderbird3-wanted+ is at risk of not making it into thunderbird 3.

alta88 is not driving in the patch, but writes "bug has just a few nits left i recall, and anybody could finish it off."

Comment 74

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: addresses standard8's review in comment 71

(In reply to comment #71)
> (From update of attachment 348624 [details] [diff] [review])
> We're almost there I think, there is a little bit of bitrot in
> mail/themes/qute, but that was easy to fix when I applied the patch.

Here's an updated patch against the tip.


> >+  const defaultIDs = ["bodyAllowHTML",
> >+                    "bodySanitized",
> >+                    "bodyAsPlaintext"];
> >+  const rssIDs = ["bodyFeedSummaryAllowHTML",
> >+                "bodyFeedSummarySanitized",
> >+                "bodyFeedSummaryAsPlaintext"];
> 
> nit: please line the all the starting quotes up so that the are directly in
> line.

Fixed.


> >+    // Get prefs
> >+    if (isFeed) {
> >+      prefer_plaintext = pref.getBoolPref("rss.display.prefer_plaintext");
> >+      html_as = pref.getIntPref("rss.display.html_as");
> >+      disallow_classes = pref.getIntPref("rss.display.disallow_mime_handlers");
> >+    }
> >+    else {
> >     prefer_plaintext = pref.getBoolPref("mailnews.display.prefer_plaintext");
> >     html_as = pref.getIntPref("mailnews.display.html_as");
> >     disallow_classes = pref.getIntPref("mailnews.display.disallow_mime_handlers");
> >+    }
> 
> nit: you need to adjust the indentation of the lines you haven't changed.

Fixed.


> >diff -r f15f33e40beb suite/locales/en-US/chrome/mailnews/messenger.dtd
> 
> If you're changing these files, you need to change the equivalent of the mail/
> files that you've done the patch for (and the equivalent files are in 
> mailnews) otherwise there's not much point in doing the additions because 
> SeaMonkey won't pick them up.

This patch applies the changes in mail/base/content/mailWindowOverlay.(js|xul) to suite/mailnews/mailWindowOverlay.(js|xul).  I merged them by hand, since |patch| wasn't able to merge them automatically, but the merge was straightforward.  mailWindowOverlay.js is quite different between the two applications, but most of the differences in it seem cosmetic.

It looks like the rest of the changes have been made to both applications' files or are shared between the two applications.  However, I have not built and tested Seamonkey with these changes.

This patch also reverts the change to updateFolderFeedUrl(), which had replaced a reference to nsIFolder::msgDatabase with a call to nsIFolder::getMsgDatabase(), because of the fix for bug 473458, which replaced nsIFolder::getMsgDatabase() with nsIFolder::msgDatabase <http://hg.mozilla.org/comm-central/rev/649e4c6a0e38>.

And it adds MsgFeedBodyRenderPrefs() back into mailWindowOverlay.js.  That function gets called in several places and was in patch v2 but was missing from patch v3.

I'm not familiar with everything this patch does, but I did some testing, and it seems to fix bug 309696, bug 259018, and bug 325424.  I think it also fixes bug 333237.

I don't think it fixes bug 353302, though, as the View > Feed Message Body As > Original HTML|Simple HTML|Plain Text options don't change the appearance of the feed messages in the message pane (although no errors appear in the Error Console).  With MsgFeedBodyRenderPrefs() back in the patch, they should work; I don't know why they don't.

Comment 75

[Mark Banner (:standard8)]

Comment on attachment 372367 [details] [diff] [review]
patch v4: addresses standard8's review in comment 71

Myk, thanks for updating this. I'm still not sure the UI will work as the user expects, but clarkbw has given it the ok, and this is better than what we have now.

Additionally fixing 4 bugs is certainly more than enough for this patch.

The SM changes seem to work fine as well.

r=me.

Comment 76

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 372367 [details] [diff] [review]
patch v4: addresses standard8's review in comment 71

David, can you super-review the parts of this patch that require it?

Comment 77

[David :Bienvenu]

Comment on attachment 372367 [details] [diff] [review]
patch v4: addresses standard8's review in comment 71

one nit - this could just be return GetFirstSelected ...

i.e, return the if clause.


+function IsFeedItem()
+{
+  if (GetFirstSelectedMessage() &&
+      ((gMsgFolderSelected && gMsgFolderSelected.server.type == 'rss') ||
+      'content-base' in currentHeaderData))
+    return true;
+
+  return false;

Comment 78

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: addresses bienvenu's nit

(In reply to comment #77)
> (From update of attachment 372367 [details] [diff] [review])
> one nit - this could just be return GetFirstSelected ...
> 
> i.e, return the if clause.

Yup, here's a version with that change.  I also updated the patch to the tip.

Comment 79

[David :Bienvenu]

Comment on attachment 374992 [details] [diff] [review]
patch v5: addresses bienvenu's nit

thx very much, Myk.

Comment 80

[Myk Melez [:myk] [@mykmelez]]

Fixed by changeset http://hg.mozilla.org/comm-central/rev/ed1011d1e791.

Comment 81

[Mike Cowperthwaite]

There's a fairly large number of bugs regarding the "view feed body as" change:
bug 452828
bug 475083
bug 491187
bug 503389	
bug 513652 (confirmed)
bug 517477 (confirmed) -- has some technical info
bug 521095
bug 532414
bug 541718

I haven't gone into these in detail, but they mostly seem to be complaining that Summary display is being used where it wasn't expected.


I have just opened a few specific UI bugs about the menu and display mode:
bug 550793
bug 550794
bug 550800

A related menu bug is:
bug 515790

Comment 82

[Mike Cowperthwaite]

Why didn't this patch get ui-review?

Comment 83

[Mark Banner (:standard8)]

(In reply to comment #82)
> Why didn't this patch get ui-review?

It did. Look through the history at the obsolete patches.