Page Info shows incorrect size, not honouring locale settings. I want to notice that many European countries use commas (not periods) as a decimal separator. This must be fixed in Mozilla App Suite and Mozilla Firefox.

Use Number.toLocaleString() (bug 165200) when displaying the file size and image dimensions in Properties and Page Info windows. Let me know if I have missed something.

Comment on attachment 153618 [details] [diff] [review] patch for metaData.js and pageInfo.js for App Suite and Firefox good catch. r=db48x

Comment on attachment 153618 [details] [diff] [review] patch for metaData.js and pageInfo.js for App Suite and Firefox >+ document.getElementById("physSize").value = theBundle.getFormattedString("mediaPhysSize", [formatNumber(physWidth), formatNumber(physHeight)]); You should wrap these lines rather than letting them get this long. >+function formatNumber(numberStr) >+{ >+ return numberStr.toLocaleString(); >+} What's the point of this? why not just .toLocaleString() on the number?

Comment on attachment 153618 [details] [diff] [review] patch for metaData.js and pageInfo.js for App Suite and Firefox I'm sorry, I didn't test this enough. not everything that needs to be formated is still a number, so change the function to look like this: function formatNumber(number) { number -= 0; return number.toLocaleString(); } that will coerce it back to a number so that toLocaleString() never returns "[object String]". With that change you have my r=.

Comment on attachment 153618 [details] [diff] [review] patch for metaData.js and pageInfo.js for App Suite and Firefox >Index: browser/base/content/metaData.js >=================================================================== >RCS file: /cvsroot/mozilla/browser/base/content/metaData.js,v >retrieving revision 1.1 >diff -u -d -p -8 -r1.1 metaData.js >--- browser/base/content/metaData.js 17 Oct 2003 18:00:36 -0000 1.1 >+++ browser/base/content/metaData.js 18 Jul 2004 21:55:10 -0000 I think you'll find that metadata.js is all lower case. Please don't confuse the CVS server! >+function formatNumber(numberStr) >+{ >+ return numberStr.toLocaleString(); >+} As db48x says, sometimes the number provided here isn't a number, it's a string. In which case, it could be something like 100%, which the existing code fails miserably with (it reports 100%px)... The cases that you know are a number should use .toLocaleString directly, and then you can write a function to handle <object type="image/gif" src="about:logo" width="50%" height="200"> - always assuming you're worried about images over 999 pixels.

