Closed
Bug 967378
Opened 10 years ago
Closed 10 years ago
Network statistics: don't rely on parseFloat to deal with numbers in the charts
Categories
(DevTools :: Netmonitor, defect)
DevTools
Netmonitor
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 30
People
(Reporter: flod, Assigned: vporof)
References
Details
(Whiteboard: [qa-])
Attachments
(3 files, 2 obsolete files)
446.50 KB,
image/png
|
Details | |
17.17 KB,
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
2.85 KB,
patch
|
rcampbell
:
review+
flod
:
feedback+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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) ?
Reporter | ||
Comment 2•10 years ago
|
||
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•10 years ago
|
||
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.
Reporter | ||
Comment 4•10 years ago
|
||
See red boxes near pie charts.
Comment 5•10 years ago
|
||
(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)
Reporter | ||
Comment 6•10 years ago
|
||
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.
Assignee | ||
Comment 7•10 years ago
|
||
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.
Assignee | ||
Comment 8•10 years ago
|
||
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•10 years ago
|
||
lol :(
Assignee | ||
Comment 10•10 years ago
|
||
I'll take care of this.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Summary: Network statistics: use proper plural form in charts.totalTime2 (netmonitor.properties) → Network statistics: don't rely on parseFloat to deal with numbers in the charts
Assignee | ||
Comment 11•10 years ago
|
||
This should do the trick.
Attachment #8369960 -
Flags: review?(rcampbell)
Reporter | ||
Comment 12•10 years ago
|
||
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•10 years ago
|
||
(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 ?
Reporter | ||
Comment 14•10 years ago
|
||
(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.
Reporter | ||
Comment 15•10 years ago
|
||
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.
Assignee | ||
Comment 16•10 years ago
|
||
Fixed typo.
Attachment #8369960 -
Attachment is obsolete: true
Attachment #8369960 -
Flags: review?(rcampbell)
Attachment #8369983 -
Flags: review?(rcampbell)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8369984 -
Flags: review?(rcampbell)
Attachment #8369984 -
Flags: review?(francesco.lodolo)
Reporter | ||
Comment 18•10 years ago
|
||
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)?
Reporter | ||
Updated•10 years ago
|
Attachment #8369984 -
Flags: review?(francesco.lodolo) → feedback-
Assignee | ||
Comment 19•10 years ago
|
||
You're right!
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8369984 -
Attachment is obsolete: true
Attachment #8369984 -
Flags: review?(rcampbell)
Attachment #8369988 -
Flags: review?(rcampbell)
Attachment #8369988 -
Flags: review?(francesco.lodolo)
Reporter | ||
Comment 21•10 years ago
|
||
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.
Attachment #8369988 -
Flags: review?(francesco.lodolo) → feedback+
Comment 22•10 years ago
|
||
Flod, as per your link from comment 15. Should we be using "seconds" in both the cases for en-US ?
Reporter | ||
Comment 23•10 years ago
|
||
(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•10 years ago
|
||
Ah, so I guess, PluralForm.js takes care of this automatically ...
Updated•10 years ago
|
Attachment #8369983 -
Flags: review?(rcampbell) → review+
Updated•10 years ago
|
Attachment #8369988 -
Flags: review?(rcampbell) → review+
Comment 25•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/63b5af5f0b76 https://hg.mozilla.org/mozilla-central/rev/fb9006ed8412
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Updated•10 years ago
|
Whiteboard: [qa-]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•