Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Body of RSS feeds won't print or print preview

RESOLVED FIXED in Thunderbird 18.0

Status

MailNews Core
Printing
P2
major
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: Loeb, Assigned: alta88)

Tracking

(Blocks: 1 bug, {regression})

Trunk
Thunderbird 18.0
regression
Dependency tree / graph
Bug Flags:
in-testsuite -

Thunderbird Tracking Flags

(thunderbird17 fixed)

Details

Attachments

(4 attachments, 14 obsolete attachments)

195.28 KB, image/pjpeg
Details
6.06 KB, application/pdf
Details
6.14 KB, application/pdf
Details
40.90 KB, patch
mconley
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
User-Agent:       Mozilla/4.0 (compatible; MSIE 8.0; Windows NT 6.1; WOW64; Trident/4.0; GTB0.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.8) Gecko/20100802 Lightning/1.0b2 Thunderbird/3.1.2

When printing RSS "messages" from craigslist in "News and Blogs" section of Thunderbird, only the message header and footer print, the body is blank.  The body and images ARE visible on the screen.  Email messages (as opposed to News and Blogs messages) with HTML print properly.

Reproducible: Always

Steps to Reproduce:
1. Subscribe to a craigslist RSS feed in "News and Blogs" such as <http://sfbay.craigslist.org/search/boa?query=catalina+27&catAbbreviation=boa&minAsk=min&maxAsk=max&format=rss>
2.When you get a new message for this feed, click on the message
3.Choose Print or Print Preview.  You will notice that there is not message body printed, only the header and footer.
Actual Results:  
Message printed with header and footer visible, no body is visible.

Expected Results:  
I expected the whole message to print, header, body, and footer (WYSIWYG).  Tbird V2 worked properly.

The reason this is a "Major" bug is, because of the fleeting nature of news feeds, I like to archive important ones by printing to a PDF file.  If I can't print from Tbird, I'll have to use a different RSS reader.
(Reporter)

Updated

7 years ago
Priority: -- → P2
Version: unspecified → 3.1
(Reporter)

Comment 1

7 years ago
Created attachment 475091 [details]
Screen shot of original RSS message
(Reporter)

Comment 2

7 years ago
Created attachment 475093 [details]
This is what prints
(Reporter)

Comment 3

7 years ago
Printed result is the same whether it is a Print or a Print Preview and whether one is printing to a physical printer or to Abobe PDF.
Component: General → Printing
Product: Thunderbird → MailNews Core
QA Contact: general → printing
Version: 3.1 → Trunk
Confirming with both TB and SM trunk, Windows and Linux.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Duplicate of this bug: 576741
(Assignee)

Comment 6

5 years ago
Created attachment 616318 [details] [diff] [review]
patch.
Assignee: nobody → alta88
Attachment #616318 - Flags: review?(mbanner)
(Assignee)

Comment 7

5 years ago
Comment on attachment 616318 [details] [diff] [review]
patch.

