492329 - Polish the error message for an expired newsgroup article

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Vlado Valastiak [:wladow]]

Attachment: screenshot

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b5pre) Gecko/20090510 Shredder/3.0b3pre

When there is an expired newsgroup article, Shredder sets up the error message by joining 2 sentenses together, but with no space between them:

Error!
newsgroup server responded:no such article in group
                  ^^^^^^^^^^^

Comment 1

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

Attachment: article not found mockup

If someone is going to work on this I'd suggest some other changes as well.

Start with the Firefox 'problem loading page' and clean this whole thing up.  (see attached)

Comment 2

[Richard Marti (:Paenglab)]

In which file is this message generated?

Comment 3

[Magnus Melin [:mkmelin]]

Richard: http://mxr.mozilla.org/comm-central/search?string=htmlNewsError

Comment 4

[Joshua Cranmer [:jcranmer]]

<http://mxr.mozilla.org/comm-central/source/mailnews/news/src/nsNNTPProtocol.cpp?mark=1990-2052#1989> contains the source code for the construction of the HTML, with some of the contents for strings being found in localized properties files.

Comment 5

[Richard Marti (:Paenglab)]

Unfortunately c++ is totally out of my possibilities.

I think if "chrome://global/skin/netError.css" could be included here as stylesheet, this could already give the needed polish without other big changes.

Comment 6

[Jim Porter (:squib)]

This would be nice. Question: should we also go ahead and make this page work like about:neterror[1]? That would probably be a step up from generating HTML in C++.

[1] http://mxr.mozilla.org/comm-central/source/mozilla/docshell/resources/content/netError.xhtml

Comment 7

[David :Bienvenu]

(In reply to Jim Porter (:squib) from comment #6)
> This would be nice. Question: should we also go ahead and make this page
> work like about:neterror[1]? That would probably be a step up from
> generating HTML in C++.
Sure, seems reasonable to me.

Comment 8

[Jim Porter (:squib)]

Attachment: Prettify the "article not found" page

Here's a patch to do this. The result should be identical to the mockup (except for no bold text on the server response).

Comment 9

[Jim Porter (:squib)]

Oh, I couldn't test the "Remove expired articles" button, since it didn't seem to do anything before the patch either... if someone else can get it to work, it's probably worth testing just to make sure it still works (content policy might bite us).

Comment 10

[[e-mail]]

Comment on attachment 576695 [details] [diff] [review]
Prettify the "article not found" page

Is there an easy way to test this?

>+          document.getElementById("errorTryAgain").hidden = true;
hidden doesn't work in HTML.

>+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
>+                       escapedResponse))
Please use MsgEscapeURL from nsMsgUtils.h

>+      uri.Trim("&", false, true, false);
Please can you use the 3-argument version of Trim.

Comment 11

[Jim Porter (:squib)]

(In reply to [e-mail] from comment #10)
> Is there an easy way to test this?

The only way I have to test it is from some articles on mozilla.support.thunderbird, which is actually what prompted me to fix this. It's probably not super helpful, but if you filter on messages by "Ron Hunter", there should be a few recent articles that trigger this. Specifically, "Re: Attaching a shortcut to an email", ID <[e-mail]> works for me to test.

> >+          document.getElementById("errorTryAgain").hidden = true;
> hidden doesn't work in HTML.

It's listed in the global attributes for HTML5 (granted, this page is XHTML, so I might be violating the spec anyway): https://developer.mozilla.org/en/HTML/Global_attributes

> >+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
> >+                       escapedResponse))
> Please use MsgEscapeURL from nsMsgUtils.h

Is there a reason this is preferred over NS_EscapeURL?

> >+      uri.Trim("&", false, true, false);
> Please can you use the 3-argument version of Trim.

I assume that's the same, but just without the last argument?

Two other things:

1) I noticed that I forgot to add the about: redirection for Seamonkey, so I'll need to
   do that.
2) There appear to be only two other places that DisplayHTMLInMessagePane is used (SMIME
   decryption errors and no cached bodies in offline mode). Maybe we should fix these
   while we're here too. I could either a) make separate files for them, or b) move
   newsError.xhtml to mailnewsError.xhtml and incorporate those errors into that page.
   Then we could deprecate and ultimately remove DisplayHTMLInMessagePane (which just
   loads a data: URI anyway).

Comment 12

[[e-mail]]

