724704 - Use 1000-byte kilobytes on Mac

Status: NEW

(MailNews Core :: Backend, defect)

Description

[Jim Porter (:squib)]

Mac uses SI units (i.e. 1 KB = 1000 bytes)*. Whenever possible, we should use the platform's conventions, so we should have 1000-byte kilobytes on Mac, but 1024 on Windows and Linux**. All we really need is an ifdef here (at FormatFileSize): http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgUtils.cpp#506

Things I will resist doing unless Someone Important demands it:

1) Use "KiB" (no one knows what that is)
2) Add a pref for this (toggling between 1024- and 1000-byte kilobytes is silly
   when most of the world except for Apple and hard drive manufacturers uses the
   former for discussing storage; you'd really want to toggle between "KB" and
   "KiB", but that's not what this bug is about)


*  Technically, Mac should be using kB (lowercase k), but oh well.
** Linux is all over the place, but Ubuntu (and probably all of GNOME) uses
   1024-byte KB, so let's go with that.

Comment 1

[kozhevin.dima]

I want to try to fix it

Comment 2

[kozhevin.dima]

Attachment: first try to fix bug

Probably fix it.Please review.

Comment 3

[Josh Matthews [:jdm]]

Comment on attachment 677994 [details]
first try to fix bug

Unfortunately we can't review full files. You'll need to create a diff file that just shows the changes you made; learn how at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_diff_and_patch_files.3F

Comment 4

[kozhevin.dima]

Attachment: possible patch

possible patch

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 678157 [details] [diff] [review]
possible patch

Jim's a good person to look at this, since I know nothing about the MailNews code and he's the mentor here.

Comment 6

[Jim Porter (:squib)]

Comment on attachment 678157 [details] [diff] [review]
possible patch

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

Overall, this looks good. However, we'll need tests for this. You can expand upon the tests here: http://mxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_formatFileSize.js

I'm not sure what the best way to check in the unit test for what platform you're running on. I'm sure someone on IRC would know though; the server is irc.mozilla.org and the channel is #maildev.

::: mailnews/base/util/nsMsgUtils.cpp
@@ +495,5 @@
>    float unitSize = size;
>    uint32_t unitIndex = 0;
>  
> +#ifdef XP_MACOSX
> +    float KiB = 1000;

We should probably make this a const and call it something like "unitScale". "KiB" doesn't make sense here, since we're not using KiB on OS X.

Comment 7

[Jim Porter (:squib)]

Comment on attachment 678157 [details] [diff] [review]
possible patch

Oops. r- for now, until we get unit tests.

Comment 8

[kozhevin.dima]

>> const and call it something like "unitScale"
i agree with you.

unit test fails?

Comment 9

[Jim Porter (:squib)]

(In reply to kozhevin.dima from comment #8)
> unit test fails?

I haven't had a chance to run the unit test against your patch yet, but looking at the test file, I can see a couple of spots are are sure to fail.

Comment 10

[ashwin_sreenivas]

Attachment: This is a possible patch, but I don't quite understand what useKB checks for. But based on the code I've read, this should work.

Comment 11

[Jim Porter (:squib)]

Comment on attachment 8351365 [details] [diff] [review]
This is a possible patch, but I don't quite understand what useKB checks for. But based on the code I've read, this should work.

I can't actually review this, since it's not a patch. I'll upload a patchified version here shortly.

Comment 12

[Jim Porter (:squib)]

Attachment: Use 1000-byte KB on Mac (no tests)

Here's a patch for this. No tests yet. That's where our enterprising community members come in now! :)

Here's a try run, which I expect will fail, since we assume 1024-byte KB in our tests: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=ce6112fd0bbd

Comment 13

[Jim Porter (:squib)]

Attachment: Use 1000-byte KB on OS X (no tests)

Whoops, I uploaded the wrong version of the patch

Comment 14

[Jim Porter (:squib)]

The next step here is for someone to fix these test failures from the TBPL logs:

TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/mailnews/base/test/unit/test_formatFileSize.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/talos-slave/test/build/xpcshell/tests/mailnews/base/test/unit/test_formatFileSize.js | "10.2 KB" == "10.0 KB" - See following stack:

Some additional tests might also be nice.

Comment 15

[Jim Porter (:squib)]

The tests will probably need to have a check for what OS they're being run on, since OS X will behave differently with this patch. I think the following will work (not tested):

  var xulRuntime = Components.classes["@mozilla.org/xre/app-info;1"]
                   .getService(Components.interfaces.nsIXULRuntime);
  var isOSX = xulRuntime.OS == "Darwin";

Then you can use `isOSX` to change the tests so that there are slightly-different tests on OSX and non-OSX.

Comment 16

[Kushal Singh]

Hello,

I would like to start working on this issue as one of my first issues. From what it seems to me so far, the patch is already done and we just want to fix the failures for the unit tests (or are we supposed to write a couple of more unit tests ? ). Feel free to correct me.

Also, the url in comment #12 isn't really working, so can someone guide me about the next steps and how I can reproduce the errors here [tbpl logs etc], since I am a bit new.

Thanks !
- Kushal