Last Comment Bug 781133 - Fix locale specific numbers in hung plugin CPU usage reporting
: Fix locale specific numbers in hung plugin CPU usage reporting
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: 17 Branch
: All Windows 7
: -- normal (vote)
: mozilla17
Assigned To: Georg Fritzsche [:gfritzsche]
:
Mentors:
Depends on:
Blocks: 764660
  Show dependency treegraph
 
Reported: 2012-08-08 04:29 PDT by Georg Fritzsche [:gfritzsche]
Modified: 2012-08-21 19:10 PDT (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix locale specific CPU usage numbers (1.84 KB, patch)
2012-08-08 07:32 PDT, Georg Fritzsche [:gfritzsche]
benjamin: review+
Details | Diff | Splinter Review
Fix locale specific CPU usage numbers (1.70 KB, patch)
2012-08-09 02:21 PDT, Georg Fritzsche [:gfritzsche]
benjamin: review+
Details | Diff | Splinter Review
Fix locale specific CPU usage numbers (1.71 KB, patch)
2012-08-20 10:23 PDT, Georg Fritzsche [:gfritzsche]
no flags Details | Diff | Splinter Review

Description Georg Fritzsche [:gfritzsche] 2012-08-08 04:29:21 PDT

    
Comment 1 Georg Fritzsche [:gfritzsche] 2012-08-08 07:32:21 PDT
Created attachment 650117 [details] [diff] [review]
Fix locale specific CPU usage numbers

Avoid writing locale specific numbers for the plugin CPU usage report.
Stop fixing locale specific numbers in the associated test.
Comment 2 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-08-08 10:03:27 PDT
Comment on attachment 650117 [details] [diff] [review]
Fix locale specific CPU usage numbers

I'm surprised that nsPrintfCString was locale-dependent (and that this function would be different). But if this works, let's do it!
Comment 3 Georg Fritzsche [:gfritzsche] 2012-08-08 10:15:05 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] [away 27-July until 7-Aug] from comment #2)
> I'm surprised that nsPrintfCString was locale-dependent (and that this
> function would be different). But if this works, let's do it!

Apparently nsPrintfCString behaves like printf() locale-wise, while PR_cnvtf() behaves like specified for ECMA script.
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-08-08 12:51:24 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8d6c79a074
Comment 5 Ryan VanderMeulen [:RyanVM] 2012-08-08 19:58:36 PDT
This got caught up in the fray with all the other backouts today.
https://hg.mozilla.org/integration/mozilla-inbound/rev/40e59c86c2b3

Re-landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecb6aaee9601
Comment 6 Phil Ringnalda (:philor, back in August) 2012-08-08 22:48:05 PDT
Backed out yet again in https://hg.mozilla.org/integration/mozilla-inbound/rev/beb7a4c0d06f

https://tbpl.mozilla.org/php/getParsedLog.php?id=14247588&tree=Mozilla-Inbound
4453 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/test_busy_hang.xul | got extra field for plugin cpu usage
4454 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/dom/plugins/test/test_busy_hang.xul | plugin cpu usage is >0%
Comment 7 Georg Fritzsche [:gfritzsche] 2012-08-09 02:21:35 PDT
Created attachment 650475 [details] [diff] [review]
Fix locale specific CPU usage numbers

Use nsCString::AppendFloat() to avoid local buffer management, round manually.

Try: https://tbpl.mozilla.org/?tree=Try&rev=7b06c9be10a3
Comment 8 Georg Fritzsche [:gfritzsche] 2012-08-09 08:24:34 PDT
Comment on attachment 650475 [details] [diff] [review]
Fix locale specific CPU usage numbers

Try build results are fine. Benjamin, can you do a follow review?
Comment 9 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-08-13 09:18:28 PDT
Comment on attachment 650475 [details] [diff] [review]
Fix locale specific CPU usage numbers

Why std::ceilf instead of ceilf? I'm not sure it matters either way, though.
Comment 10 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2012-08-13 09:29:57 PDT
From IRC discussion, this should either be ::ceilf or std::ceil, since std::ceilf is apparently a Windows-only weirdism.
Comment 11 Georg Fritzsche [:gfritzsche] 2012-08-20 10:23:04 PDT
Created attachment 653426 [details] [diff] [review]
Fix locale specific CPU usage numbers

Fixed std::ceilf() oversight.
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-08-21 03:38:13 PDT
Green on Try (the Android R2 failure is unrelated).
https://tbpl.mozilla.org/?tree=Try&rev=cfba471376c9

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cfd16fe1cb3
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-08-21 19:10:50 PDT
https://hg.mozilla.org/mozilla-central/rev/6cfd16fe1cb3

Note You need to log in before you can comment on or make changes to this bug.