(In reply to Jim Porter from comment #11)
> (In reply to from comment #10)
> > Is there an easy way to test this?
> The only way I have to test it is from some articles on
> mozilla.support.thunderbird, which is actually what prompted me to fix this.
> It's probably not super helpful, but if you filter on messages by "Ron
> Hunter", there should be a few recent articles that trigger this.
> Specifically, "Re: Attaching a shortcut to an email", ID
> <[e-mail]> works for me to test.
Hmm, but if it's expired, then I won't have it in my message list, will I?

> > >+          document.getElementById("errorTryAgain").hidden = true;
> > hidden doesn't work in HTML.
> It's listed in the global attributes for HTML5
Aha, it was added by bug 567663. Thanks for pointing that out.

> > >+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
> > >+                       escapedResponse))
> > Please use MsgEscapeURL from nsMsgUtils.h
> Is there a reason this is preferred over NS_EscapeURL?
Yes, it works with --with-libxul-sdk.

> > >+      uri.Trim("&", false, true, false);
> > Please can you use the 3-argument version of Trim.
> I assume that's the same, but just without the last argument?
Yes, except when using --with-libxul-sdk which only supports three arguments.

> Then we could deprecate and ultimately remove DisplayHTMLInMessagePane
Sounds like a followup bug.

Comment 13

[Jim Porter (:squib)]

(In reply to [e-mail] from comment #12)
> (In reply to Jim Porter from comment #11)
> > (In reply to from comment #10)
> > > Is there an easy way to test this?
> > The only way I have to test it is from some articles on
> > mozilla.support.thunderbird, which is actually what prompted me to fix this.
> > It's probably not super helpful, but if you filter on messages by "Ron
> > Hunter", there should be a few recent articles that trigger this.
> > Specifically, "Re: Attaching a shortcut to an email", ID
> > <[e-mail]> works for me to test.
> Hmm, but if it's expired, then I won't have it in my message list, will I?

I'll attach my .msf file for the newsgroup. That should hopefully make it possible to test this.

> > Then we could deprecate and ultimately remove DisplayHTMLInMessagePane
> Sounds like a followup bug.

Fair enough.

Comment 14

[Jim Porter (:squib)]

Attachment: My .msf file for m.s.thunderbird

Here's an .msf file which should have an expired article. Filter on "Ron Hunter attachment" with the quick filter (on subject/sender) to find the expired article.

Comment 15

[:aceman]

Hi. Could you please file the followup bug for removing DisplayHTMLInMessagePane . It supposedly would fix bug 57115. Thanks.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 576695 [details] [diff] [review]
Prettify the "article not found" page

I mostly like it, but I've got a couple of questions.

For reference, there's a screenshot of the patch in action at http://dl.dropbox.com/u/2301433/Glass/MissingNewsgroupArticle.png

1) Why not bold the server response?
2) Now that we've landed OpenSearch, I think it would be nice for the message id after "Try searching for article: " to be a link to actually search for that article.

But even without those, it's a big step forward, so ui-r=me.

Thanks,
Blake.

Comment 17

[David :Bienvenu]

Attachment: de-bitrotted patch

Comment 18

[David :Bienvenu]

Comment on attachment 597623 [details] [diff] [review]
de-bitrotted patch

nit - no space after nsCOMPtr
+  nsCOMPtr <nsIDocShell> docShell;
+  GetMessageWindowDocShell(getter_AddRefs(docShell));

you could pass &rv to do_QueryInterface and return the actual rv instead of NS_ERROR_FAILURE with NS_ENSURE_SUCCESS(rv, rv);

+  nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(docShell));
+  NS_ENSURE_TRUE(webNav, NS_ERROR_FAILURE);

I tried this in TB and it is a nice improvement. Removing expired articles button didn't work, however, which is why I've minused this. We start the url, but we never get into the nntp protocol state machine. I don't think content policy is kicking us out (which is usually the issue with things like this), but I'm not sure why we're not getting into the state machine. Did it work for you?

Comment 19

[David :Bienvenu]

nsNntpService::HandleContent is canceling the url - I'll poke around a bit.

Comment 20

[Jim Porter (:squib)]

