905759 - move telemetry sending from java into native code

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Nathan Froyd [:froydnj]]

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.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add Telemetry infrastructure for telemetry gecko events

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...

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add Telemetry-specific GeckoEvent and related machinery

This patch encompasses all the Java-side and widget-side machinery for telemetry
events.  Pretty straightforward.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - delete browser.js code for receiving telemetry events from Java

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.

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 5

[Vladan Djeric (:vladan)]

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

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 790910 [details] [diff] [review]
part 3 - delete browser.js code for receiving telemetry events from Java

Bouncing this to Brian.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 8

[Nathan Froyd [:froydnj]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eaadde1a566d
https://hg.mozilla.org/mozilla-central/rev/d7a0ad0c7f9d
https://hg.mozilla.org/mozilla-central/rev/134285f3187f