(In reply to comment #5) > (From update of attachment 153618 [details] [diff] [review]) > >Index: browser/base/content/metaData.js > >=================================================================== > >RCS file: /cvsroot/mozilla/browser/base/content/metaData.js,v > >retrieving revision 1.1 > >diff -u -d -p -8 -r1.1 metaData.js > >--- browser/base/content/metaData.js 17 Oct 2003 18:00:36 -0000 1.1 > >+++ browser/base/content/metaData.js 18 Jul 2004 21:55:10 -0000 > I think you'll find that metadata.js is all lower case. Please don't confuse > the CVS server! No, the case in the patch is correct, it's the tree that has different case for App Suite (mozilla/xpfe) and Firefox (mozilla/browser). :-) My patch was not altered in any way and is the direct output of a 'cvs diff' command. :-) > >+function formatNumber(numberStr) > >+{ > >+ return numberStr.toLocaleString(); > >+} > As db48x says, sometimes the number provided here isn't a number, it's a > string. In which case, it could be something like 100%, which the existing code > fails miserably with (it reports 100%px)... The cases that you know are a > number should use .toLocaleString directly, and then you can write a function > to handle <object type="image/gif" src="about:logo" width="50%" height="200"> - > always assuming you're worried about images over 999 pixels. But isn't that another bug? :-) Do we have different per cent symbols for different localisations?

% isn't allowed in that attribute even though mozilla technically allows it. That said, I've never seen page info show 50%px, since the width and height properties just return the computed value.

You can try it with this url to check that it still works in this case: data:text/html;charset=utf-8;base64,PGltZyBoZWlnaHQ9IjUwJSIgd2lkdGg9IjUwJSIgc3JjPSJodHRwOi8vd3d3Lm1vemlsbGEub3JnL2ltYWdlcy9tb3ppbGxhLWJhbm5lci5naWYiPg0K

just chekcked in, marking bug 19492 and it's dependants fixed

(In reply to comment #9) > just chekcked in, marking bug 19492 and it's dependants fixed You meant bug 195492, did you not? :-) Reopening, and correcting the summary to reflect the attached patch. The attached patch in this bug also contains a fix for 'metaData.js' and 'metadata.js', whilst the attachments in bug 195492 and the check-in only contain fixes for 'pageInfo*'. I'll try to take care about this bug and the patch sometime soon. Thanks.

Seems this bug haven't fixed yet. Constantine, are you going to fix it? If you are not, I can make the patch.

Constantine seems not to be listening. Behnam, I'm assigning the bug to you: if you agree, please "Accept", else reassign it to nobody@mozilla.org.

Here are an update to the problem. I'm going to put a copy of new formatNumber() from infopage.js to metadata.js and use it. Just wondering shouldn't such a function be somewhere global accessible?

Updated the title. Should I make patches for both Firefox and Application Suite together, or they must be separated with separate bugs?

Changes in pageInfo.js is just to clean up the function usage and to prevent using formated-number string by mistake in the future.

Comment on attachment 322375 [details] [diff] [review] patch for metaData.js and pageInfo.js for Firefox 3.0 Why don't you ask a Firefox peer for review on this patch? It doesn't have anything SM-specific.

Comment on attachment 322375 [details] [diff] [review] patch for metaData.js and pageInfo.js for Firefox 3.0 (In reply to comment #17) > Why don't you ask a Firefox peer for review on this patch? Doesn't db48x own page info?

(In reply to comment #18) > (From update of attachment 322375 [details] [diff] [review]) > (In reply to comment #17) > > Why don't you ask a Firefox peer for review on this patch? > Doesn't db48x own page info? I'm not sure, but I don't think so. Check out <http://www.mozilla.org/projects/firefox/review.html>. I guess the right person to ask here would be either mconnor, mano, or gavin.

(In reply to comment #17) > Why don't you ask a Firefox peer for review on this patch? It doesn't have > anything SM-specific. I just assigned previous reviewers, but then realized the last patch was reviewed on 2004. So feel free to re-assign them. [I don't know why, but I haven't got any bugmail in hours. I will take a look here later again.]

Yes, in general I'm quite happy to review Page Info changes, and the "real" peers don't mind. In this case, however, I'm on a trip and won't be able to get to it for a week or so. From a quick glance I'd say the patch is fine, but it is generally a good idea to patch both Firefox and Seamonkey at the same time. If you've got a spare moment could you go ahead and do that?

Again, changes in both pageInfo.js files is just to clean up the function usage and to prevent using formated-number string by mistake in the future. Would someone set the superreview please?

Comment on attachment 322611 [details] [diff] [review] patch for metaData.js and pageInfo.js for Firefox and Seamonkey Setting r/sr for SeaMonkey-specific parts of this patch.

BTW, for more information on SM review/super-review requirements, check out: <http://www.seamonkey-project.org/dev/review-and-flags> <http://www.seamonkey-project.org/dev/project-areas>

Comment on attachment 322611 [details] [diff] [review] patch for metaData.js and pageInfo.js for Firefox and Seamonkey There's some interesting indentation going in here caused by the use of tabs instead of spaces. sr=me with that fixed. Out of interest, which numbers are really strings?

(In reply to comment #25) > (From update of attachment 322611 [details] [diff] [review]) > There's some interesting indentation going in here caused by the use of tabs > instead of spaces. sr=me with that fixed. I use VIM and I thought it understands the guideline in the files correctly. My mistake.

Comment on attachment 322611 [details] [diff] [review] patch for metaData.js and pageInfo.js for Firefox and Seamonkey r=db48x. sorry it took me so long to get back to this.

Now that Firefox and SeaMonkey are in different (Mercurial) repositories I am updating the SeaMonkey part of the patch. (In Reply to Comment 25) > There's some interesting indentation going in here caused by the use of tabs > instead of spaces. sr=me with that fixed. Fixed. Due to the bit-rot asking for sr again.

Comment on attachment 388925 [details] [diff] [review] Patch Sv1.0 SeaMonkey part. p=Behnam Esfahbod r=db48x sr=neil >+function formatNumber(number) >+{ >+ // coerce number to a numeric value before calling toLocaleString() >+ return (+number).toLocaleString(); >+} Nit: this version should stay in sync with page info (or vice versa).

Carrying forward r+/sr+ >>+function formatNumber(number) >>+{ >>+ // coerce number to a numeric value before calling toLocaleString() >>+ return (+number).toLocaleString(); >>+} > Nit: this version should stay in sync with page info (or vice versa). Fixed.

Comment on attachment 389115 [details] [diff] [review] Patch Sv1.1 SeaMonkey part. p=Behnam Esfahbod r=db48x sr=neil [Checkin: Comment 31] http://hg.mozilla.org/comm-central/rev/790bf5348dbe

I've spun off the Firefox part into bug 507139. So I shall resolve this bug as fixed.

KaiRo, this is using toLocaleString, i.e., OS locale and not gecko locale. Is that what you want?

(In reply to comment #33) > KaiRo, this is using toLocaleString, i.e., OS locale and not gecko locale. > > Is that what you want? Actually, I think using the Gecko locale would be better. Probably warrants a followup bug.

Filed bug 542502, because I don't think we have any logic to do that right now.