Closed Bug 1133009 Opened 9 years ago Closed 9 years ago

filterlog.html shows error console message (first open of empty log)

Categories

(MailNews Core :: Filters, defect)

Product:
MailNews Core
Component:
Filters
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

(thunderbird38 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 38.0
Tracking Flags:
Tracking Status
thunderbird38 --- fixed

People

(Reporter: rkent, Assigned: aceman)

References

Details

Attachments

(1 file, 2 obsolete files)

1133009.patch v1.2
9.90 KB, patch
aceman
: review+
Details | Diff | Splinter Review 
Timestamp: 2/13/2015 11:24:03 AM
Error: The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol.
Source File: file:///C:/Users/Kent/AppData/Roaming/Thunderbird/Profiles/atun7lqf.test2@dryrain.org/ImapMail/mail.messagingengine-1.com/filterlog.html
Line: 0
When was that file created? When I open my filterlog.html it does contain charset declaration (utf-8). Or is it a regression that we no longer write the charset in recent versions?
This was a fairly new profile, running a local comm-central build about 2 weeks old.
OK, I can see the message. If you "clear log" the file gets truncated to 0 bytes so no header. Then the message appears as the filter log dialog still shows it.
But when the first filter hits and a message is logged in the filter log, the charset header is emited.

So what do you propose? Do we emit the header as soon as filterlog.html is created or reset?
I don't have a proposed solution. I just want us to rid of all of these extraneous error console messages.
(In reply to :aceman from comment #3) 
> So what do you propose? Do we emit the header as soon as filterlog.html is
> created or reset?

Yes, if it's an html file rendered it should be valid html, not empty.
There are some more improvements to be done, as the file starts with <meta> so probably isn't valid html even if we fix the charset.

In the file nsMsgFilterList.cpp there is:
#define LOG_HEADER "<head><meta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-8\"><style type=\"text/css\">body{font-family:Consolas,\"Lucida Console\",Monaco,\"Courier New\

But this still isn't the header that I see in the file (when log hits are already logged there). So there is something wrong going on. I can look at it if you don't mind.
Attached patch 1133009.patch (obsolete) — — Splinter Review 
This could do it. I ensure that after return of GetLogFile or GetLogUrl the file does exist and has the proper header. And the caller does not need to care about that (e.g. in viewLog.js). Is that fine? Or are there any scenarios where we just want the filename but it should not exist yet? Outside of nsIMsgFilterList of course.
Do we care that the HTML is not properly closed (</body></html>) ? Or only after the HTML core starts complaining about that? :)
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Attachment #8564399 - Flags: review?(rkent)
Attachment #8564399 - Flags: review?(mkmelin+mozilla)
Severity: normal → minor
OS: Windows 8.1 → All
Hardware: x86_64 → All
(In reply to Kent James (:rkent) from comment #0)
> Error: The character encoding of the HTML document was not declared. (snip)

FYI.
If Firefox(34.0.5), this error can easily be observed by pretty simple local HTML.
 (1) <html><head><!-- <meta http-equiv="Content-Type" Content="text/html; charset=iso-8859-1"> --></head>
Following is shown at Web Console,
> The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations 
> if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document
>  or in the transfer protocol. image.html
(2) <html><head><meta http-equiv="Content-Type" Content="text/html; charset=iso-8859-1"></head>
No error message.

If local HTML file, Content-Type: heder is not sent from server. So this error message is perhaps shown.
I don't remember when this error started to occur.
Comment on attachment 8564399 [details] [diff] [review]
1133009.patch

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

::: mailnews/base/search/src/nsMsgFilterList.cpp
@@ +24,5 @@
>  #include "mozilla/ArrayUtils.h"
>  #include <ctype.h>
>  
> +#define FILTERLOG_PERMS 0644
> +

I don't think you should define this separetely here. The correct perms are 0666 which gives you permissions according to system umask.

::: mailnews/base/src/nsSpamSettings.cpp
@@ +197,5 @@
>    mLogStream = aLogStream;
>    return NS_OK;
>  }
>  
> +#define LOG_HEADER "<html>\n<head>\n<meta http-equiv=\"Content-Type\" content=\"text/html; charset=utf-8\">\n<style type=\"text/css\">body{font-family:Consolas,\"Lucida Console\",Monaco,\"Courier New\",Courier,monospace;font-size:small}</style>\n</head>\n<body>\n"

please add <!DOCTYPE html> before <html> too while you're at it
please quote utf-8 too as all the other attrs are quoted
Attachment #8564399 - Flags: review?(mkmelin+mozilla) → review+
Attached patch 1133009.patch v1.1 (obsolete) — — Splinter Review 
While I think the utf-8 was quoted correctly (together with text/html), I changed it to the shorter HTML5 form using <meta charset="utf-8"> .
Attachment #8564399 - Attachment is obsolete: true
Attachment #8564399 - Flags: review?(rkent)
Attachment #8564732 - Flags: review?(rkent)
Comment on attachment 8564732 [details] [diff] [review]
1133009.patch v1.1

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

r=me with requested change (compile fails in Windows without this change).

::: mailnews/base/search/src/nsMsgFilterList.h
@@ +67,5 @@
>  
>  private:
>    nsresult TruncateLog();
>    nsresult GetLogFile(nsIFile **aFile);
> +  nsresult EnsureLogFile(nsIFile *file);

By doing this, you now need to change in nsMsgFilterList.cpp:

-NS_IMETHODIMP nsMsgFilterList::EnsureLogFile(nsIFile *file)
+nsresult nsMsgFilterList::EnsureLogFile(nsIFile *file)
Attachment #8564732 - Flags: review?(rkent) → review+
Interesting that it compiled on Linux fine.
Attachment #8564732 - Attachment is obsolete: true
Attachment #8565216 - Flags: review+
Keywords: checkin-needed
Pushed as https://hg.mozilla.org/comm-central/rev/157f78f86aad
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-thunderbird38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
Regressions: 1789244
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: