Closed
Bug 1252617
Opened 8 years ago
Closed 8 years ago
Probe to measure when offline error page is shown
Categories
(Firefox for Android Graveyard :: Metrics, defect)
Firefox for Android Graveyard
Metrics
Tracking
(firefox48 fixed)
RESOLVED
FIXED
Firefox 48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: barbara, Assigned: barbara)
Details
Attachments
(1 file, 2 obsolete files)
2.16 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Adding my first probe, what do you think about the cool "extra" label?
Assignee: nobody → bbermes
Attachment #8725854 -
Flags: review?(mark.finkle)
Comment 2•8 years ago
|
||
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+
Comment 3•8 years ago
|
||
(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?
Assignee | ||
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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+
Assignee | ||
Comment 6•8 years ago
|
||
Added back neterror errorType as requested.
Attachment #8727535 -
Flags: review?(mark.finkle)
Comment 7•8 years ago
|
||
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•8 years ago
|
Keywords: checkin-needed
Updated•8 years ago
|
Attachment #8726366 -
Attachment is obsolete: true
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/893ca601352a
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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
•