Report small distances as fractions of mm's, rather than um's. (issue1080001)

[dbat...@google.com]

Reviewers: mdsteele,



Please review this at http://page-speed-codereview.appspot.com/1080001/

Affected files:
M pagespeed/formatters/formatter_util.cc
M pagespeed/formatters/formatter_util_test.cc


Index: pagespeed/formatters/formatter_util_test.cc
===================================================================
--- pagespeed/formatters/formatter_util_test.cc (revision 2561)
+++ pagespeed/formatters/formatter_util_test.cc (working copy)
@@ -54,8 +54,10 @@
TEST(FormatterUtilTest, Distance) {
EXPECT_EQ("0mm", pagespeed::formatters::FormatDistance(-10));
EXPECT_EQ("0mm", pagespeed::formatters::FormatDistance(0));
- EXPECT_EQ("123um", pagespeed::formatters::FormatDistance(123));
- EXPECT_EQ("999um", pagespeed::formatters::FormatDistance(999));
+ EXPECT_EQ("0.01mm", pagespeed::formatters::FormatDistance(1));
+ EXPECT_EQ("0.12mm", pagespeed::formatters::FormatDistance(123));
+ EXPECT_EQ("0.13mm", pagespeed::formatters::FormatDistance(126));
+ EXPECT_EQ("1mm", pagespeed::formatters::FormatDistance(999));
EXPECT_EQ("1mm", pagespeed::formatters::FormatDistance(1000));
EXPECT_EQ("4.6mm", pagespeed::formatters::FormatDistance(4567));
EXPECT_EQ("3.9mm", pagespeed::formatters::FormatDistance(3949));
Index: pagespeed/formatters/formatter_util.cc
===================================================================
--- pagespeed/formatters/formatter_util.cc (revision 2561)
+++ pagespeed/formatters/formatter_util.cc (working copy)
@@ -44,7 +44,6 @@
};

