596234 - Body of RSS feeds won't print or print preview

Status: RESOLVED FIXED

(MailNews Core :: Printing, defect, P2)

Description

[Loeb]

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.

Comment 1

[Loeb]

Attachment: Screen shot of original RSS message

Comment 2

[Loeb]

Attachment: This is what prints

Comment 3

[Loeb]

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.

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

Confirming with both TB and SM trunk, Windows and Linux.

Comment 6

[alta88]

Attachment: patch.

Comment 7

[alta88]

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

Comment 8

[alta88]

Attachment: tweak to make it slightly safer.

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 617037 [details] [diff] [review]
tweak to make it slightly safer.

Too late for TB12, unless there is a chemspill.

Comment 10

[Mark Banner (:standard8)]

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+.

Comment 11

[alta88]

Attachment: 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.

Comment 12

[Mark Banner (:standard8)]

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 13

[Mark Banner (:standard8)]

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.

Comment 14

[Loeb]

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

Comment 15

[Loeb]

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

Comment 16

[Loeb]

Attachment: PDF Print demonstrating bug

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

Comment 17

[alta88]

(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.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

I can reproduce this on Daily. Examining patch now...

Comment 19

[Mike Conley (:mconley) (:⚙️)]

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

Comment 20

[alta88]

Attachment: Patch v8

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 626934 [details] [diff] [review]
Patch v8

Just giving me a larger link to click. ;)

Comment 22

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 23

[Lewis Rosenthal]

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.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

(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 25

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 26

[Lewis Rosenthal]

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).

Comment 27

[alta88]

Attachment: 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.

Comment 28

[Lewis Rosenthal]

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!

Comment 29

[alta88]

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 30

[Mike Conley (:mconley) (:⚙️)]

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");

Comment 31

[alta88]

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.

Comment 32

[alta88]

this is fixed in bug 728563.

Comment 33

[alta88]

Attachment: 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.

Comment 34

[alta88]

mike, a ping.  it would be good to get this in for 17.

Comment 35

[[e-mail]]

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.

Comment 36

[alta88]

Attachment: patch

updated patch with no effect on suite.

Comment 37

[Mike Conley (: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?

Comment 38

[alta88]

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 39

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 40

[alta88]

(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.

Comment 41

[alta88]

Attachment: 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.

Comment 42

[Mike Conley (: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.

Comment 43

[alta88]

Attachment: patch

Comment 44

[Mike Conley (: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.

Comment 45

[alta88]

Attachment: patch

Comment 46

[alta88]

???

Comment 47

[Mike Conley (:mconley) (:⚙️)]

(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 48

[Mike Conley (:mconley) (:⚙️)]

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

Comment 49

[alta88]

Attachment: patch

breakage from your appmenu patch which trampled on/bitrotted this patch.

Comment 50

[Mike Conley (:mconley) (:⚙️)]

This patch does not apply cleanly - 1 hunk is failing in mailWindowOverlay.js.

Comment 51

[alta88]

Attachment: patch

once more.

Comment 52

[Mike Conley (: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 53

[Mike Conley (:mconley) (:⚙️)]

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

Comment 54

[alta88]

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.

Comment 55

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 56

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 57

[alta88]

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.

Comment 58

[alta88]

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..

Comment 59

[Mike Conley (:mconley) (:⚙️)]

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?

Comment 60

[alta88]

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.

Comment 61

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 62

[alta88]

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.

Comment 63

[alta88]

mike, if you comment out
+    getBrowser().contentDocument.body.hidden = true;
do the tests pass locally for you?

Comment 64

[Mike Conley (:mconley) (:⚙️)]

(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.

Comment 65

[alta88]

Attachment: 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.

Comment 66

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 657292 [details] [diff] [review]
patch

I agree. Good stuff.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/4b79ea516c52

Comment 68

[Mike Conley (:mconley) (:⚙️)]

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

Comment 69

[alta88]

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?

Comment 70

[alta88]

Attachment: patch

fixed typo, which didn't throw anywhere (tests or console) and made itself hard to find.

Comment 71

[alta88]

can we please finish this..

Comment 72

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 657685 [details] [diff] [review]
patch

Bit-rotted again. :/

Sorry this keeps happening - couldn't review last week due to MozCamp.

Comment 73

[alta88]

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.

Comment 74

[Mike Conley (:mconley) (:⚙️)]

(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?

Comment 75

[alta88]

Attachment: rotted by Bug 784676

Comment 76

[Mike Conley (: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.

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/dbd153b0fa9c

Comment 78

[Kent James (:rkent)]

I don't know much about the issues here, but this seems like a significant fix that we should consider for TB 17.

Comment 79

[alta88]

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.

Comment 80

[Lukas Blakk [:lsblakk] use ?needinfo]

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

Looks like you want approval-comm-aurora here, changing the flag.

Comment 81

[Mark Banner (:standard8)]

Checked in: https://hg.mozilla.org/releases/comm-aurora/rev/328fbe81793e