Don't GONE the unsuspended video only players

Don't GONE the unsuspended video only players

Video only players are reported to RendererWebMediaPlayerDelegate as GONE
even if they're playing. This CL would make sure we only report suspended
players as GONE. This affects the logic in MediaWebContentsDelegate that
creates the power blocker for videos.

BUG=703105
TEST=manual

Review-Url: https://codereview.chromium.org/2767253002
Cr-Commit-Position: refs/heads/master@{#458995}
Committed: https://chromium.googlesource.com/chromium/src/+/5f34b644166c2dc02f7f937f69ce0fd08e89e626

[whywhat, 2017-03-22 22:33:21]

Updated logic, updated tests

[whywhat, 2017-03-22 22:34:40]

Remove log statement

[whywhat, 2017-03-22 22:47:45]

Fixed logic for audible videos...

[whywhat, 2017-03-22 22:49:39]

avayvod@chromium.org changed reviewers:
+ sandersd@chromium.org

[whywhat, 2017-03-22 22:49:40]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2017-03-22 22:49:40]

PTaL

I basically restored the old |has_session_playing| condition.
I tried avoiding |!alive| by inversing the |alive| expression but lazy
evaluation bit me somehow and the audible videos stopped loading (although the
unittests still passed).

[commit-bot: I haz the power, 2017-03-22 22:50: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, 2017-03-22 22:50:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 22:50:13]

No L-G-T-M from a valid reviewer yet.
CQ run can only be started once the patch has received an L-G-T-M from a full
committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a
full super star committer.
Committers are members of the group "project-chromium-committers".
Note that this has nothing to do with OWNERS files.

[whywhat, 2017-03-22 22:52:50]

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

[commit-bot: I haz the power, 2017-03-22 22:53:26]

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

[sandersd (OOO until July 31), 2017-03-23 00:02:36]

Logic appears correct to me. I'm assuming that you've verified that this does
not create mediasession controls?

LGTM % comments need to be updated/replaced.

https://codereview.chromium.org/2767253002/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2767253002/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:2008: // |has_remote_controls| indicates if
the player can be controlled outside the
This comment does not mach the code, since (for example) |is_remote| no longer
affects |has_remote_controls|.

[commit-bot: I haz the power, 2017-03-23 00:40:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 00:40:28]

Dry run: This issue passed the CQ dry run.

[whywhat, 2017-03-23 01:15:51]

Fixed comments

[whywhat, 2017-03-23 01:16:39]

On 2017/03/23 at 00:02:36, sandersd wrote:
> Logic appears correct to me. I'm assuming that you've verified that this does
not create mediasession controls?

Yes, appears correct to me for both for videos with and without an audio track.

> 
> LGTM % comments need to be updated/replaced.
> 
>
https://codereview.chromium.org/2767253002/diff/60001/media/blink/webmediapla...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/2767253002/diff/60001/media/blink/webmediapla...
> media/blink/webmediaplayer_impl.cc:2008: // |has_remote_controls| indicates if
the player can be controlled outside the
> This comment does not mach the code, since (for example) |is_remote| no longer
affects |has_remote_controls|.

[whywhat, 2017-03-23 01:17:23]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2017-03-23 01:17:24]

https://codereview.chromium.org/2767253002/diff/60001/media/blink/webmediapla...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2767253002/diff/60001/media/blink/webmediapla...
media/blink/webmediaplayer_impl.cc:2008: // |has_remote_controls| indicates if
the player can be controlled outside the
On 2017/03/23 at 00:02:36, sandersd wrote:
> This comment does not mach the code, since (for example) |is_remote| no longer
affects |has_remote_controls|.

Right. Fixed.

[whywhat, 2017-03-23 01:17:24]

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

[commit-bot: I haz the power, 2017-03-23 01:18:00]

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, 2017-03-23 03:14:08]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490231843415210,
"parent_rev": "cd5c1457b96b49938e484359839a5e62b13ac5c4", "commit_rev":
"5f34b644166c2dc02f7f937f69ce0fd08e89e626"}

[commit-bot: I haz the power, 2017-03-23 03:15:19]

Description was changed from

==========
Don't GONE the unsuspended video only players

Video only players are reported to RendererWebMediaPlayerDelegate as GONE
even if they're playing. This CL would make sure we only report suspended
players as GONE. This affects the logic in MediaWebContentsDelegate that
creates the power blocker for videos.

BUG=703105
TEST=manual
==========

to

==========
Don't GONE the unsuspended video only players

Video only players are reported to RendererWebMediaPlayerDelegate as GONE
even if they're playing. This CL would make sure we only report suspended
players as GONE. This affects the logic in MediaWebContentsDelegate that
creates the power blocker for videos.

BUG=703105
TEST=manual

Review-Url: https://codereview.chromium.org/2767253002
Cr-Commit-Position: refs/heads/master@{#458995}
Committed:
https://chromium.googlesource.com/chromium/src/+/5f34b644166c2dc02f7f937f69ce...
==========

[commit-bot: I haz the power, 2017-03-23 03:15:20]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5f34b644166c2dc02f7f937f69ce...