Skip to enter the fullscreen automatically when hole-punching is enabled.

Skip to enter the fullscreen automatically when hole-punching is enabled.

BUG=391724
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282588

[xhwang, 2014-07-07 16:18:33]

Could you provide more info in the CL description why you make this change? I
don't fully understand the background here. Thanks!

https://codereview.chromium.org/378543002/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/378543002/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:327: return;
Can you put this check in MediaSourcePlayer? e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...

Otherwise, the function name doesn't match what it's doing here.

[xhwang, 2014-07-07 16:23:33]

On 2014/07/07 16:18:33, xhwang wrote:
> Could you provide more info in the CL description why you make this change? I
> don't fully understand the background here. Thanks!
> 
>
https://codereview.chromium.org/378543002/diff/1/content/browser/media/androi...
> File content/browser/media/android/browser_media_player_manager.cc (right):
> 
>
https://codereview.chromium.org/378543002/diff/1/content/browser/media/androi...
> content/browser/media/android/browser_media_player_manager.cc:327: return;
> Can you put this check in MediaSourcePlayer? e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...
> 
> Otherwise, the function name doesn't match what it's doing here.

I took a look at the bug and found more info there. The point is valid. But I
still feel it's better to not call RequestFullScreen() at all in MSP in this
case, rather than hacking RequestEnterFullscreen not to really enter fullscreen
:)

[ycheo (away), 2014-07-08 12:40:09]

https://codereview.chromium.org/378543002/diff/1/content/browser/media/androi...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/378543002/diff/1/content/browser/media/androi...
content/browser/media/android/browser_media_player_manager.cc:327: return;
On 2014/07/07 16:18:33, xhwang wrote:
> Can you put this check in MediaSourcePlayer? e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...
> 
> Otherwise, the function name doesn't match what it's doing here.

Done.

[xhwang, 2014-07-08 16:23:43]

Looking good. Just one more question.

https://codereview.chromium.org/378543002/diff/20001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/378543002/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:329: return
prefs->use_video_overlay_for_embedded_encrypted_video;
Is this setting VIDEO_HOLE specific? 

If yes, this setting and this code isn't guarded by VIDEO_HOLE ifdef. Actually
the render side ShouldUseVideoOverlayForEmbeddedEncryptedVideo() code is guarded
by VIDEO_HOLE ifdef.

If no, you need to update the CL description to make this CL not VIDEO_HOLE
specific.

[ycheo (away), 2014-07-09 01:18:07]

https://codereview.chromium.org/378543002/diff/20001/content/browser/media/an...
File content/browser/media/android/browser_media_player_manager.cc (right):

https://codereview.chromium.org/378543002/diff/20001/content/browser/media/an...
content/browser/media/android/browser_media_player_manager.cc:329: return
prefs->use_video_overlay_for_embedded_encrypted_video;
On 2014/07/08 16:23:43, xhwang wrote:
> Is this setting VIDEO_HOLE specific? 
> 
> If yes, this setting and this code isn't guarded by VIDEO_HOLE ifdef. Actually
> the render side ShouldUseVideoOverlayForEmbeddedEncryptedVideo() code is
guarded
> by VIDEO_HOLE ifdef.
> 
> If no, you need to update the CL description to make this CL not VIDEO_HOLE
> specific.

Actually, the setting exists regardless of VIDEO_HOLE, but the usage is limited
to VIDEO_HOLE.
I didn't guard this because it makes that the condition in medio_source_player
looks ugly.
But, as you said, the function is only for the VIDEO_HOLE.
I think it would be better to guard it.

[ycheo (away), 2014-07-11 02:55:25]

PTAL.

[xhwang, 2014-07-11 05:05:09]

sorry I missed the message. lgtm!

[ycheo (away), 2014-07-11 05:07:45]

On 2014/07/11 05:05:09, xhwang wrote:
> sorry I missed the message. lgtm!

NP. thanks!

[ycheo (away), 2014-07-11 05:12:22]

The CQ bit was checked by ycheo@chromium.org

[commit-bot: I haz the power, 2014-07-11 05:14:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ycheo@chromium.org/378543002/40001

[commit-bot: I haz the power, 2014-07-11 07:05:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[commit-bot: I haz the power, 2014-07-11 08:21:26]

Change committed as 282588