Closed
Bug 905759
Opened 11 years ago
Closed 11 years ago
move telemetry sending from java into native code
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: froydnj, Unassigned)
Details
Attachments
(3 files)
2.25 KB,
patch
|
vladan
:
review+
|
Details | Diff | Splinter Review |
5.61 KB,
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
2.75 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
We shouldn't need to deal with JSON for this; even though it's a small amount, even this little bit of JSON shows up at 1% or so on traces for startup times. Also reduces the amount of work we have to do in Javascript, which is a win.
Reporter | ||
Comment 1•11 years ago
|
||
Java doesn't have access to histogram IDs, so we need some way of adding samples to histograms by string ID. I hope the comment is sufficient to deter people from using the string Accumulate...
Attachment #790907 -
Flags: review?(vdjeric)
Reporter | ||
Comment 2•11 years ago
|
||
This patch encompasses all the Java-side and widget-side machinery for telemetry events. Pretty straightforward.
Attachment #790909 -
Flags: review?(blassey.bugs)
Reporter | ||
Comment 3•11 years ago
|
||
We don't need most of this code anymore, so let's delete it. I left the Telemetry object for the sake of not touching lots and lots of the file and that it's a little clearer, IMHO.
Attachment #790910 -
Flags: review?(mark.finkle)
Comment 4•11 years ago
|
||
Comment on attachment 790909 [details] [diff] [review] part 2 - add Telemetry-specific GeckoEvent and related machinery Review of attachment 790909 [details] [diff] [review]: ----------------------------------------------------------------- yes please, more of this
Attachment #790909 -
Flags: review?(blassey.bugs) → review+
Comment 5•11 years ago
|
||
Comment on attachment 790907 [details] [diff] [review] part 1 - add Telemetry infrastructure for telemetry gecko events >+ Histogram *h; >+ rv = GetHistogramByEnumId(id, &h); >+ if (NS_SUCCEEDED(rv)) >+ h->Add(sample); Nit: add curlies around the if > /** >+ * Adds a sample to a histogram defined in TelemetryHistograms.h. >+ * You should use the by-enum-id version defined above unless you have >+ * a really good reason to use this one. >+ * >+ * @param name - histogram name >+ * @param sample - value to record >+ */ >+void Accumulate(const char* name, uint32_t sample); Explain why this function even exists, so readers have a better idea of what a really good reason might be
Attachment #790907 -
Flags: review?(vdjeric) → review+
Reporter | ||
Comment 6•11 years ago
|
||
Comment on attachment 790910 [details] [diff] [review] part 3 - delete browser.js code for receiving telemetry events from Java Bouncing this to Brian.
Attachment #790910 -
Flags: review?(mark.finkle) → review?(bnicholson)
Comment 7•11 years ago
|
||
Comment on attachment 790910 [details] [diff] [review] part 3 - delete browser.js code for receiving telemetry events from Java Sorry I missed this review. Looks good to me.
Attachment #790910 -
Flags: review?(bnicholson) → review+
Reporter | ||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaadde1a566d https://hg.mozilla.org/integration/mozilla-inbound/rev/d7a0ad0c7f9d https://hg.mozilla.org/integration/mozilla-inbound/rev/134285f3187f Mochitest-only try run: https://tbpl.mozilla.org/?tree=Try&rev=7957914f3c46
Flags: in-testsuite-
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/eaadde1a566d https://hg.mozilla.org/mozilla-central/rev/d7a0ad0c7f9d https://hg.mozilla.org/mozilla-central/rev/134285f3187f
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Assignee | ||
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•