Closed Bug 1255198 Opened 4 years ago Closed 4 years ago

Add new Telemetry probes for visible/invisible requests

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mds, Assigned: mds)

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 2 obsolete files)

We should add two new telemetry probes in the geolocation code to determine whether the success callbacks for .watchPosition() and .getCurrentPosition() are being fulfilled with a visible or invisible window owner.
Assignee: nobody → michelangelo
Not for review/telemetry feedback yet.
Attachment #8728735 - Flags: feedback?(dveditz)
Comment on attachment 8728735 [details] [diff] [review]
[Telemetry] Record whether geo requests are fulfilled with visible window

Review of attachment 8728735 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, although Hidden() may not be quite what you want. In a single OS window document visibility (Hidden) applies to whether the tab is front-most or background--so that's what you want if the user has a single OS window with many tabs. But in a separate window the front-most tab is still considered "visible" even if the window itself is completely covered by other windows or applications. And even in a single-window browser the top-most document will be visible even if behind other applications.

As an alternative you could use HasFocus() to distinguish between the window the user is looking at from the window hidden behind it. Also depends on whether you want to count location as "visible" only when the user might actually be interacting with the page, or don't mind if we're counting their browser as "visible" even if they're using Skype/Facetime and paying no attention. Possible downside to HasFocus() is that in the rare case someone has two browser windows open side-by-side they can "see" both of them but at most one can have focus. We don't seem to have any telemetry on numbers of tabs or windows, but I imagine it's a fairly small population who might do that kind of thing.
Attachment #8728735 - Flags: feedback?(dveditz) → feedback+
Interesting.

Would it be reasonable to use a check like "!Hidden() && HasFocus()" to determine that the current window is surely under the user's eyes?
I've asked a bit on #Developers and GiJS was suggesting that maybe the "HasFocus" approach might be less than ideal: the check might return false in a number of several corner-cases.

The reason why I went poking around is because during some test I've noticed that the document focus was lost after the door-hanger returned. Same thing may happen when the URL/search bar is focused, and so on...

I was also suggested that there might be a way to ask the browser whether it is top-level, but I'm unsure how.

So, the question is: given we're just adding a probe here, does it make sense to keep digging for the "universal" solution or we can simply rely on the Visibility APIs?
Flags: needinfo?(dveditz)
Visibility is probably OK: for the majority of people who have only one browser window it's accurate enough. Just know that your data has some amount of fuzziness about it.
Flags: needinfo?(dveditz)
Attachment #8728735 - Attachment is obsolete: true
Asking for bsmedberg's feedback on the probe; I guess this has to predate the actual formal review of the patch.

Hopefully I'm not entirely wrong on this.:)
Flags: needinfo?(benjamin)
Attachment #8733137 - Flags: feedback?(benjamin)
Flags: needinfo?(benjamin)
Comment on attachment 8733137 [details] [diff] [review]
[Telemetry] Record whether geo requests are fulfilled with visible window

For data reviews I only review the docs/Histograms.json: it's your and the code reviewer's job to make sure the code matches the docs.

You should not use expires_in_version: default. If this is exploratory data, you should either set an expiration version (typically 6 months in the future). If you intend to monitor this permanently as part of ongoing product quality, you can make it permanent but you need to identify who is responsible for ongoing monitoring and what kind of dashboards or alerting you plan to use. When you make this change, it is not necessary to add these to histogram-whitelists.

Please make the description more prescriptive: "This metric is recorded every time (...). A false value is recorded if the owner is not visible according to docshell.isVisible."
Attachment #8733137 - Flags: feedback?(benjamin) → feedback-
Attachment #8733137 - Attachment is obsolete: true
Comment on attachment 8734557 [details]
MozReview Request: Bug 1255198 - [Telemetry] Geolocation probes to record fulfilled requests according to document.isVisible. data-review=bsmedberg r=jdm

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)

> You should not use expires_in_version: default. If this is exploratory data,

Done. Expiring in 55, as the other geo probes.

> Please make the description more prescriptive: "This metric is recorded
> every time (...). A false value is recorded if the owner is not visible
> according to docshell.isVisible."

Done. Is it clearer now?
Thanks for your feedback, Benjamin.

Also asking Doug's formal review.
Attachment #8734557 - Flags: review?(dougt)
Attachment #8734557 - Flags: feedback?(benjamin)
Comment on attachment 8734557 [details]
MozReview Request: Bug 1255198 - [Telemetry] Geolocation probes to record fulfilled requests according to document.isVisible. data-review=bsmedberg r=jdm