[Approval Request Comment]
Regression caused by (bug #): 438429
User impact if declined: print preview/print of feed msgs will not work
Testing completed (on c-c, etc.): local testing
Risk to taking this patch (and alternatives if risky): almost zero
Attachment #616318 - Flags: approval-comm-beta?
Attachment #616318 - Flags: approval-comm-aurora?
(Assignee)

Updated

5 years ago
Keywords: regression
(Assignee)

Comment 8

5 years ago
Created attachment 617037 [details] [diff] [review]
tweak to make it slightly safer.
Attachment #616318 - Attachment is obsolete: true
Attachment #617037 - Flags: review?(mbanner)
Attachment #617037 - Flags: approval-comm-beta?
Attachment #617037 - Flags: approval-comm-aurora?
Attachment #616318 - Flags: review?(mbanner)
Attachment #616318 - Flags: approval-comm-beta?
Attachment #616318 - Flags: approval-comm-aurora?
Comment on attachment 617037 [details] [diff] [review]
tweak to make it slightly safer.

Too late for TB12, unless there is a chemspill.
Comment on attachment 617037 [details] [diff] [review]
tweak to make it slightly safer.

Sorry, I can't seem to get this to work on Mac. It doesn't seem to pick up the fact it is there. Maybe you need to try it before the .showWindow?

As Callek said, not taking for beta, as its too late for that. We'll reconsider when we get a patch with r+.
Attachment #617037 - Flags: review?(mbanner)
Attachment #617037 - Flags: review-
Attachment #617037 - Flags: approval-comm-beta?
Attachment #617037 - Flags: approval-comm-aurora?
(Assignee)

Comment 11

5 years ago
Created attachment 617939 [details] [diff] [review]
patch.


the .showWindow is better later to make it (appear to) paint more smoothly.

wfm on win7 and linux both ways; someone with a mac would have to test there.  i'm willing to wager a free checkin that you didn't run it with -purgecaches after the apply.
Attachment #617037 - Attachment is obsolete: true
Attachment #617939 - Flags: review?(mbanner)
Sorry for the delay in this. I pushed it to try so that I could get builds to test it on the platforms I don't normally build and forgot about it :-(

I've repushed, for my notes the try link is this:

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=b8e4356120b5

I should get to this tomorrow.
Comment on attachment 617939 [details] [diff] [review]
patch.

Ok, I somehow can't reproduce this, when using 12.0.1 on Windows, print preview appears to be working fine. So there doesn't seem to be any change before or after the patch.

I've tried the feed mentioned in this bug, and my own blog feed and I can't see any different.

Redirecting to Mike in case he can pick up on anything.
Attachment #617939 - Flags: review?(mbanner) → review?(mconley)
(Reporter)

Comment 14

5 years ago
I just tested using TB 12.0.1 and a RSS message from Craigslist and the behavior has not changed, the big is still there.  I'll add a PDF of what prints to this record where you can see the header is the message subject/title and the body does not print.  PDF was printed from within TB RSS.  RSS feed is from http://seattle.craigslist.org/see/egr/3030017600.html
(Reporter)

Comment 15

5 years ago
Title of attachment will be "I&C Engineer III (PLC Programmer) (Seattle, WA).pdf"
(Reporter)

Comment 16

5 years ago
Created attachment 626847 [details]
PDF Print demonstrating bug

Here's an updated example of what prints from 05/24/12, using TB 12.0.1
(Assignee)

Comment 17

5 years ago
(In reply to Mark Banner (:standard8) from comment #13)
> Comment on attachment 617939 [details] [diff] [review]
> patch.
> 
> Ok, I somehow can't reproduce this, when using 12.0.1 on Windows, print
> preview appears to be working fine. So there doesn't seem to be any change
> before or after the patch.
> 
> I've tried the feed mentioned in this bug, and my own blog feed and I can't
> see any different.
> 
> Redirecting to Mike in case he can pick up on anything.

not that i don't believe you, but there's no way technically for print preview to work.  in the mbox, the feed message is always stored as a summary, constructed as
<body id="msgFeedSummaryBody" selected="false">,
and a css rule in /messageBody.css, hardcoded in cpp to be injected in a message <browser>,
body[selected="false"] {
  display: none;
}

the reason for this is that there would be an unacceptably annoying flash of the summary, on message select, if the default were to show web page. (there of course may be a different way to accomplish this).

it, heh, fails for me in win7, winxp, linux without the patch.
I can reproduce this on Daily. Examining patch now...
Comment on attachment 617939 [details] [diff] [review]
patch.

Hey alta88, this patch fails to apply cleanly. Can you un-bitrot it please, and re-request review from me?

Thanks,

-Mike
Attachment #617939 - Flags: review?(mconley) → review-
(Assignee)

Comment 20

5 years ago
Created attachment 626934 [details] [diff] [review]
Patch v8
Attachment #617939 - Attachment is obsolete: true
Attachment #626934 - Flags: review?(mconley)
Comment on attachment 626934 [details] [diff] [review]
Patch v8

Just giving me a larger link to click. ;)
Attachment #626934 - Attachment description: . → Patch v8
Attachment #626934 - Attachment is patch: true
Comment on attachment 626934 [details] [diff] [review]
Patch v8

Review of attachment 626934 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me, with the nit I found fixed. Was able to reproduce the issue, and this fixed it as advertised.

r=me - handing over to Ian from the SM team to make sure it's OK for them.

::: mailnews/base/content/msgPrintEngine.js
@@ +90,5 @@
>      setPPTitle(document.getElementById("content").contentDocument.title);
>      document.getElementById("content").collapsed = true;
> +
> +    // Ensure feed body is viewable.
> +    var browser = document.getElementById("ppBrowser");

We prefer let over var. Same for msgFeedSummaryBody.
Attachment #626934 - Flags: review?(mconley)
Attachment #626934 - Flags: review?(iann_bugzilla)
Attachment #626934 - Flags: review+

Comment 23

5 years ago
This works for me on:

Mozilla/5.0 (OS/2; Warp 4.5; rv:10.0.4) Gecko/20120430 Firefox/10.0.4 SeaMonkey/2.7.2 ID:20120430222706

when applied to msgPrintEngine.js in the appropriate location (the line numbers match, at least, but the location in omni.ja is slightly different for SM than TB).

However, this only addresses the print preview on OS/2 (or on SM in general; I haven't tested on Linux or Win, yet). Printing still results in a blank body.
(In reply to Lewis Rosenthal from comment #23)
> This works for me on:
> 
> Mozilla/5.0 (OS/2; Warp 4.5; rv:10.0.4) Gecko/20120430 Firefox/10.0.4
> SeaMonkey/2.7.2 ID:20120430222706
> 
> when applied to msgPrintEngine.js in the appropriate location (the line
> numbers match, at least, but the location in omni.ja is slightly different
> for SM than TB).
> 
> However, this only addresses the print preview on OS/2 (or on SM in general;
> I haven't tested on Linux or Win, yet). Printing still results in a blank
> body.

Ack, I didn't actually test this by printing - just by Print Preview (which you think would, y'know, preview what would be printed). Lemme give printing a shot.
Comment on attachment 626934 [details] [diff] [review]
Patch v8

Confirmed the last comment - while this fixes print preview, printing still doesn't work properly.

Kinda baffling, really. You'd think they'd match.
Attachment #626934 - Flags: review?(iann_bugzilla)
Attachment #626934 - Flags: review-
Attachment #626934 - Flags: review+

Comment 26

5 years ago
Reading it over, it seems to address the ppBrowser element only, and while I agree, Mike, that I would expect that to do the trick, looking at the printing code, it appears that "msgFeedSummaryBody" does not necessarily mean "content" as referenced in the section handing the job off to the printer (though in truth, I may be misreading the javascript, having just skimmed it briefly).
(Assignee)

Comment 27

5 years ago
Created attachment 628751 [details] [diff] [review]
patch


this print code is hacky.  it previews one <browser> doc but sends another to the printer, sort of defeats the purpose.  anyway, this addresses both docs.

let cannot be used here, there is a reserved keyword throw.
Attachment #626934 - Attachment is obsolete: true
Attachment #628751 - Flags: review?(mconley)

Comment 28

5 years ago
Almost there.

Printing from print preview works (thanks!), but printing directly does not (on SM 2.7.2 on OS/2, at least). Somehow, when we're looking for content, we're not getting it passed. I looked at it a bit last night, but got distracted (again).

Good work so far, though!
(Assignee)

Comment 29

5 years ago
unfortunately, direct print is a different path, passing the url directly to the print engine interface, and this intercept point won't work.  i'm not going to open the print engine up for that.

i think the solution will have to be something like removing the css from the file auto injected in a message <browser> and applying it to the <browser> only in the messagepane.
Comment on attachment 628751 [details] [diff] [review]
patch

Review of attachment 628751 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the one style nit fixed.

So this takes care of the Print Preview -> Print case. We should keep this bug open until we solve the general Print case.

::: mailnews/base/content/msgPrintEngine.js
@@ +63,5 @@
> +      browser ? browser.contentDocument.getElementById("msgFeedSummaryBody") : null;
> +    if (msgFeedSummaryBody) {
> +      msgFeedSummaryBody.removeAttribute("selected");
> +      var cbrowser = document.getElementById("content");
> +      cbrowser.contentDocument.getElementById("msgFeedSummaryBody").

If you're going to break up this line, please do it like:

cbrowser.contentDocument
        .getElementById("msgFeedSummaryBody")
        .removeAttribute("selected");
Attachment #628751 - Flags: review?(mconley) → review+
(Assignee)

Comment 31

5 years ago
we could check this in, but the solution i have in mind would just reverse it.  however, i'll be computer free for the rest of june and won't look at it for a while.  if it's felt that a partial solution now is worth it, i can fix the nit for checkin.
(Assignee)

Comment 32

5 years ago
this is fixed in bug 728563.
Depends on: 728563
(Assignee)

Comment 33

5 years ago
Created attachment 651013 [details] [diff] [review]
patch


as the UI items are unclear in the above bug, this regression and other issues in the backend shouldn't depend on it.  so the backend work has been removed to this patch.  it attempts the following:

1) fix print/print preview issue,
2) prevent blank display in messagepane (do incremental loading),
3) fix standalone window not honoring display pref (broken by tabs impl),
4) code refactor and global scope un-pollution.