const UnitDescriptor kDistances[] = {
- { 1000, "um" },
{ 1000, "mm" },
{ 1000, "m" },
{ -1, "km" },
@@ -119,7 +118,9 @@
if (micrometers <= 0) {
return "0mm";
}
- double distance = micrometers;
+
+ // The smallest unit used for output is a millimeter, not a micrometer.
+ double distance = micrometers / 1000.0;
const char* display_name = "";
for (size_t i = 0; i < kDistancesSize && distance > 0; ++i) {
const UnitDescriptor *desc = kDistances + i;
@@ -130,9 +131,23 @@
distance /= desc->quantity;
}

- // If the value is between 0 and 10, and the first decimal place is
- // non-zero, then show a single digit decimal value. Otherwise,
- // round to the nearest whole number.
+ // If the value is less than 0, and the second decimal place is non-zero,
+ // then show a two digit decimal value. Otherwise, if the value is
between
+ // 0 and 10 and the first decimal place is non-zero, then show a single
digit
+ // decimal value. Otherwise, round to the nearest whole number.
+ if (distance < 1) {
+ if (distance <= 0.01) {
+ // Make sure that the formatted version of any non-zero distance is
+ // non-zero by rounding up very small numbers.
+ return StringPrintf("0.01%s", display_name);
+ }
+ const int hundredths = static_cast<int>(round(distance * 100)) % 100;
+ if (hundredths > 0) {
+ // Round to nearest hundredth.
+ double rounded_distance = round(distance * 100) / 100;
+ return StringPrintf("%.2f%s", rounded_distance, display_name);
+ }
+ }
if (distance < 10) {
const int tenths = static_cast<int>(round(distance * 10)) % 10;
if (tenths > 0) {

[mdst...@google.com]

http://page-speed-codereview.appspot.com/1080001/diff/1/pagespeed/formatters/formatter_util.cc
File pagespeed/formatters/formatter_util.cc (right):

http://page-speed-codereview.appspot.com/1080001/diff/1/pagespeed/formatters/formatter_util.cc#newcode134
pagespeed/formatters/formatter_util.cc:134: // If the value is less than

0, and the second decimal place is non-zero,

I think maybe you meant "If the value is less than 1, and the first two
decimal places are not both zero,"?

http://page-speed-codereview.appspot.com/1080001/diff/1/pagespeed/formatters/formatter_util_test.cc
File pagespeed/formatters/formatter_util_test.cc (right):

http://page-speed-codereview.appspot.com/1080001/diff/1/pagespeed/formatters/formatter_util_test.cc#newcode58
pagespeed/formatters/formatter_util_test.cc:58: EXPECT_EQ("0.12mm",
pagespeed::formatters::FormatDistance(123));
Can you add a test for FormatDistance(100)? Is that supposed to come
out to "0.10mm" or "0.1mm"?

http://page-speed-codereview.appspot.com/1080001/

[dbat...@google.com]

http://page-speed-codereview.appspot.com/1080001/diff/1/pagespeed/formatters/formatter_util.cc
File pagespeed/formatters/formatter_util.cc (right):

http://page-speed-codereview.appspot.com/1080001/diff/1/pagespeed/formatters/formatter_util.cc#newcode134
pagespeed/formatters/formatter_util.cc:134: // If the value is less than
0, and the second decimal place is non-zero,

On 2013/11/19 19:58:26, mdsteele wrote:
> I think maybe you meant "If the value is less than 1, and the first
two decimal
> places are not both zero,"?

Reworked the comment, hopefully it makes more sense now.

http://page-speed-codereview.appspot.com/1080001/diff/1/pagespeed/formatters/formatter_util_test.cc
File pagespeed/formatters/formatter_util_test.cc (right):

http://page-speed-codereview.appspot.com/1080001/diff/1/pagespeed/formatters/formatter_util_test.cc#newcode58
pagespeed/formatters/formatter_util_test.cc:58: EXPECT_EQ("0.12mm",
pagespeed::formatters::FormatDistance(123));

On 2013/11/19 19:58:26, mdsteele wrote:
> Can you add a test for FormatDistance(100)? Is that supposed to come
out to
> "0.10mm" or "0.1mm"?

Done. Wanted it to come out as 0.1mm for consistency... and this test
caught a bug. Thanks :)

http://page-speed-codereview.appspot.com/1080001/

[mdst...@google.com]

LGTM


http://page-speed-codereview.appspot.com/1080001/diff/60001/pagespeed/formatters/formatter_util.cc
File pagespeed/formatters/formatter_util.cc (right):

http://page-speed-codereview.appspot.com/1080001/diff/60001/pagespeed/formatters/formatter_util.cc#newcode146
pagespeed/formatters/formatter_util.cc:146: const int hundredths =
static_cast<int>(round(distance * 100)) % 100 % 10;
Err, % 100 % 10? Shouldn't that just be % 10? And if I'm wrong and
both %'s are needed, can you at least add parentheses for those of us
who can't remember if % is left or right-associative in C++? (-:

http://page-speed-codereview.appspot.com/1080001/

[dbat...@google.com]

http://page-speed-codereview.appspot.com/1080001/diff/60001/pagespeed/formatters/formatter_util.cc
File pagespeed/formatters/formatter_util.cc (right):

http://page-speed-codereview.appspot.com/1080001/diff/60001/pagespeed/formatters/formatter_util.cc#newcode146
pagespeed/formatters/formatter_util.cc:146: const int hundredths =
static_cast<int>(round(distance * 100)) % 100 % 10;

On 2013/11/19 20:44:28, mdsteele wrote:
> Err, % 100 % 10? Shouldn't that just be % 10? And if I'm wrong and
both %'s
> are needed, can you at least add parentheses for those of us who can't
remember
> if % is left or right-associative in C++? (-:

Why yes, what I wrote was ridiculous.

http://page-speed-codereview.appspot.com/1080001/

[mdst...@google.com]

LGTM

Thanks for fixing this!

http://page-speed-codereview.appspot.com/1080001/