1414698 - Reset WEBVR_TIME_SPENT_VIEWING_IN_2D telemetry when the tab moves to the background.

Status: RESOLVED FIXED

(Core :: WebVR, enhancement)

Description

[Daosheng Mu[:daoshengmu]]

Currently, the amount time for viewing VR content in 2D user is starting from a tab is launched until the tab is closed whether it is an active tab or a background tab. I think we should make it be stopped and send this histogram ping when it is switched to be as a background tab. After it switches to be an active tab, the timer would be reset and run again.

Comment 1

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1414698 - Part 1: Stop gamepad vibration when changing to the background;

Review commit: https://reviewboard.mozilla.org/r/196566/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196566/

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8925440 [details]
Bug 1414698 - Part 1: Stop gamepad vibration when changing to the background;

https://reviewboard.mozilla.org/r/196566/#review201788


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/base/nsGlobalWindow.cpp:10651
(Diff revision 1)
> +    if (inner && changed) {
>        inner->StopGamepadHaptics();
> +      inner->ResetVRTelemetry(true);
>      }
>      return;
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment 3

[Andrea Marchesini [:baku]]

Comment on attachment 8925440 [details]
Bug 1414698 - Part 1: Stop gamepad vibration when changing to the background;

https://reviewboard.mozilla.org/r/196566/#review201784

::: dom/base/nsGlobalWindow.h:776
(Diff revision 1)
>  
>    // Inner windows only.
>    // Enable/disable updates for VR
>    void EnableVRUpdates();
>    void DisableVRUpdates();
> +  // Reset telemery data when switching windows.

telemetry

::: dom/base/nsGlobalWindow.cpp:10646
(Diff revision 1)
>    if (aIsBackground) {
>      // Notify gamepadManager we are at the background window,
>      // we need to stop vibrate.
> -    if (inner) {
> +    // Stop the vr telemery time spent when it switches to
> +    // the background window.
> +    if (inner && changed) {

here you are changing the logic for StopGamepadHaptics().
Separate patch for this if needed.

::: dom/base/nsGlobalWindow.cpp:10648
(Diff revision 1)
>      // we need to stop vibrate.
> -    if (inner) {
> +    // Stop the vr telemery time spent when it switches to
> +    // the background window.
> +    if (inner && changed) {
>        inner->StopGamepadHaptics();
> +      inner->ResetVRTelemetry(true);

Add a comment here. What is this true? /* update */

::: dom/base/nsGlobalWindow.cpp:10653
(Diff revision 1)
> +      inner->ResetVRTelemetry(true);
>      }
>      return;
> +  } else {
> +    // Switching to be as a top tab, restart the telemetry.
> +    inner->ResetVRTelemetry(false);

here as well.

::: dom/vr/VREventObserver.cpp:63
(Diff revision 1)
>  }
>  
>  void
> +VREventObserver::UpdateSpentTimeIn2DTelemetry(bool aUpdate)
> +{
> +  if (mWindow && mIs2DView && aUpdate && mHasReset) {

What do you need mHasReset? Can you just use aUpdate? If not, write a comment explaining why.

::: dom/vr/VREventObserver.cpp:64
(Diff revision 1)
>  
>  void
> +VREventObserver::UpdateSpentTimeIn2DTelemetry(bool aUpdate)
> +{
> +  if (mWindow && mIs2DView && aUpdate && mHasReset) {
> +    printf_stderr("Send VREventObserver::UpdateSpentTimeIn2DTelemetry... \n");

Why this printf? If you need to have logs, use MOZ_LOG.

Comment 4

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1414698 - Part 2: Reset WEBVR_TIME_SPENT_VIEWING_IN_2D telemetry when the tab moves to the background;

Review commit: https://reviewboard.mozilla.org/r/196588/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196588/

Comment 5

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8925440 [details]
Bug 1414698 - Part 1: Stop gamepad vibration when changing to the background;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196566/diff/1-2/

Comment 6

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #5)
> Comment on attachment 8925440 [details]
> Bug 1414698 - Part 1: Stop gamepad vibration when changing to the background;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/196566/diff/1-2/

I have confirmed locally that we only need to notify gamepads to be stopped when the state is changed to avoid redundant messages.

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8925440 [details]
Bug 1414698 - Part 1: Stop gamepad vibration when changing to the background;

https://reviewboard.mozilla.org/r/196566/#review201826

::: dom/base/nsGlobalWindow.cpp:10644
(Diff revision 2)
>    }
>  
>    if (aIsBackground) {
>      // Notify gamepadManager we are at the background window,
>      // we need to stop vibrate.
> -    if (inner) {
> +    // Stop the vr telemery time spent when it switches to

Nothing related to VR telemetry.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8925472 [details]
Bug 1414698 - Part 2: Reset WEBVR_TIME_SPENT_VIEWING_IN_2D telemetry when the tab moves to the background;

https://reviewboard.mozilla.org/r/196588/#review201828

Comment 9

[:kip (Kearwood Gilbert)]

Comment on attachment 8925472 [details]
Bug 1414698 - Part 2: Reset WEBVR_TIME_SPENT_VIEWING_IN_2D telemetry when the tab moves to the background;

https://reviewboard.mozilla.org/r/196588/#review201966

LGTM, thanks!

Comment 10

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8925440 [details]
Bug 1414698 - Part 1: Stop gamepad vibration when changing to the background;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196566/diff/2-3/

Comment 11

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8925472 [details]
Bug 1414698 - Part 2: Reset WEBVR_TIME_SPENT_VIEWING_IN_2D telemetry when the tab moves to the background;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196588/diff/1-2/

Comment 12

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #10)
> Comment on attachment 8925440 [details]
> Bug 1414698 - Part 1: Stop gamepad vibration when changing to the background;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/196566/diff/2-3/

Move the VR telemetry comment to P2.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46690f53a2f7
Part 1: Stop gamepad vibration when changing to the background; r=baku
https://hg.mozilla.org/integration/autoland/rev/fb5d2596b7a0
Part 2: Reset WEBVR_TIME_SPENT_VIEWING_IN_2D telemetry when the tab moves to the background; r=baku,kip

Comment 14

[Daosheng Mu[:daoshengmu]]

In order to get more correct telemetry in FF 57, if no one disagrees with it, I would send a uplift for them.

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/46690f53a2f7
https://hg.mozilla.org/mozilla-central/rev/fb5d2596b7a0

Comment 16

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8925472 [details]
Bug 1414698 - Part 2: Reset WEBVR_TIME_SPENT_VIEWING_IN_2D telemetry when the tab moves to the background;

Approval Request Comment
[Feature/Bug causing the regression]: Please help me uplift it to FF57 for avoiding measuring user time spent improperly in telemetry even though it has already been at the background window.
[User impact if declined]: We will continue to collect the wrong number of telemetry and predict users' behavior incorrectly.
[Is this code covered by automated tests?]: This is for the telemetry data upload, I have confirmed it locally.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Nope.
[List of other uplifts needed for the feature/fix]: Nope.
[Is the change risky?]: Nope.
[Why is the change risky/not risky?]: It just affects the way of collecting the WEBVR_TIME_SPENT_VIEWING_IN_2D telemetry, it will not influence user behaviors. 
[String changes made/needed]: Nope.

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8925472 [details]
Bug 1414698 - Part 2: Reset WEBVR_TIME_SPENT_VIEWING_IN_2D telemetry when the tab moves to the background;

57 is current in RC/code freeze. We are only accepting patches for dot release drivers. I'd prefer to let this fix ride the 58 train.