324291 - Reporter should record the character encoding used to display the page

Status: RESOLVED FIXED

(Other Applications Graveyard :: Reporter, enhancement)

Description

[Jesse Ruderman]

Some odd behavior on web sites depends on what character encoding Firefox decided to use to display the page.  For example, a problem might appear only when the page is interpreted as being UTF-8.  It would be nice if reporter.mozilla.org told me the encoding in addition to telling me the URL.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: client side patch

Needs appropriate server side changes.

Comment 2

[Robert Accettura [:raccettura]]

Asa, I'd appreciate your opinion on this one.  As well as anyone else you think would be good to chime.

It's a reasonable idea, but I'm not sure we need this.  I'd like to see some evidence that this is widespread enough for it to help.

I'm against a situation where we start capture an insane amount of data, for very seldom use.  Especially data we can get ourselves (information about the page itself).

If this is something that would help identify a large number of all rendering issues, then yes, this may be worth while.

Otherwise, you should note there is a problem via reports in reporter, take a look at the reported pages, and file a bug in bugzilla with an appropriate test case and note the encoding there.  Soon reporter will be able to point to bugzilla bugs related to that hostname, which will further facilitate this.

reporter isn't an alternative to bugzilla.  It's simply a conduit for users to communicate with developers without the complexities of a system like bugzilla and needing to gather information.  The information we gather is about the client at the moment in time (when visiting the page) so we can reproduce the problem.

What pushes me towards saying it's unneeded is that it's server side metadata. i.e. anyone checking the page to confirm the issue can find it out very easily.  If it were a client side option/configuration, then I'd be more inclined to take the data, as it wouldn't be something we could gather otherwise.  For example, if knowing intl.charset.default would help in many cases, then it would be something to include.

I am actively coding screenshot support, which will allow users to show us the problem.  This would help in identification of actual issues as well.

It's not clear to me that there is a real burden by not capturing the page encoding.  Perhaps there is.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

It isn't server side data in some cases - the user may have specified a charset manually, leading to the page being displayed incorrectly. The value captured is the charset being used to display the page, not the charset specified by the page.

Comment 4

[Jesse Ruderman]

When a page doesn't specify a charset, Firefox picks one based on all kinds of wacky heuristics, including the frequency distribution of bytes in the page (which might change slightly) and the charset of the linking page.

Comment 5

[Robert Accettura [:raccettura]]

Ok, I'm about 80% agreeing on this addition.

Gavin, the patch looks fine (I need to do the server side obviously).  

I'm working on screenshot support as well.  I'd like to time this bug to land together with that.  

A few things to consider here:
- Are we capturing the interpreted encoding?  Or the page encoding?  I presume interpreted.
- If the encoding is coming from external sources (i.e. from the page), I'd prefer that the case be the same (always upper, or always lower).  It's not critical, but would be nice.  If it's set internally, whatever the browser does is fine.

Reassign->gavin as he's got a patch

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #5)
> - Are we capturing the interpreted encoding?  Or the page encoding?  I presume
> interpreted.

Correct. It's what the browser is using to display the page.

> - If the encoding is coming from external sources (i.e. from the page), I'd
> prefer that the case be the same (always upper, or always lower).  It's not
> critical, but would be nice.  If it's set internally, whatever the browser does
> is fine.

The case should be consistent, since this is the browser's internal representation of the charset.

Comment 7

[Robert Accettura [:raccettura]]

That's exactly what I wanted. :-D

Comment 8

[:Gavin Sharp [email: [e-mail]]]

This mostly needs server-side work, so it shouldn't really be assigned to me.
-> Default owner

Comment 9

[Robert Accettura [:raccettura]]

Gavin: if you don't mind, file a webtools:reporter bug for the server side.  I've got it in the back of my mind anyway.

I'd rather keep them separate so I can see what's done, and what needs to be done.

Comment 10

[Robert Accettura [:raccettura]]

This is a little late, and we don't support this on the server, but we could land it since the server will ignore what it doesn't understand... then when we upgrade the server, it will start recording charset of reports.

Gavin, care to chime in on this plan?

Comment 11

[Robert Accettura [:raccettura]]

Comment on attachment 209260 [details] [diff] [review]
client side patch

I've reviewed this

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Sounds like a fine plan to me.

Comment 13

[Robert Accettura [:raccettura]]

Comment on attachment 209260 [details] [diff] [review]
client side patch

r=me

We likely want to add the results to the report detail for viewing on completion.

Comment 14

[Robert Accettura [:raccettura]]

Comment on attachment 209260 [details] [diff] [review]
client side patch

We should also take this for 1.8.1 if we can.  No string change, we can leave this out of the details view.  It's not really important.

Comment 15

[Mike Schroepfer]

Comment on attachment 209260 [details] [diff] [review]
client side patch

We are closing in on the RC1 code freeze and are only taking bugs that match the criteria for 181 approvals:

http://developer.mozilla.org/devnews/index.php/2006/08/28/firefox-2gecko-181-approvals-2/

Thanks for putting this together and since it is on the trunk we'll pickup in a future release.

Comment 16

[Robert Accettura [:raccettura]]

Attachment: Update Gavin's Patch

Updated to fix bitrot.  I could have sworn this was checked in on the trunk a while ago.  My mistake.

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 287272 [details] [diff] [review]
Update Gavin's Patch

>Index: resources/content/reporter/reportWizard.js

>     'buildconfig':      gParamBuildConfig,
>     'language':         gParamLanguage,
>     'email':            gParamEmail,
>+    'charset':          gCharset,
>     'sysid':            sysId
>   };

If this patch had more context (-u8 next time? :) you'd see a "SOAP Params" comment above this block that should probably be changed.

r=me, though I'm assuming you've tested it, because I haven't.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 287272 [details] [diff] [review]
Update Gavin's Patch

a=beltzner for M9, but we won't hold release for this

Comment 19

[Robert Accettura [:raccettura]]

(In reply to comment #17)
> r=me, though I'm assuming you've tested it, because I haven't.

Did, looks fine just like the other patch.

(In reply to comment #18)
> (From update of attachment 287272 [details] [diff] [review])
> a=beltzner for M9, but we won't hold release for this
> 

Ditto.  I can checkin later this week when I have time to be on the hook for a few hours.

Comment 20

[Reed Loden [:reed]]

I changed "// SOAP params" to just "// params".

Checking in extensions/reporter/locales/en-US/chrome/reportResults.dtd;
/cvsroot/mozilla/extensions/reporter/locales/en-US/chrome/reportResults.dtd,v  <--  reportResults.dtd
new revision: 1.3; previous revision: 1.2
done
Checking in extensions/reporter/resources/content/reporter/report.xhtml;
/cvsroot/mozilla/extensions/reporter/resources/content/reporter/report.xhtml,v  <--  report.xhtml
new revision: 1.3; previous revision: 1.2
done
Checking in extensions/reporter/resources/content/reporter/reportWizard.js;
/cvsroot/mozilla/extensions/reporter/resources/content/reporter/reportWizard.js,v  <--  reportWizard.js
new revision: 1.31; previous revision: 1.30
done
Checking in extensions/reporter/resources/content/reporter/reporterOverlay.js;
/cvsroot/mozilla/extensions/reporter/resources/content/reporter/reporterOverlay.js,v  <--  reporterOverlay.js
new revision: 1.10; previous revision: 1.9
done