1252617 - Probe to measure when offline error page is shown

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Metrics, defect)

Description

[Barbara Bermes [:barbara] - NI please!]

In order to understand the usage of offline cached pages (neterror.1 > toast > cache) vs. just pure offline error pages usage, we should add a probe to track this.

Comment 1

[Barbara Bermes [:barbara] - NI please!]

Attachment: Probe to measure when offline error page is shown

Adding my first probe, what do you think about the cool "extra" label?

Comment 2

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

Comment on attachment 8725854 [details] [diff] [review]
Probe to measure when offline error page is shown

># HG changeset patch
># User Barbara Bermes <[e-mail]>
># Date 1456950965 18000

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>         if (docURI.startsWith("about:certerror"))
>           errorType = "certerror";
>         else if (docURI.startsWith("about:blocked"))
>           errorType = "blocked"
>-        else if (docURI.startsWith("about:neterror"))
>-          errorType = "neterror";
>+        else if (docURI.startsWith("about:neterror")){

Add a space before {

Not your code, but since you added braces to one block, you might as well add them to the rest, which is our coding style. This code is just old enough that we didn't add braces to single line blocks back then.

>+            UITelemetry.addEvent("neterror.1", "content", null, "errorpage");
>+            errorType = "neterror";
>+        }
>+

Remove extra blank line

r+ with nits

Comment 3

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

(In reply to Mark Finkle (:mfinkle) from comment #2)

> >+            UITelemetry.addEvent("neterror.1", "content", null, "errorpage");

I just remembered, this condition | docURI.startsWith("about:neterror") | can happen for more than "offline". We also show this page for DNS and server errors, IIRC.

We pass the "error" into the about:neterror page as a query param. We extract it here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/netError.xhtml#44

Here is a list of error types (et_*)
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/netError.xhtml#287

We could extract the error code and use it as the Extra in the UITemeletry call.

Thoughts?

Comment 4

[Barbara Bermes [:barbara] - NI please!]

Attachment: Probe to measure when offline error page is shown (v2)

Added extra value to be the actual net error, cleaned up code

Comment 5

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

Comment on attachment 8726366 [details] [diff] [review]
Probe to measure when offline error page is shown (v2)


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js


>-        else if (docURI.startsWith("about:neterror"))
>-          errorType = "neterror";

On IRC, I mentioned removing | errorType | since it wasn't used in the small snippet we were looking at. Turns out, it's used and required. Put it back please.

Otherwise looks fine. I assume you tested this a little and saw good error types being extracted from the URL?

r+ with nits

Comment 6

[Barbara Bermes [:barbara] - NI please!]

Attachment: Probe to measure when offline error page is shown (v3)

Added back neterror errorType as requested.

Comment 7

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

Comment on attachment 8727535 [details] [diff] [review]
Probe to measure when offline error page is shown (v3)

LGTM

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/893ca601352a

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/893ca601352a