r? for neil for suite .css change.
Attachment #628751 - Attachment is obsolete: true
Attachment #651013 - Flags: review?(neil)
Attachment #651013 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
No longer depends on: 728563
(Assignee)

Comment 34

5 years ago
mike, a ping.  it would be good to get this in for 17.
Comment on attachment 651013 [details] [diff] [review]
patch

Sorry to take so long to get back to you on this.

It looks as if you're completely going to break SeaMonkey's RSS support, for instance you're removing several preferences referenced in our code. So from my point of view I'm going to have to mark this r- on that basis.
Attachment #651013 - Flags: review?(neil) → review-
(Assignee)

Comment 36

5 years ago
Created attachment 653355 [details] [diff] [review]
patch


updated patch with no effect on suite.
Attachment #651013 - Attachment is obsolete: true
Attachment #651013 - Flags: review?(mconley)
Attachment #653355 - Flags: review?(mconley)
(In reply to alta88 from comment #34)
> mike, a ping.  it would be good to get this in for 17.

Hm. This is a far more extensive change than I remember from your previous patches.

While I can review the style and basic architecture of the code, I can't really claim any degree of expertise with our RSS / Newsreader front-end code.

Mark: is there anybody else around more qualified than I am?
(Assignee)

Comment 38

5 years ago
it is, however, necessary.  all it really does is find a new better entry point in the web page/summary decision, and this removes the css hack that killed both printing and message pane loading if there was just one broken link.  the rest is just code reorg in an object.

i think that would be sufficient.  i'm not sure there is anyone who understands the current feeds arch other than me.  it has been extensively renovated over the last 8 months, by me.  you can refer to bugzilla or ping bienvenu..
Comment on attachment 653355 [details] [diff] [review]
patch

Review of attachment 653355 [details] [diff] [review]:
-----------------------------------------------------------------

Nothing major - just a few suggestions here and there.

::: mail/base/content/mailWindowOverlay.js
@@ +138,5 @@
>   *  responsible for updating the menu items' state to reflect reality.
>   */
>  function view_init()
>  {
> +  var isFeed = gFolderDisplay &&

We prefer let over var

@@ +139,5 @@
>   */
>  function view_init()
>  {
> +  var isFeed = gFolderDisplay &&
> +               ((gFolderDisplay.displayedFolder && 

Nit - whitespace

@@ +176,5 @@
>  
>    var viewRssMenuItemIds = ["bodyFeedGlobalWebPage",
>                              "bodyFeedGlobalSummary",
>                              "bodyFeedPerFolderPref"];
> +  var checked = FeedMessageHandler.onSelectPref;

We prefer let over var

@@ +333,5 @@
>    if (winType == "mail:3pane")
>      document.getElementById('openMessageWindowMenuitem').hidden = isFeed;
>  
>    // Initialize the Open Feed Message handler menu
> +  var index = FeedMessageHandler.onOpenPref;

We prefer let over var - I'll stop mentioning that now.

@@ +410,5 @@
>    var menuIDs = isFeed ? rssIDs : defaultIDs;
>    try
>    {
>      // Get prefs
> +    if (isFeed && 0) {

&& 0? This will always evaluate to false...

@@ +456,5 @@
>      AllBodyParts_menuitem.setAttribute("checked", true);
>    // else (the user edited prefs/user.js) check none of the radio menu items
>  
>    if (isFeed) {
> +    AllowHTML_menuitem.hidden = !FeedMessageHandler.gShowSummary;

I appreciate the effort to move some of these globals into FeedMessageHandler.  Kudos.

@@ +3171,5 @@
> +   * be in an rss acount type folder.  In Tb15 and later, a flag is set on the
> +   * message itself upon initial store; the message can be moved to any folder.
> +   *
> +   * @param aMsgHdr aMsgHdr - the message.
> +   * 

Trailing whitespace

@@ +3186,5 @@
> +   * message pane.
> +   *
> +   * @param nsIMsgDBHdr aMsgHdr - the message.
> +   * @param bool aToggle        - true if in toggle mode, false otherwise.
> +   * 

Trailing whitespace

@@ +3218,5 @@
> +      return gShowFeedSummary = this.gShowSummary = !this.gShowSummary;
> +
> +    let wintype = document.documentElement.getAttribute("windowtype");
> +    let tabMail = document.getElementById("tabmail");
> +    let folderTab = tabMail && tabMail.currentTabInfo.mode.type == "folder";

I don't see folderTab or glodaListTab being used...

@@ +3246,3 @@
>          }
> +        catch (ex) {
> +          // Not in a feed account folder or other error.

Instead of swallowing this silently, should we at the very least pump something out to the Error Console?

@@ +3296,5 @@
> +    MsgHdrToMimeMessage(aMessageHdr, null, function(aMsgHdr, aMimeMsg) {
> +      if (aMimeMsg && aMimeMsg.headers["content-base"] &&
> +          aMimeMsg.headers["content-base"][0]) {
> +        let url = aMimeMsg.headers["content-base"], uri;
> +        let where = aWhere;

why are you aliasing aWhere to where? Can't you just use aWhere?

@@ +3308,5 @@
> +          return;
> +        }
> +        if (where.browser)
> +          Components.classes["@mozilla.org/uriloader/external-protocol-service;1"].
> +          getService(Components.interfaces.nsIExternalProtocolService).loadURI(uri);

I'd prefer this formatted as:

Components.classes["@mozilla.org/uriloader/external-protocol-service;1"]
          .getService(Components.interfaces.nsIExternalProtocolService)
          .loadURI(uri);

@@ +3326,5 @@
> +  },
> +
> +  /**
> +   * Display summary or load web page for feed messages.  Caller should already
> +   * know if the message is a feed message. 

Trailing whitespace

::: mailnews/mailnews.js
@@ +525,5 @@
>  pref("rss.display.html_as", 0);
>  pref("rss.display.disallow_mime_handlers", 0);
>  
> +// Feed message display (summary or web page), on select.
> +//0 - global override, load web page

While you're here, please put a space after // on lines 529 - 531

@@ +537,5 @@
>  // 1 - open summary in new window
>  // 2 - toggle load summary and content-base url in message pane
>  pref("rss.show.content-base", 0);
> +
> +//Feed message additional web page display.

Space after // on lines 541 - 543.
Attachment #653355 - Flags: review?(mconley) → review-
(Assignee)

Comment 40

5 years ago
(In reply to Mike Conley (:mconley) from comment #39)
> Comment on attachment 653355 [details] [diff] [review]
> patch
> 
> Review of attachment 653355 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> We prefer let over var
> 

of course.  but what is the policy for touching one statement in a huge file full of vars?  mix/match seems odd.  and suite reviewers strongly dislike mass var -> let changes.

> 
> Nit - whitespace
> 

all cleaned up.

> 
> && 0? This will always evaluate to false...
> 

yes, but the code was left in case the feed specific rendering bug is ever done.
remove?

>
> I don't see folderTab or glodaListTab being used...
> 

removed.

> @@ +3246,3 @@
> >          }
> > +        catch (ex) {
> > +          // Not in a feed account folder or other error.
> 
> Instead of swallowing this silently, should we at the very least pump
> something out to the Error Console?
> 

ok.

> @@ +3296,5 @@
> > +    MsgHdrToMimeMessage(aMessageHdr, null, function(aMsgHdr, aMimeMsg) {
> > +      if (aMimeMsg && aMimeMsg.headers["content-base"] &&
> > +          aMimeMsg.headers["content-base"][0]) {
> > +        let url = aMimeMsg.headers["content-base"], uri;
> > +        let where = aWhere;
> 
> why are you aliasing aWhere to where? Can't you just use aWhere?
> 

ok.

> 
> I'd prefer this formatted as:
> 
> Components.classes["@mozilla.org/uriloader/external-protocol-service;1"]
>           .getService(Components.interfaces.nsIExternalProtocolService)
>           .loadURI(uri);
> 

ok.

> 
> ::: mailnews/mailnews.js
> @@ +525,5 @@
> 
> While you're here, please put a space after // on lines 529 - 531
> 

> 
> Space after // on lines 541 - 543.

ok.
(Assignee)

Comment 41

5 years ago
Created attachment 655006 [details] [diff] [review]
patch


address comments and add two lines to handle browser open case, inadvertently omitted.

please address the other above points one way or another so this can be closed out.
Attachment #653355 - Attachment is obsolete: true
Attachment #655006 - Flags: review?(mconley)
Comment on attachment 655006 [details] [diff] [review]
patch

This patch has bitrotted.

Can you please regenerate? I'm hoping to get this reviewed (and hopefully landed) today.
Attachment #655006 - Flags: review?(mconley) → review-
(Assignee)

Comment 43

5 years ago
Created attachment 655668 [details] [diff] [review]
patch
Attachment #655006 - Attachment is obsolete: true
Attachment #655668 - Flags: review?(mconley)
(In reply to alta88 from comment #40)
> (In reply to Mike Conley (:mconley) from comment #39)
> > Comment on attachment 653355 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 653355 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > 
> > We prefer let over var
> > 
> 
> of course.  but what is the policy for touching one statement in a huge file
> full of vars?  mix/match seems odd.  and suite reviewers strongly dislike
> mass var -> let changes.

Just don't insert new ones. If you touch one, change it to let. It's like asbestos. :)

> 
> yes, but the code was left in case the feed specific rendering bug is ever
> done.
> remove?

Yes please - or, if you find that the code might be useful in the future, comment it out and explain why it's not being used.
(Assignee)

Comment 45

5 years ago
Created attachment 655701 [details] [diff] [review]
patch
Attachment #655668 - Attachment is obsolete: true
Attachment #655668 - Flags: review?(mconley)
Attachment #655701 - Flags: review?(mconley)
(Assignee)

Comment 46

5 years ago
???
(In reply to alta88 from comment #46)
> ???

I'll finish the review today. We can then request approval to have it uplifted to Aurora.
Comment on attachment 655701 [details] [diff] [review]
patch

This patch gives me a broken TB on trunk with the following in the error console:

Timestamp: 12-08-29 12:14:21 PM
Error: SyntaxError: syntax error
Source File: chrome://messenger/content/mailWindowOverlay.js
Line: 233
Source Code:
} 

Timestamp: 12-08-29 12:14:23 PM
Error: ReferenceError: gPrefBranch is not defined
Source File: chrome://messenger/content/msgMail3PaneWindow.js
Line: 169

Timestamp: 12-08-29 12:14:23 PM
Error: TypeError: pref is undefined
Source File: chrome://messenger/content/threadPane.js
Line: 284
Attachment #655701 - Flags: review?(mconley) → review-
(Assignee)

Comment 49

5 years ago
Created attachment 656497 [details] [diff] [review]
patch


breakage from your appmenu patch which trampled on/bitrotted this patch.
Attachment #655701 - Attachment is obsolete: true
Attachment #656497 - Flags: review?(mconley)
This patch does not apply cleanly - 1 hunk is failing in mailWindowOverlay.js.
(Assignee)

Comment 51

5 years ago
Created attachment 656500 [details] [diff] [review]
patch


once more.
Attachment #656497 - Attachment is obsolete: true
Attachment #656497 - Flags: review?(mconley)
Attachment #656500 - Flags: review?(mconley)
Comment on attachment 656500 [details] [diff] [review]
patch

Review of attachment 656500 [details] [diff] [review]:
-----------------------------------------------------------------

Code looks good, but I'm getting some failing Mozmill tests locally, under folder-display in particular.

Pushing to try to get the full story.  Results coming in here: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=9ad55a2a0acb
Comment on attachment 656500 [details] [diff] [review]
patch

Here's a report of the failing tests:

https://tbpl.mozilla.org/php/getParsedLog.php?id=14815022&tree=Thunderbird-Try
Attachment #656500 - Flags: review?(mconley) → review-
(Assignee)

Comment 54

5 years ago
if it's in folderDisplay, then it's undoubtedly due to tests being triggered by displayMessageChanged().  that's where the message is hidden to prevent double content loads and a very displeasing 'flash', not just for feeds but any messagepane change (old content remains after threadpane selection has visibly changed).

just guessing, but the tests should trigger on messageDisplay.onLoadCompleted().

so what is is the solution?  it's not a code problem.
(In reply to alta88 from comment #54)
> 
> so what is is the solution?  it's not a code problem.

I'm poking at it now, hopefully a solution today.
alta88:

Ok, so I'm tackling the failures in test-tabs-simple.js first.

For these, I think we're hitting a race - I'm getting this in my error console when hitting one of the test failures:

Timestamp: 12-08-30 10:21:57 AM
Error: [Exception... "'TypeError: getBrowser(...).contentDocument.body is null' when calling method: [nsIMsgDBViewCommandUpdater::displayMessageChanged]"  nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)"  location: "JS frame :: chrome://messenger/content/messageDisplay.js :: MessageDisplayWidget_makeActive :: line 409"  data: no]
Source File: chrome://messenger/content/messageDisplay.js
Line: 409

Here is the code in the test:

messageB = select_click_row(0);
// Let's focus the message pane now
focus_message_pane();
tabMessageB = open_selected_message_in_new_tab();
assert_selected_and_displayed(messageB);
assert_message_pane_focused();

If I cause the event loop to spin for a few seconds before we open the selected message in the tab, this test passes, as well as the rest of the tests in the file.

Any idea why this might be happening?
(Assignee)

Comment 57

5 years ago
right, because of the one line in displayMessageChanged()
+    getBrowser().contentDocument.body.hidden = true;

this prevents the user from seeing the stale content 'flash/mismatch' (as soon as possible, on the js side), and is automatically (as part of a DOM load result) set back to .hidden = false.

however, the test code triggers before .hidden = true again (so the document is null) thus the errors.  spinning the loop makes the timing work.

as theorized above, making that part of the test run async,  after messageDisplay.onLoadCompleted(), might fix the timing.
(Assignee)

Comment 58

5 years ago
in fact, this may indicate that the test successfully tests for the *prior* document, and not the newly selected not-yet-loaded message.  (without getting into the whole design/reuse of messagepane across tabs etc etc issue).

the test would always pass if the 3pane message is the same as the new tab message, as it seems it doesn't test the actual load of the tab message in messagepane.  and it doesn't test/wouldn't pass the case of a rt click on a non selected message open in a new tab, since those would always differ until the messagepane load.  

a theory..
Hm.

So I've been poking at this, and it's been made abundantly clear to me that I'm pretty ignorant about our message display code, and how it operates.

I understand that we want to wait for messageDisplay.onLoadCompleted() to finish - ie, when messageDisplay.messageLoaded = true.

The select_click_row(0); function appears to be key. It's defined here: http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#919

In the case of this test (test_open_message_b_in_tab), the currently selected message was already 0, and we select_click_row it again, so hasMessageDisplay is true but willDisplayMessage is set to false.

Then we call wait_for_message_display_completion, which is defined here: http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#1550

This is the function that, I guess, isn't waiting enough. Can you see what we need to keep waiting for in wait_for_message_display_completion?
(Assignee)

Comment 60

5 years ago
it seems to be a function of mozmill testing that it be purely linear, and so spin event loops rather than execute on an event fire.  so there may not be a prewritten way to actually have a test wait on the load event asynchronously.

perhaps setting the sleep(0) in wait_for_message_display_completion to something like 20 or 40 or whatever number makes this synchronous test pass is the only way.  if, in fact, the waitFor function returns immediately; it has a high default timeout otherwise.
(In reply to alta88 from comment #60)
> it seems to be a function of mozmill testing that it be purely linear, and
> so spin event loops rather than execute on an event fire.  so there may not
> be a prewritten way to actually have a test wait on the load event
> asynchronously.

Sure there is - the pattern generally goes:

let done = false

some_async_function(function callback(aResult) {
  // Check aResult
  done = true;
});

mc.waitFor(function() done);

But do you see what we need to do to make wait_for_message_display_completion work correctly? Simply forcing it to wait for messageLoaded = true does not seem to suffice.
(Assignee)

Comment 62

5 years ago
but isn't the waitFor simply spinning 50x 100ms intervals (default) until the done is set?  it's sort of semantic; all 'push' is really fast and frequent 'pull', in some layer.

onLoadCompleted() is called by onEndMsgDownload in messageHeaderSink:
http://mxr.mozilla.org/comm-central/source/mail/base/content/msgHdrViewOverlay.js#669
which is notified by
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgStatusFeedback.cpp#131

displayMessageChanged() is called by nsMsgDBView::UpdateDisplayMessage 
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgDBView.cpp#1226

the messenger->openURL is called first, meaning that in this test it could be possible that onLoadCompleted is finished before displayMessageChanged.  because it's not coming from disk and is a fake 'in memory' message?

so messageLoading = true could do it.
(Assignee)

Comment 63

5 years ago
mike, if you comment out
+    getBrowser().contentDocument.body.hidden = true;
do the tests pass locally for you?
(In reply to alta88 from comment #63)
> mike, if you comment out
> +    getBrowser().contentDocument.body.hidden = true;
> do the tests pass locally for you?

Yes they do.
(Assignee)

Comment 65

5 years ago
Created attachment 657292 [details] [diff] [review]
patch


i think rather than trying to figure out the tests, we should get the patch in as it fixes some bad feed UX.  the content mismatch is comparatively less critical and can be revisited separately.
Attachment #656500 - Attachment is obsolete: true
Attachment #657292 - Flags: review?(mconley)
Comment on attachment 657292 [details] [diff] [review]
patch

I agree. Good stuff.
Attachment #657292 - Flags: review?(mconley) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/4b79ea516c52
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0

Updated

5 years ago
Blocks: 787612
Standard8 correctly identified this as the cause of some additional Mozmill failures. I had thought that we'd gotten them all, but I was wrong. Sorry about that.

Here are the traces:

SUMMARY-UNEXPECTED-FAIL | test-search-window.js | test-search-window.js::test_open_single_search_result_in_tab
  EXCEPTION: There should be 2 tabs open, but there are actually 1 tabs open. Tabs: [undefined undefined]
    at: test-folder-display-helpers.js line 113
       testHelperModule.do_throw test-folder-display-helpers.js 113
       mark_failure logHelper.js 620
       assert_number_of_tabs_open test-folder-display-helpers.js 770
       test_open_single_search_result_in_tab test-search-window.js 122
       Runner.wrapper frame.js 582
       Runner._runTestModule frame.js 652
       Runner.runTestModule frame.js 698
       Runner.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge._execFunction server.js 179
       Bridge.execFunction server.js 183
SUMMARY-UNEXPECTED-FAIL | test-search-window.js | test-search-window.js::test_open_multiple_search_results_in_new_tabs
  EXCEPTION: Timed out waiting for message display completion.
    at: utils.js line 447
       TimeoutError utils.js 447
       waitFor utils.js 485
       wait_for_message_display_completion test-folder-display-helpers.js 1596
       test_open_multiple_search_results_in_new_tabs test-search-window.js 150
       Runner.wrapper frame.js 582
       Runner._runTestModule frame.js 652
       Runner.runTestModule frame.js 698
       Runner.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge._execFunction server.js 179
       Bridge.execFunction server.js 183
SUMMARY-UNEXPECTED-FAIL | test-search-window.js | test-search-window.js::test_open_search_result_in_new_window
  EXCEPTION: Timed out waiting for window open!
    at: utils.js line 447
       TimeoutError utils.js 447
       waitFor utils.js 485
       WindowWatcher_waitForWindowOpen test-window-helpers.js 265
       wait_for_new_window test-window-helpers.js 561
       test_open_search_result_in_new_window test-search-window.js 187
       Runner.wrapper frame.js 582
       Runner._runTestModule frame.js 652
       Runner.runTestModule frame.js 698
       Runner.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge._execFunction server.js 179
       Bridge.execFunction server.js 183
SUMMARY-UNEXPECTED-FAIL | test-search-window.js | test-search-window.js::test_open_search_result_in_existing_window
  EXCEPTION: Timed out waiting for window open!
    at: utils.js line 447
       TimeoutError utils.js 447
       waitFor utils.js 485
       WindowWatcher_waitForWindowOpen test-window-helpers.js 265
       wait_for_new_window test-window-helpers.js 561
       test_open_search_result_in_existing_window test-search-window.js 208
       Runner.wrapper frame.js 582
       Runner._runTestModule frame.js 652
       Runner.runTestModule frame.js 698
       Runner.runTestDirectory frame.js 522
       runTestDirectory frame.js 704
       Bridge._execFunction server.js 179
       Bridge.execFunction server.js 183
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 69

5 years ago
if the msgHdr is not a feed it doesn't go into the code, so this is odd.

is it possible an overlay script is not getting included for tests?  the old code imported
-          Services.scriptloader.loadSubScript("chrome://messenger-newsblog/content/utils.js");

while the new code adds it to mailWindowOverlay.xul.  so it wouldn't be available, and throw, in a search window?

in retrospect, getBrowser() may not always get anything, so trying to get its contentDocument may have thrown.  are throws logged or do tests catch and timeout?
(Assignee)

Comment 70

5 years ago
Created attachment 657685 [details] [diff] [review]
patch


fixed typo, which didn't throw anywhere (tests or console) and made itself hard to find.
Attachment #657292 - Attachment is obsolete: true
Attachment #657685 - Flags: review?(mconley)
(Assignee)

Comment 71

5 years ago
can we please finish this..
Comment on attachment 657685 [details] [diff] [review]
patch

Bit-rotted again. :/

Sorry this keeps happening - couldn't review last week due to MozCamp.
Attachment #657685 - Flags: review?(mconley) → review-
(Assignee)

Comment 73

5 years ago
but you had time to checkin the patch that bitrotted this one?

i've had it with the incredible lack of review etiquette and consideration for others' work by the gatekeepers around here.

so you fix it.. or not.
(In reply to alta88 from comment #73)
> but you had time to checkin the patch that bitrotted this one?
> 
> i've had it with the incredible lack of review etiquette and consideration
> for others' work by the gatekeepers around here.
> 
> so you fix it.. or not.

Which bug bitrotted this patch, do you know off hand?
(Assignee)

Comment 75

5 years ago
Created attachment 663004 [details] [diff] [review]
rotted by Bug 784676
Attachment #663004 - Flags: review?(mconley)
Comment on attachment 663004 [details] [diff] [review]
rotted by Bug 784676

Ok, I like it. :)

I know you've been through a bit of a tumble here, alta88. Thank you for your patience, and your work.
Attachment #663004 - Flags: review?(mconley) → review+
Attachment #657685 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/dbd153b0fa9c
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 78

5 years ago
I don't know much about the issues here, but this seems like a significant fix that we should consider for TB 17.
tracking-thunderbird17: --- → ?
(Assignee)

Updated

5 years ago
Blocks: 532071
(Assignee)

Updated

5 years ago
Depends on: 531397
(Assignee)

Updated

5 years ago
Blocks: 531397
No longer depends on: 531397
(Assignee)

Comment 79

5 years ago
Comment on attachment 663004 [details] [diff] [review]
rotted by Bug 784676

[Approval Request Comment]
Bug caused by (feature/regressing bug #): comment 7.
User impact if declined: several visible bad UX problems will remain.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): low.
String or UUID changes made by this patch: 

this will require bug 787475 to follow it along.
Attachment #663004 - Flags: approval-mozilla-aurora?
Depends on: 793427
No longer depends on: 793427
Comment on attachment 663004 [details] [diff] [review]
rotted by Bug 784676

Looks like you want approval-comm-aurora here, changing the flag.
Attachment #663004 - Flags: approval-mozilla-aurora? → approval-comm-aurora?
Attachment #663004 - Flags: approval-comm-aurora? → approval-comm-aurora+
Checked in: https://hg.mozilla.org/releases/comm-aurora/rev/328fbe81793e
status-thunderbird17: --- → fixed
tracking-thunderbird17: ? → ---

Updated

4 years ago
Depends on: 887763
You need to log in before you can comment on or make changes to this bug.