672453 - Whine to console when an HTML file falls off the 'good' encoding detection path

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, enhancement)

Description

[Henri Sivonen (:hsivonen)]

To make it easier for Web authors to discover that their pages are broken, the HTML parser should warn to console when the following condition is met
 * There's no HTTP-level charset
AND
 * There's no BOM
AND
 * The meta prescan failed.

Furthermore, the parser should emit an error to the console when an encoding change restart occurs.

According to http://blogs.msdn.com/b/ieinternals/archive/2011/07/18/optimal-html-head-ordering-to-avoid-parser-restarts-redownloads-and-improve-performance.aspx , IE tells authors about restarts.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Fix without tests

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: Whine to console about encoding-related authoring errors

The changes to tests that look unrelated are meant to silence these console messages for other tests that listen to the console and expect to find their own messages.

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Whine to console about encoding-related authoring errors, with test changes

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 582851 [details] [diff] [review]
Whine to console about encoding-related authoring errors, with test changes

Do we really want to do this?
I would assume this will cause *lots* of warnings, even so much that no
one will care.

At least add a pref to enable/disable this and then we could
evaluate whether this approach is feasible.

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #4)
> Do we really want to do this?
> I would assume this will cause *lots* of warnings, even so much that no
> one will care.

The warnings triggered for sites authored by someone else will be annoying, sure, but they will still be fewer than the warnings the CSS parser spits out.

I think the warnings are useful for the person who has authored the pages that trigger the warnings. If you consider the case where the person reading the warnings is diagnosing pages (s)he wrote, this patch will add console messages only when the author has made a mistake (and the mistake is super-easy to fix). Contrast this with the CSS parser which outputs tons of warnings about stuff that the author did deliberately.

> At least add a pref to enable/disable this and then we could
> evaluate whether this approach is feasible.

How about I instead disable the "EncNoDeclaration" message for framed docs--i.e. different-origin frames?

AFAICT, ad iframes are the most noisy case and with those the console message is pretty useless anyway, since the author of the page that contains the ad iframes probably isn't empowered to adjust the ad server's output anyway.

The messages other than "EncNoDeclaration" will be rare enough that I think it would be an overreaction to limit them in any way (beyond the patch already limiting the messages to at most one message per document).

Comment 6

[Henri Sivonen (:hsivonen)]

smaug, ping. Would this be r+-worthy if I disabled EncNoDeclaration for framed docs?

As mentioned above, these warnings/errors would be much fewer than the flood of warnings about -webkit-foo CSS properties we currently have.

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Whine to console about encoding-related authoring errors, except about different-origin frames lacking a declaration altogether

This patch implements the tweak proposed above. This patch is silent in normal browsing use at least for me.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 608239 [details] [diff] [review]
Whine to console about encoding-related authoring errors, except about different-origin frames lacking a declaration altogether

Could you use <meta charset="utf-8">, not <meta charset=utf-8>
consistently

"When viewed viewed" -> When viewed

mAlreadyComplainedAboutCharset is initialized to false somewhere?

Use C++ casting, not C (char*)

We can try this, but I'm not sure whether this will stick.

Comment 9

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Could you use <meta charset="utf-8">, not <meta charset=utf-8>
> consistently

Done.

> "When viewed viewed" -> When viewed

Done.

> mAlreadyComplainedAboutCharset is initialized to false somewhere?

Zeroing operator new.

> Use C++ casting, not C (char*)

Done.

> We can try this, but I'm not sure whether this will stick.

Thanks. Landed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2342271b2be1

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2342271b2be1

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Fix intermittent orange

Intermittent oranges like <https://tbpl.mozilla.org/php/getParsedLog.php?id=10359487&tree=Firefox#error0> are no fun.

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 609156 [details] [diff] [review]
Fix intermittent orange

Oops. Sorry. Thanks.

Comment 13

[Masatoshi Kimura [:emk]]

(In reply to Henri Sivonen (:hsivonen) from comment #6)
> As mentioned above, these warnings/errors would be much fewer than the flood
> of warnings about -webkit-foo CSS properties we currently have.
-webkit-foo properties (and any vendor-prefixed properties other than -moz-foo) are not warned to the console, BTW.

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to Masatoshi Kimura [:emk] from comment #13)
> (In reply to Henri Sivonen (:hsivonen) from comment #6)
> > As mentioned above, these warnings/errors would be much fewer than the flood
> > of warnings about -webkit-foo CSS properties we currently have.
> -webkit-foo properties (and any vendor-prefixed properties other than
> -moz-foo) are not warned to the console, BTW.

When did that change?

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Henri Sivonen (:hsivonen) from comment #14)
> (In reply to Masatoshi Kimura [:emk] from comment #13)
> > (In reply to Henri Sivonen (:hsivonen) from comment #6)
> > > As mentioned above, these warnings/errors would be much fewer than the flood
> > > of warnings about -webkit-foo CSS properties we currently have.
> > -webkit-foo properties (and any vendor-prefixed properties other than
> > -moz-foo) are not warned to the console, BTW.
> 
> When did that change?

It's been that way forever, IIRC. We only warn for unprefixed and moz-prefixed stuff we don't recognize.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Thanks, checked in

https://hg.mozilla.org/integration/mozilla-inbound/rev/63900b73be92

Comment 17

[Matt Brubeck (:mbrubeck)]

Sorry, the follow-up 63900b73be92 was backed out on inbound because of mochitest-plain-5 failures:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7da46c0c739c
https://tbpl.mozilla.org/php/getParsedLog.php?id=10375447&tree=Mozilla-Inbound

Comment 18

[Matt Brubeck (:mbrubeck)]

The test has been disabled because of frequent failures (bug 739354):
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b3d2762c0fb

Comment 19

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/63900b73be92
https://hg.mozilla.org/mozilla-central/rev/7da46c0c739c
https://hg.mozilla.org/mozilla-central/rev/6b3d2762c0fb