(In reply to David :Bienvenu from comment #18)
> I tried this in TB and it is a nice improvement. Removing expired articles
> button didn't work, however, which is why I've minused this. We start the
> url, but we never get into the nntp protocol state machine. I don't think
> content policy is kicking us out (which is usually the issue with things
> like this), but I'm not sure why we're not getting into the state machine.
> Did it work for you?

It doesn't, but it never worked before either, so I figured I was just doing something wrong (as a user). Maybe not.

Comment 21

[David :Bienvenu]

the code broke at some point, nothing at all to do with your patch from what I can tell. I'll fix it and attach a qrefreshed patch.

Comment 22

[David :Bienvenu]

Attachment: fix list-ids

this gets the button working for me. I think jcranmer regressed this with his newsuri fix

Comment 23

[[e-mail]]

Comment on attachment 597871 [details] [diff] [review]
fix list-ids

>+      // this is all we need for listing newsgroup ids, I think.
>+      if (!PL_strcasecmp(aContentType, "x-application-newsgroup-listids"))
>+        return rv;
Does this need to go on branches? If so maybe a separate bug?

Comment 24

[David :Bienvenu]

(In reply to [e-mail] from comment #23)
> Comment on attachment 597871 [details] [diff] [review]
> fix list-ids
> 
> >+      // this is all we need for listing newsgroup ids, I think.
> >+      if (!PL_strcasecmp(aContentType, "x-application-newsgroup-listids"))
> >+        return rv;
> Does this need to go on branches? If so maybe a separate bug?

Yeah, I think the regression is on one or more branches. OK, filed bug 727951

Comment 25

[[e-mail]]

Comment on attachment 597871 [details] [diff] [review]
fix list-ids

>+      nsString uri(NS_LITERAL_STRING("about:newserror?"));
>+      uri.AppendLiteral("r=");
about:newserror?r=
Might be worth putting this in an nsCString instead, makes the appends easier.

>+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
>+                       escapedResponse))
>+        uri.Append(NS_ConvertASCIItoUTF16(escapedResponse));
>+      else
>+        uri.Append(NS_ConvertASCIItoUTF16(m_responseText));
MsgEscapeString(m_responseText, nsINetUtil::ESCAPE_QUERY, escapedResponse);
etc. please.

>+      uri.AppendLiteral("&");
Don't do this here, you might not need it.

>         rv = m_newsFolder->GetMessageIdForKey(m_key, messageID);
>-        if (NS_SUCCEEDED(rv)) {
Why assign rv if you're not checking it?

>+        uri.AppendLiteral("m=");
Instead, append &m= here etc.

>+      uri.Trim("&", false, true, false);
Then this becomes unnecessary.

Comment 26

[[e-mail]]

(In reply to comment #25)
>(From update of attachment 597871 [details] [diff] [review])
>>+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
>>+                       escapedResponse))
>>+        uri.Append(NS_ConvertASCIItoUTF16(escapedResponse));
>>+      else
>>+        uri.Append(NS_ConvertASCIItoUTF16(m_responseText));
>MsgEscapeString(m_responseText, nsINetUtil::ESCAPE_QUERY, escapedResponse);
Should be ESCAPE_URL_QUERY of course.

Comment 27

[[e-mail]]

(In reply to comment #25)
>(From update of attachment 597871 [details] [diff] [review])
>>+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
>>+                       escapedResponse))
>>+        uri.Append(NS_ConvertASCIItoUTF16(escapedResponse));
>>+      else
>>+        uri.Append(NS_ConvertASCIItoUTF16(m_responseText));
>MsgEscapeString(m_responseText, nsINetUtil::ESCAPE_QUERY, escapedResponse);
Not my day, is it? This needs to be nsDependentCString(m_responseText), but the other two cases are OK because they are already nsCString objects.

Comment 28

[Jim Porter (:squib)]

I think I may have a local patch with some of these changes that I neglected to post, since I got busy with BigFiles. We'll see when I'm at my dev box...

Comment 29

[[e-mail]]

