Let AutoplayUmaHelper listen to visibilitychange instead of unload

Let AutoplayUmaHelper listen to visibilitychange instead of unload

Using window unload listeners is both unreliable and slowing natigating
away. This CL switches the AutoplayUmaHelper to use the page visibility
API instead of listening to window unload event.

BUG=653473
Committed: https://crrev.com/a93cc968201804405f261ece6fee30b660255ec2
Cr-Commit-Position: refs/heads/master@{#434272}

[Zhiqiang Zhang (Slow), 2016-10-31 16:26:15]

The CQ bit was checked by zqzhang@chromium.org to run a CQ dry run

[Zhiqiang Zhang (Slow), 2016-10-31 16:26:27]

zqzhang@chromium.org changed reviewers:
+ mlamouri@chromium.org

[commit-bot: I haz the power, 2016-10-31 16:26:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mlamouri (slow - plz ping), 2016-10-31 16:33:31]

Do we have a test for this? I guess we might not have a LayoutTest in which
case, can we have a unit test that test that the page visibily change is
properly fired when the page is "unloaded"?

https://codereview.chromium.org/2463113002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (left):

https://codereview.chromium.org/2463113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:158: 
nit: is this change intentional?

[Zhiqiang Zhang (Slow), 2016-10-31 16:48:54]

> Do we have a test for this? I guess we might not have a LayoutTest in which
> case, can we have a unit test that test that the page visibily change is
> properly fired when the page is "unloaded"?

We have one:
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/fast/even...

It's a bit hard to write such LayoutTest for AutoplayUmaHelper though.
So I think the correctness should be maintained by the Page Visibility API team?

https://codereview.chromium.org/2463113002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (left):

https://codereview.chromium.org/2463113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:158: 
On 2016/10/31 16:33:31, mlamouri wrote:
> nit: is this change intentional?

Undone this accidental change :)

[mlamouri (slow - plz ping), 2016-10-31 16:50:00]

LayoutTests would be hard but can't you add unit tests?

[Zhiqiang Zhang (Slow), 2016-11-01 16:02:32]

PTAL

On 2016/10/31 16:50:00, mlamouri wrote:
> LayoutTests would be hard but can't you add unit tests?

Done.

[mlamouri (slow - plz ping), 2016-11-03 15:52:40]

As discussed offline, maybe we should switch to use the
ContextLifecycleObserver.

[Zhiqiang Zhang (Slow), 2016-11-04 17:29:09]

PTAL (not urgent)

Using ContextLifecycleObserver now :)

[mlamouri (slow - plz ping), 2016-11-04 18:16:49]

lgtm

https://codereview.chromium.org/2463113002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right):

https://codereview.chromium.org/2463113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:167:
setContext(nullptr);
I don't think you need this.

[Zhiqiang Zhang (Slow), 2016-11-04 20:29:32]

zqzhang@chromium.org changed reviewers:
+ ojan@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-04 20:29:33]

+ojan for general review and approval

https://codereview.chromium.org/2463113002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp (right):

https://codereview.chromium.org/2463113002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/AutoplayUmaHelper.cpp:167:
setContext(nullptr);
On 2016/11/04 18:16:49, mlamouri wrote:
> I don't think you need this.

Done.

[mlamouri (slow - plz ping), 2016-11-18 14:21:53]

ojan@ PTAL? :)

[ojan, 2016-11-23 19:00:36]

The CQ bit was checked by ojan@chromium.org

[ojan, 2016-11-23 19:00:37]

lgtm

Thanks for fixing this. Sorry for the review delay. Please nag me earlier in the
future. I just have trouble keeping up with my backlog.

[ojan, 2016-11-23 19:00:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2463113002/#ps80001
(title: "nit")

[commit-bot: I haz the power, 2016-11-23 19:01:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 22:52:58]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1479927636698010,
"parent_rev": "35b37e97274c6bad96737570b9f50cc386fa71cb", "commit_rev":
"b6a935e8e3bb8f4d3c0a626844079b4c22fd830e"}

[commit-bot: I haz the power, 2016-11-23 22:53:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-23 22:57:00]

Description was changed from

==========
Let AutoplayUmaHelper listen to visibilitychange instead of unload

Using window unload listeners is both unreliable and slowing natigating
away. This CL switches the AutoplayUmaHelper to use the page visibility
API instead of listening to window unload event.

BUG=653473
==========

to

==========
Let AutoplayUmaHelper listen to visibilitychange instead of unload

Using window unload listeners is both unreliable and slowing natigating
away. This CL switches the AutoplayUmaHelper to use the page visibility
API instead of listening to window unload event.

BUG=653473

Committed: https://crrev.com/a93cc968201804405f261ece6fee30b660255ec2
Cr-Commit-Position: refs/heads/master@{#434272}
==========

[commit-bot: I haz the power, 2016-11-23 22:57:01]

Patchset 5 (id:??) landed as
https://crrev.com/a93cc968201804405f261ece6fee30b660255ec2
Cr-Commit-Position: refs/heads/master@{#434272}