Closed
Bug 1121559
Opened 9 years ago
Closed 9 years ago
Use telemetry to measure WAP sites served to mobile Firefox browsers
Categories
(Toolkit :: Telemetry, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: miketaylr, Assigned: miketaylr)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
2.61 KB,
patch
|
mcmanus
:
review+
benjamin
:
feedback+
|
Details | Diff | Splinter Review |
The Web Compat team would like to measure how frequently mobile Firefox users are served WAP/WML content. Ideally this would run for few months and we could make a decision on next steps at that point. See Bug 997668 and Bug 941241 (more specifically the bugs they block) for examples of mobile Firefox users being served WAP/WML content. I think a "count" histogram makes sense as a proxy for page views.
Assignee | ||
Comment 1•9 years ago
|
||
Here's a proposed patch--but I have no idea who to ping for a review. Any suggestions?
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → miket
Assignee | ||
Comment 2•9 years ago
|
||
AFAICT, there's no way to set a cpp_guard for "mobile" or b2g+Android (or is there?). Presumably Desktop browsers are never sent WAP content, so I think we could live with the few (if any) false negatives here. Unless someone has a better suggestion.
Comment 3•9 years ago
|
||
(In reply to Mike Taylor [:miketaylr] from comment #2) > AFAICT, there's no way to set a cpp_guard for "mobile" or b2g+Android (or is > there?). Presumably Desktop browsers are never sent WAP content, so I think > we could live with the few (if any) false negatives here. Unless someone has > a better suggestion. You'd probably want to #ifdef the measurement part anyway, which would be easily doable for "mobile"; having an extra histogram in Histograms.json isn't going to hurt anything. The Histograms.json portion of the patch looks OK, can't speaking to the networking part.
Updated•9 years ago
|
Attachment #8548999 -
Flags: feedback+
Assignee | ||
Comment 4•9 years ago
|
||
Thanks for the feedback Nathan. I've #ifdef'ed it on ANDROID and MOZ_B2G.
Attachment #8548999 -
Attachment is obsolete: true
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8549030 [details] [diff] [review] 1121559-2.patch r? :mcmanus because I see he did a review for Bug 976912 (hope that's oK!)
Comment 6•9 years ago
|
||
Comment on attachment 8549030 [details] [diff] [review] 1121559-2.patch Review of attachment 8549030 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +1557,5 @@ > +#if defined(ANDROID) || defined(MOZ_B2G) > + // Gather telemetry on being sent a WAP content-type > + // This check will catch (at least) the following content types: > + // application/wml+xml, application/vnd.wap.xhtml+xml, > + // text/vnd.wap.wml, application/vnd.wap.wmlc if (gHttpHandler->IsTelemetryEnabled()) {} will avoid those string operations if we aren't reporting telemetry. @@ +1560,5 @@ > + // application/wml+xml, application/vnd.wap.xhtml+xml, > + // text/vnd.wap.wml, application/vnd.wap.wmlc > + if (mResponseHead->ContentType().Find(".wap") != -1 || > + mResponseHead->ContentType().Find("/wml") != -1) { > + Telemetry::Accumulate(Telemetry::HTTP_WAP_CONTENT_TYPE_COUNT, 1); I think you want a boolean here - isWap and report that. That way you can figure out the fraction of responses that are wap instead of a raw number without any context. ::: toolkit/components/telemetry/Histograms.json @@ +1211,5 @@ > "description": "Whether a HTTP transaction routed via Alt-Svc was scheme=http" > }, > + "HTTP_WAP_CONTENT_TYPE_COUNT": { > + "expires_in_version": "40", > + "kind": "count", boolean
Attachment #8549030 -
Flags: review?(mcmanus)
Assignee | ||
Comment 7•9 years ago
|
||
Thanks for the review! I think I've addressed all comments and also added a ContentType().IsEmpty() condition before setting isWap to true.
Attachment #8549030 -
Attachment is obsolete: true
Comment 8•9 years ago
|
||
Comment on attachment 8550570 [details] [diff] [review] 1121559-3.patch Review of attachment 8550570 [details] [diff] [review]: ----------------------------------------------------------------- lgtm thanks
Attachment #8550570 -
Flags: review?(mcmanus) → review+
Comment 10•9 years ago
|
||
Hi, can we get a try run for this change, thanks!
Flags: needinfo?(miket)
Keywords: checkin-needed
Assignee | ||
Comment 11•9 years ago
|
||
Here's the try run (ran this on Friday--forgot to link /o\): https://treeherder.mozilla.org/#/jobs?repo=try&revision=3104f386a2d7 This was my first time using try, I wasn't really sure what to select, but this is green.
Keywords: checkin-needed
Comment 12•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/8b09ab087404
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 13•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8b09ab087404
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla38
Assignee | ||
Comment 14•9 years ago
|
||
My apologies, I didn't see [1] until today. ni? :bsmedberg for approval, even though this has landed. [1] <https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval>
Updated•9 years ago
|
Flags: needinfo?(benjamin)
Attachment #8550570 -
Flags: feedback+
You need to log in
before you can comment on or make changes to this bug.
Description
•