(In reply to comment #25)
>(From update of attachment 597871 [details] [diff] [review])
>>+      if (NS_EscapeURL(m_responseText, strlen(m_responseText), esc_Query,
>>+                       escapedResponse))
>>+        uri.Append(NS_ConvertASCIItoUTF16(escapedResponse));
>>+      else
>>+        uri.Append(NS_ConvertASCIItoUTF16(m_responseText));
>MsgEscapeString(m_responseText, nsINetUtil::ESCAPE_QUERY, escapedResponse);
Should be MsgEscapeURL of course. Oops.

Comment 30

[[e-mail]]

Comment on attachment 597871 [details] [diff] [review]
fix list-ids

>+    "newserror": {url: "chrome://messenger/content/newsError.xhtml",
>+                flags: Ci.nsIAboutModule.ALLOW_SCRIPT},
Hmm, I guess we should implement something like this; currently we have eight separate about handler files, so this would be the ninth... although there is something to be said about having this handler in a shared location.

>+<!ENTITY articleNotFound.desc "The newsgroup server reports that it can't find the article">
Needs trailing full stop.

>+<!ENTITY articleExpired.title "Perhaps the article has expired">
Needs trailing question mark.

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 597871 [details] [diff] [review]
fix list-ids

Cancelling review request - from what I can tell in Neil's comments, this patch needs some updates before we can move forward.

Comment 32

[[e-mail]]

(In reply to comment #30)
>(From update of attachment 597871 [details] [diff] [review])
>>+    "newserror": {url: "chrome://messenger/content/newsError.xhtml",
>>+                flags: Ci.nsIAboutModule.ALLOW_SCRIPT},
>Hmm, I guess we should implement something like this; currently we have
>eight separate about handler files, so this would be the ninth... although
>there is something to be said about having this handler in a shared location.
SeaMonkey has now switched to a single nsAbout.js for all of its handlers, so it would be helpful if you could include it in your next patch. Thanks.

Comment 33

[Richard Marti (:Paenglab)]

Attachment: patch updated to tip

I've updated the to tip and it is still working. :)

Jim, if you have time, please could you look at Neil's comments and finish this patch? IMHO I think it is not so much to do to finish it.

Comment 34

[Richard Marti (:Paenglab)]

Adding n-i.

Comment 35

[Jim Porter (:squib)]

Resetting assignee since I have no immediate plans of working on this.

Comment 36

[Richard Marti (:Paenglab)]

Attachment: patch updated again

Updated to apply on tip.

Comment 37

[Lars Rohwedder]

Patch 8704505 results in a compiler error on Linux:

https://pastebin.mozilla.org/8855953

PRUnichar is uint16_t on Linux which is a different type than char16_t, of course. Which one is the right one here?

Comment 38

[aleth [:aleth]]

(In reply to Lars Rohwedder from comment #37)
> PRUnichar is uint16_t on Linux which is a different type than char16_t, of
> course. Which one is the right one here?

You could look at the definition of webNav->LoadURI to find out what is expected here.

Comment 39

[Jorg K (CEST = GMT+2)]

Attachment: article-not-found.patch

Rebased.

Comment 40

[Jorg K (CEST = GMT+2)]

Attachment: article-not-found.patch (JK v2)

Made it compile.

Comment 41

[Jorg K (CEST = GMT+2)]

On an expired article the current patch shows an empty screen with this error in the error console:

Security Error: Content at moz-nullprincipal:{dd24085b-bc16-4419-af75-e1caa8a7de6c} may not load or link to about:newserror?r=no%20such%20article%20in%20group%20(3451%203452-3460)&m=[e-mail]&f=news://news.mozilla.org/mozilla.test.multimedia.

That error URL needs to be loaded with a system principal, updated patch coming.

Comment 42

[Jorg K (CEST = GMT+2)]

Attachment: 492329-article-not-found.patch (JK v3)

This is working now. I've shuffled the principals around a bit.

I clicked on the link and the expired articles disappeared, so working as designed.

Comment 43

[Jorg K (CEST = GMT+2)]

Attachment: 492329-article-not-found.patch (JK v4)

OK, I've addressed the nits from comment #25, so as far as I can see, this is ready to go.

Sadly, I have blown away all my expired articles, so I can't test it any more. Alfred, can you please test this for me.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: 492329-article-not-found.patch (JK v4b).

Oops, cut&paste error.

Alfred, maybe you can also look over the code to see whether you can spot any problems.

Comment 45

[Alfred Peters [:infofrommozilla]]

Comment on attachment 8880948 [details] [diff] [review]
492329-article-not-found.patch (JK v4b).

I do not see any obvious problems.
Work as intended.

Just a nit:

We fetch the articles via the article number (m_key).
My server reports accordingly: "no such article number in this group"

So far we have the number also reported.

| -    PR_snprintf(outputBuffer, OUTPUT_BUFFER_SIZE,"<P>&lt;%.512s&gt; (%lu)", messageID.get(), m_key);

Comment 46

[Jorg K (CEST = GMT+2)]

Attachment: 492329-article-not-found.patch (JK v5)

Alfred, thanks for looking at this. This version should now satisfy the most demanding news reader ;-)

Comment 47

[Alfred Peters [:infofrommozilla]]

Comment on attachment 8881028 [details] [diff] [review]
492329-article-not-found.patch (JK v5)

Excellent!

For the normal user, the information is of no interest.
But, if the message ID index of the server is broken, 'ARTICLE <m_key>' and ARTICLE <message-id>' can produce different results.

Comment 48

[Jorg K (CEST = GMT+2)]

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