You should not add these to histogram-whitelists.json, which is meant to allow legacy histograms to bypass the rules about having a bug_number and alert_emails. I suspect you still need to add an "alert_emails" record.

data-review=me with that change
Attachment #8734557 - Flags: feedback?(benjamin) → feedback+
Whiteboard: btpp-active
Comment on attachment 8734557 [details]
MozReview Request: Bug 1255198 - [Telemetry] Geolocation probes to record fulfilled requests according to document.isVisible. data-review=bsmedberg r=jdm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42355/diff/1-2/
Attachment #8734557 - Attachment description: MozReview Request: Bug 1255198 - [Telemetry] Geolocation probes to record fulfilled requests according to docshell.isVisible. → MozReview Request: Bug 1255198 - [Telemetry] Geolocation probes to record fulfilled requests according to docshell.isVisible. data-review=bsmedberg
Attachment #8734557 - Flags: review?(dougt)
Attachment #8734557 - Flags: feedback+
Comment on attachment 8734557 [details]
MozReview Request: Bug 1255198 - [Telemetry] Geolocation probes to record fulfilled requests according to document.isVisible. data-review=bsmedberg r=jdm

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #12)

> You should not add these to histogram-whitelists.json, which is meant to
> allow legacy histograms to bypass the rules about having a bug_number and
> alert_emails. I suspect you still need to add an "alert_emails" record.
> 
> data-review=me with that change

Done. Thank you, Benjamin.

Doug: I've added my email here for the email alerts; not sure whether we have something more appropriate.
Attachment #8734557 - Flags: review?(dougt)
Attachment #8734557 - Flags: review?(dougt) → review?(josh)
Attachment #8734557 - Flags: review?(josh) → review+
Comment on attachment 8734557 [details]
MozReview Request: Bug 1255198 - [Telemetry] Geolocation probes to record fulfilled requests according to document.isVisible. data-review=bsmedberg r=jdm

https://reviewboard.mozilla.org/r/42355/#review41579

::: dom/geolocation/nsGeolocation.cpp:499
(Diff revision 2)
>      Telemetry::Accumulate(Telemetry::GEOLOCATION_REQUEST_GRANTED, mProtocolType + 10);
> +
> +    // Record whether a location callback is fulfilled while the owner window
> +    // is not visible.
> +    bool isVisible = false;
> +    nsCOMPtr<nsPIDOMWindowInner> window = mLocator ? mLocator->GetParentObject()

We use mLocator unconditionally below this, so there's no need to check it here.

::: toolkit/components/telemetry/Histograms.json:526
(Diff revision 2)
> +  "GEOLOCATION_GETCURRENTPOSITION_VISIBLE": {
> +    "alert_emails": ["michelangelo@mozilla.com"],
> +    "expires_in_version": "55",
> +    "kind": "boolean",
> +    "bug_numbers": [1255198],
> +    "description": "This metric is recorded every time a navigator.geolocation.getCurrentPosition() request gets allowed/fulfilled. A false value is recorded if the owner is not visible according to docshell.isVisible."

Docshell is different than document.

::: toolkit/components/telemetry/Histograms.json:533
(Diff revision 2)
> +  "GEOLOCATION_WATCHPOSITION_VISIBLE": {
> +    "alert_emails": ["michelangelo@mozilla.com"],
> +    "expires_in_version": "55",
> +    "kind": "boolean",
> +    "bug_numbers": [1255198],
> +    "description": "This metric is recorded every time a navigator.geolocation.watchPosition() request gets allowed/fulfilled. A false value is recorded if the owner is not visible according to docshell.isVisible."

Same as previous.
Comment on attachment 8734557 [details]
MozReview Request: Bug 1255198 - [Telemetry] Geolocation probes to record fulfilled requests according to document.isVisible. data-review=bsmedberg r=jdm

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/42355/diff/2-3/
Attachment #8734557 - Attachment description: MozReview Request: Bug 1255198 - [Telemetry] Geolocation probes to record fulfilled requests according to docshell.isVisible. data-review=bsmedberg → MozReview Request: Bug 1255198 - [Telemetry] Geolocation probes to record fulfilled requests according to document.isVisible. data-review=bsmedberg r=jdm
Thank you for your review Josh!
https://hg.mozilla.org/mozilla-central/rev/16ecc568c14e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.