Closed Bug 1414698 Opened 2 years ago Closed 2 years ago

Reset WEBVR_TIME_SPENT_VIEWING_IN_2D telemetry when the tab moves to the background.

Categories

(Core :: WebVR, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: daoshengmu, Assigned: daoshengmu)

Details

Attachments

(2 files)

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.
Assignee: nobody → dmu
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 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.
Attachment #8925440 - Flags: review?(amarchesini) → review-
(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 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.
Attachment #8925440 - Flags: review?(amarchesini) → review+
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
Attachment #8925472 - Flags: review?(amarchesini) → review+
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!
Attachment #8925472 - Flags: review?(kgilbert) → review+
(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.
Pushed by dmu@mozilla.com:
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
In order to get more correct telemetry in FF 57, if no one disagrees with it, I would send a uplift for them.
https://hg.mozilla.org/mozilla-central/rev/46690f53a2f7
https://hg.mozilla.org/mozilla-central/rev/fb5d2596b7a0
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.
Attachment #8925472 - Flags: approval-mozilla-beta?
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.
Attachment #8925472 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.