Use 1000-byte kilobytes on Mac

NEW
Assigned to

Status

--
minor
7 years ago
2 years ago

People

(Reporter: squib, Assigned: ashwin_sreenivas, Mentored)

Tracking

Trunk
All
Mac OS X

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][lang=c++][patchlove])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

7 years ago
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.
(Reporter)

Updated

6 years ago
Whiteboard: [good first bug][lang=c++][mentor=squib]

Comment 1

6 years ago
I want to try to fix it

Comment 2

6 years ago
Created attachment 677994 [details]
first try to fix bug

Probably fix it.Please review.
Attachment #677994 - Flags: review?(josh)
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
Attachment #677994 - Attachment is patch: false
Attachment #677994 - Flags: review?(josh)

Comment 4

6 years ago
Created attachment 678157 [details] [diff] [review]
possible patch

possible patch
Attachment #678157 - Flags: review?(josh)
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.
Attachment #678157 - Flags: review?(josh) → review?(squibblyflabbetydoo)
(Reporter)

Comment 6

6 years ago
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.
(Reporter)

Comment 7

6 years ago
Comment on attachment 678157 [details] [diff] [review]
possible patch

Oops. r- for now, until we get unit tests.
Attachment #678157 - Flags: review?(squibblyflabbetydoo) → review-

Comment 8

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

unit test fails?
(Reporter)

Comment 9

6 years ago
(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.
OS: Linux → Mac OS X
Hardware: x86_64 → All
(Assignee)

Comment 10

5 years ago
Created 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.
Attachment #8351365 - Flags: review?(squibblyflabbetydoo)
Assignee: nobody → ashwin_sreenivas
(Reporter)

Comment 11

5 years ago
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.
Attachment #8351365 - Flags: review?(squibblyflabbetydoo) → review-
(Reporter)

Comment 12

5 years ago
Created attachment 8362730 [details] [diff] [review]
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
Attachment #677994 - Attachment is obsolete: true
Attachment #678157 - Attachment is obsolete: true
Attachment #8351365 - Attachment is obsolete: true
(Reporter)

Comment 13

5 years ago
Created attachment 8362780 [details] [diff] [review]
Use 1000-byte KB on OS X (no tests)

Whoops, I uploaded the wrong version of the patch
Attachment #8362730 - Attachment is obsolete: true
(Reporter)

Comment 14

5 years ago
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.
Mentor: squibblyflabbetydoo
Whiteboard: [good first bug][lang=c++][mentor=squib] → [good first bug][lang=c++]
(Reporter)

Comment 15

4 years ago
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.

Updated

2 years ago
Severity: normal → minor
Whiteboard: [good first bug][lang=c++] → [good first bug][lang=c++][patchlove]

Comment 16

2 years ago
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
You need to log in before you can comment on or make changes to this bug.