Probe to measure when offline error page is shown

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: barbara, Assigned: barbara)

Tracking

unspecified
Firefox 48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 8725854 [details] [diff] [review]
Probe to measure when offline error page is shown

Adding my first probe, what do you think about the cool "extra" label?
Assignee: nobody → bbermes
Attachment #8725854 - Flags: review?(mark.finkle)
Comment on attachment 8725854 [details] [diff] [review]
Probe to measure when offline error page is shown

># HG changeset patch
># User Barbara Bermes <bbermes@mozilla.com>
># 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
Attachment #8725854 - Flags: review?(mark.finkle) → review+
(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?
Created attachment 8726366 [details] [diff] [review]
Probe to measure when offline error page is shown (v2)

Added extra value to be the actual net error, cleaned up code
Attachment #8725854 - Attachment is obsolete: true
Attachment #8726366 - Flags: review?(mark.finkle)
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
Attachment #8726366 - Flags: review?(mark.finkle) → review+
Created attachment 8727535 [details] [diff] [review]
Probe to measure when offline error page is shown (v3)

Added back neterror errorType as requested.
Attachment #8727535 - Flags: review?(mark.finkle)
Comment on attachment 8727535 [details] [diff] [review]
Probe to measure when offline error page is shown (v3)

LGTM
Attachment #8727535 - Flags: review?(mark.finkle) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Attachment #8726366 - Attachment is obsolete: true

Comment 9

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/893ca601352a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.