[Video] Fix DCHECK crash in PauseVideoIfHidden

[Video] Fix DCHECK crash in PauseVideoIfHidden

WMPI::OnFrameHidden() can get called when the frame is closed.
WMPI::IsHidden() will return false in this case, however it's assumed to
return true in OnFrameHidden and the methods called from there.

BUG=684256
TEST=manual

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

[whywhat, 2017-01-24 04:49:37]

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

[whywhat, 2017-01-24 04:49:38]

PTaL

I took a little more conservative approach here than what I suggested in the bug
as maybe watch time and playstate have to be updated when the frame is closed?

[sandersd (OOO until July 31), 2017-01-24 19:51:21]

lgtm

https://codereview.chromium.org/2653553005/diff/1/media/blink/webmediaplayer_...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/2653553005/diff/1/media/blink/webmediaplayer_...
media/blink/webmediaplayer_impl.cc:1408: if (!delegate_->IsFrameClosed()) {
Perhaps just call IsHidden()?

Also, rename IsHidden() to IsBackgrounded() to reduce confusion?

[watk, 2017-01-24 21:50:04]

On 2017/01/24 19:51:21, sandersd wrote:
> lgtm
> 
>
https://codereview.chromium.org/2653553005/diff/1/media/blink/webmediaplayer_...
> File media/blink/webmediaplayer_impl.cc (right):
> 
>
https://codereview.chromium.org/2653553005/diff/1/media/blink/webmediaplayer_...
> media/blink/webmediaplayer_impl.cc:1408: if (!delegate_->IsFrameClosed()) {
> Perhaps just call IsHidden()?
> 
> Also, rename IsHidden() to IsBackgrounded() to reduce confusion?

+1. When I looked at this after the crash I was really confused by the two
IsHidden()s.

[whywhat, 2017-01-24 21:59:27]

Added IsHidden() check

[whywhat, 2017-01-24 22:02:08]

Since I want to merge this to 57, I'll do the rename in a follow up.
To me the confusing part is that OnFrameHidden can be called by the delegate
when it's closed so IsHidden() will return false. Renaming one to IsBackground()
would probably underline these are different but won't tell why which to me
would still be confusing - what's the difference between "hidden" and
"background"?

[whywhat, 2017-01-24 22:02:12]

The CQ bit was checked by avayvod@chromium.org

[whywhat, 2017-01-24 22:02:13]

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/2653553005/#ps20001
(title: "Added IsHidden() check")

[commit-bot: I haz the power, 2017-01-24 22:03:38]

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-01-24 22:09:38]

On 2017/01/24 22:02:08, whywhat_slow_PST_till_Feb_6 wrote:
> Since I want to merge this to 57, I'll do the rename in a follow up.
> To me the confusing part is that OnFrameHidden can be called by the delegate
> when it's closed so IsHidden() will return false. Renaming one to
IsBackground()
> would probably underline these are different but won't tell why which to me
> would still be confusing - what's the difference between "hidden" and
> "background"?

"Hidden" is a render frame concept, it's true when the frame isn't visible.

"Backgrounded" is a media concept, it's true when the page has been temporarily
switched away from.

I don't think the exact distinctions are important; WMPI should compute
'backgrounded' from 'hidden' (and comment the difference where that computation
is made), then use 'backgrounded' everywhere.

We could push the distinction up into the delegate, only exposing 'IsClosed' and
'IsBackgrounded', which would be mutually exclusive.

[watk, 2017-01-24 22:12:14]

Yeah, I'd add that even if the names don't convey their meaning precisely, it's
important that they're not the same name.

[commit-bot: I haz the power, 2017-01-25 00:11:07]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485295332401220,
"parent_rev": "c42649fbd766116b8eb45aaaa5369dcb175eaa9f", "commit_rev":
"5f91d1754a7ffa39cff9619db090b91b8d1b4a98"}

[commit-bot: I haz the power, 2017-01-25 00:11:45]

Description was changed from

==========
[Video] Fix DCHECK crash in PauseVideoIfHidden

WMPI::OnFrameHidden() can get called when the frame is closed.
WMPI::IsHidden() will return false in this case, however it's assumed to
return true in OnFrameHidden and the methods called from there.

BUG=684256
TEST=manual
==========

to

==========
[Video] Fix DCHECK crash in PauseVideoIfHidden

WMPI::OnFrameHidden() can get called when the frame is closed.
WMPI::IsHidden() will return false in this case, however it's assumed to
return true in OnFrameHidden and the methods called from there.

BUG=684256
TEST=manual

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

[commit-bot: I haz the power, 2017-01-25 00:11:46]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5f91d1754a7ffa39cff9619db090...