967378 - Network statistics: don't rely on parseFloat to deal with numbers in the charts

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[Francesco Lodolo [:flod]]

https://hg.mozilla.org/mozilla-central/rev/6990058bde98

For reference: every time you use a number followed by a noun, you need to use a proper plural form.
https://developer.mozilla.org/en/docs/Localization_and_Plurals

Note that this is broken for en-US too (1 seconds).

Comment 1

[Girish Sharma [:Optimizer]]

While we should use proper plurals, its quiet rare to get an integer 1 there.

Also, what is the correct form in case of < 1 decimals ? (0.45) ?

Comment 2

[Francesco Lodolo [:flod]]

I don't see decimals, so far I've just seen an integer number between 0 and 2.
I'm talking about the graph, in the footer I see decimals but also "s", not "seconds".

Comment 3

[Girish Sharma [:Optimizer]]

Can you post a screenshot of what you are talking about ?

Normally, the footer should have the big "seconds" and the graph should have "s". Also, the number would be rarely decimal.

Comment 4

[Francesco Lodolo [:flod]]

Attachment: netmonitor.png

See red boxes near pie charts.

Comment 5

[Girish Sharma [:Optimizer]]

(In reply to Francesco Lodolo [:flod] from comment #4)
> Created attachment 8369896 [details]
> netmonitor.png
> 
> See red boxes near pie charts.

Hmm, then there is something wrong. That footer time should be sum of all the individual times in the table which is to the right of the pie chart.

It should be 0.49 and 3.68 resp. (At least, its for me)

Comment 6

[Francesco Lodolo [:flod]]

I would think about failure with the decimal separator, but then I don't understand why it's correctly displayed in the footer (top) or legends.

Comment 7

[Victor Porof [:vporof][:vp]]

Because to calculate the sums, we do a parseFloat on the required items in the table, and parseFloat doesn't work with comma decimal separators. Sad panda.

Comment 8

[Victor Porof [:vporof][:vp]]

In fact, if you take a look at "images" in the second chart, it should clearly be the largest slice, but it's actually pretty much invisible, because it's parsed as being 1.507 instead of 1507.48 :)

Relevant code is in here: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/Chart.jsm Should probably do something smarter than just simply parseFloat, but maintain the same convenient API.

Comment 9

[Girish Sharma [:Optimizer]]

lol :(

Comment 10

[Victor Porof [:vporof][:vp]]

I'll take care of this.

Comment 11

[Victor Porof [:vporof][:vp]]

Attachment: v1

This should do the trick.

Comment 12

[Francesco Lodolo [:flod]]

Comment on attachment 8369960 [details] [diff] [review]
v1

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

The original problem still persists (plural form). Can't we just move to use "s" even in this context?

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +2366,5 @@
>      window.emit(EVENTS.EMPTY_CACHE_CHART_DISPLAYED);
>    },
>  
>    /**
> +   * Common strigifier predicates used for items and totals in both the

typo (strigifier)?

Comment 13

[Girish Sharma [:Optimizer]]

(In reply to Francesco Lodolo [:flod] from comment #12)
> Comment on attachment 8369960 [details] [diff] [review]
> v1
> 
> Review of attachment 8369960 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The original problem still persists (plural form). Can't we just move to use
> "s" even in this context?

Not really, as I said, with decimals, plurals are used. Since we never show integers , but only decimals (even in case 1.00).

Is there a guideline of plurals wrt decimals ?

Comment 14

[Francesco Lodolo [:flod]]

(In reply to Girish Sharma [:Optimizer] from comment #13)
> Not really, as I said, with decimals, plurals are used. Since we never show
> integers , but only decimals (even in case 1.00).

Beware that not everything works the same as in English.

> Is there a guideline of plurals wrt decimals ?

I'm trying to find out. I might wrong but using the abbreviation is the fastest way out.

Comment 15

[Francesco Lodolo [:flod]]

CLDR Page
http://www.unicode.org/cldr/charts/latest/supplemental/language_plural_rules.html

PluralForm.js
mxr.mozilla.org/mozilla-central/source/intl/locale/src/PluralForm.jsm

So we support decimal numbers with plural forms.

Comment 16

[Victor Porof [:vporof][:vp]]

Attachment: v2, part 1

Fixed typo.

Comment 17

[Victor Porof [:vporof][:vp]]

Attachment: v2, part 2 (use plural form)

Comment 18

[Francesco Lodolo [:flod]]

Comment on attachment 8369984 [details] [diff] [review]
v2, part 2 (use plural form)

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

::: browser/devtools/netmonitor/netmonitor-view.js
@@ +2386,5 @@
>        return L10N.getFormatStr("charts.totalSize", string);
>      },
>      time: total => {
>        let string = L10N.numberWithDecimals(total / 1000, REQUEST_TIME_DECIMALS);
> +      return PluralForm.get(total, L10N.getStr("charts.totalSeconds")).replace("#1", string);

Not sure I'm reading it right, but aren't you displaying "total/1000" (e.g. 1.1), and ask the plural form for total (e.g. 1100)?

Comment 19

[Victor Porof [:vporof][:vp]]

You're right!

Comment 20

[Victor Porof [:vporof][:vp]]

Attachment: v2, part 2 (use plural form)

Comment 21

[Francesco Lodolo [:flod]]

Comment on attachment 8369988 [details] [diff] [review]
v2, part 2 (use plural form)

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

Switching to feedback since I'm not a real reviewer. This part looks good to me now.

Comment 22

[Girish Sharma [:Optimizer]]

Flod, as per your link from comment 15. Should we be using "seconds" in both the cases for en-US ?

Comment 23

[Francesco Lodolo [:flod]]

(In reply to Girish Sharma [:Optimizer] from comment #22)
> Flod, as per your link from comment 15. Should we be using "seconds" in both
> the cases for en-US ?

Not really. Singular form will just never be used for en-US unless the number is exactly 1, but "second" is correct.

Comment 24

[Girish Sharma [:Optimizer]]

Ah, so I guess, PluralForm.js takes care of this automatically ...

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63b5af5f0b76
https://hg.mozilla.org/mozilla-central/rev/fb9006ed8412