Bug 84414 – Expose a flag so that fullscreen video on android can work with FULLSCREEN_API

• Component: Media
• Status: RESOLVED FIXED

[Min Qin]

Expose a flag so that fullscreen video on android can work with FULLSCREEN_API

[Min Qin]

Created attachment 138032 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 138032 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Min Qin]

Created attachment 138034 [details]
Patch

[Eric Carlson]

Comment on attachment 138034 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138034&action=review

The WebCore part of this looks fine to me modulo the simplification comment.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:768
> +#if ENABLE(PLUGIN_PROXY_FOR_VIDEO)
>  void MediaPlayer::enterFullscreen()
>  {
>      m_private->enterFullscreen();
> -}    
> +}
> +#elif ENABLE(NATIVE_FULLSCREEN_VIDEO)
> +bool MediaPlayer::enterFullscreen() const
> +{
> +    return m_private->enterFullscreen();
> +}

I think you might as well just change m_private->enterFullscreen to return a bool so you can combine these.

[Min Qin]

Created attachment 138125 [details]
Patch

[Eric Carlson]

This looks fine to me, but I am not marking r+ pending approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail].

[James Robinson]

Comment on attachment 138125 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138125&action=review

> Source/WebKit/chromium/public/WebMediaPlayer.h:212
> +    virtual bool enterFullscreen() { return false; }

could you document what this does (i'm guessing tell this media player to go fullscreen) and what the bool return value means?

[Min Qin]

Created attachment 138137 [details]
Patch

[Min Qin]

Comments added for enter and exit fullscreen

[Min Qin]

Ping, anyone from the chromium side can review this?

[Darin Fisher (:fishd, Google)]

Comment on attachment 138137 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138137&action=review

> Source/WebKit/chromium/src/WebViewImpl.cpp:1592
>      if (m_client && m_client->enterFullScreen())

so then on the chromium side you will make this enterFullScreen()
method always return false?  or, will you try to make fullscreen
mode work for non-video elements too?

[Min Qin]

On chromium side, the enterFullScreen should only work for video element. For audio element, we will return false.  There is no current plan for MediaSource element yet. All the logic should be in webmediaplayer_android.cc.

[WebKit Review Bot]

Comment on attachment 138137 [details]
Patch

Rejecting attachment 138137 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Source/WebKit/chromium/public/WebMediaPlayer.h.rej
patching file Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp
patching file Source/WebKit/chromium/src/WebMediaPlayerClientImpl.h
patching file Source/WebKit/chromium/src/WebViewImpl.cpp
Hunk #1 succeeded at 1584 (offset 5 lines).
Hunk #2 succeeded at 1603 (offset 5 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Darin Fish..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12555355

[Min Qin]

Created attachment 139467 [details]
Patch

[Min Qin]

Sorry, there is a merge error during the previous commit process. Resolved the conflicts now

[WebKit Review Bot]

Comment on attachment 139467 [details]
Patch

Clearing flags on attachment: 139467

Committed r115661: <http://trac.webkit.org/changeset/115661>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

Comment on attachment 139467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139467&action=review

> Source/WebCore/platform/graphics/MediaPlayer.cpp:772
> +#if ENABLE(PLUGIN_PROXY_FOR_VIDEO) || ENABLE(NATIVE_FULLSCREEN_VIDEO)

Would you explain what is NATIVE_FULLSCREEN_VIDEO?
Shouldn't it be a USE() flag instead of an ENABEL() flag?

http://trac.webkit.org/wiki/Porting%20Macros%20plan
* USE() - use a particular third-party library or optional OS service
* ENABLE() - turn on a specific feature of WebKit

[Min Qin]

For android, fullscreen video is implemented by passing a java video surfaceView object to the webmediaplayer. As a consequence, NATIVE_FULLSCREEN_VIDEO means the fullscreen video is implemented by the native system view,  and is not implemented by any of the webkit code.

Probably USE makes more sense here, I will have another change to fix this,

(In reply to comment #19)
> (From update of attachment 139467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139467&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:772
> > +#if ENABLE(PLUGIN_PROXY_FOR_VIDEO) || ENABLE(NATIVE_FULLSCREEN_VIDEO)
> 
> Would you explain what is NATIVE_FULLSCREEN_VIDEO?
> Shouldn't it be a USE() flag instead of an ENABEL() flag?
> 
> http://trac.webkit.org/wiki/Porting%20Macros%20plan
> * USE() - use a particular third-party library or optional OS service
> * ENABLE() - turn on a specific feature of WebKit

[Benjamin Poulain]

Comment on attachment 139467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139467&action=review

> Source/WebCore/platform/graphics/MediaPlayer.h:330
> +    bool enterFullscreen() const;

Why would you change this to be const?? That is not a method free of side effects on the state of the media player.

[Benjamin Poulain]

Comment on attachment 139467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139467&action=review

> Source/WebCore/platform/graphics/MediaPlayer.h:330
> +    bool enterFullscreen() const;

Why would you change this to be const?? That is not a method free of side effects on the state of the media player.

[Alexis Menard (darktears)]

Comment on attachment 139467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139467&action=review

> Source/WebKit/chromium/ChangeLog:11
> +        This change makes it possble for WebViewImpl::enterFullScreenForElement()

Typo here, be careful next time.

> Source/WebKit/chromium/ChangeLog:14
> +        Sorry, there is a merge error during the previous commit, resolved now

This sentence doesn't make sense to me. Be careful next time.

>>> Source/WebCore/platform/graphics/MediaPlayer.cpp:772
>>> +#if ENABLE(PLUGIN_PROXY_FOR_VIDEO) || ENABLE(NATIVE_FULLSCREEN_VIDEO)
>> 
>> Would you explain what is NATIVE_FULLSCREEN_VIDEO?
>> Shouldn't it be a USE() flag instead of an ENABEL() flag?
>> 
>> http://trac.webkit.org/wiki/Porting%20Macros%20plan
>> * USE() - use a particular third-party library or optional OS service
>> * ENABLE() - turn on a specific feature of WebKit
> 
> 

I would like to know too.

>>> Source/WebCore/platform/graphics/MediaPlayer.h:330
>>> +    bool enterFullscreen() const;
>> 
>> Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
> 
> Why would you change this to be const?? That is not a method free of side effects on the state of the media player.

In addition of the const I really dislike the asymmetry with exitFullscreen();
Other question : Why the bool as a return value? Could you elaborate when and which scenario it would return false?

> Source/WebKit/chromium/public/WebMediaPlayer.h:187
> +    // Instuct WebMediaPlayer to enter/exit fullscreen.

Typo here.

[Benjamin Poulain]

> > Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
> 
> In addition of the const I really dislike the asymmetry with exitFullscreen();
> Other question : Why the bool as a return value? Could you elaborate when and which scenario it would return false?

That is a good point. With all the existing code ignoring the return value, this is weird.

[Min Qin]

(In reply to comment #22)
> (From update of attachment 139467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139467&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.h:330
> > +    bool enterFullscreen() const;
> 
> Why would you change this to be const?? That is not a method free of side effects on the state of the media player.

So this is what could happen for chrome on android, when calling enterFullscreen() it could actually fail for the following 2 reasons:
1. if the MediaPlayerPrivate object is gone.
2. the native media player is already in fullscreen mode.

Since this return value is not used when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is set, returning true should just be fine.

[Min Qin]

(In reply to comment #23)
> (From update of attachment 139467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139467&action=review
> 
> > Source/WebKit/chromium/ChangeLog:11
> > +        This change makes it possble for WebViewImpl::enterFullScreenForElement()
> 
> Typo here, be careful next time.
> 
> > Source/WebKit/chromium/ChangeLog:14
> > +        Sorry, there is a merge error during the previous commit, resolved now
> 
> This sentence doesn't make sense to me. Be careful next time.
> 
> >>> Source/WebCore/platform/graphics/MediaPlayer.cpp:772
> >>> +#if ENABLE(PLUGIN_PROXY_FOR_VIDEO) || ENABLE(NATIVE_FULLSCREEN_VIDEO)
> >> 
> >> Would you explain what is NATIVE_FULLSCREEN_VIDEO?
> >> Shouldn't it be a USE() flag instead of an ENABEL() flag?
> >> 
> >> http://trac.webkit.org/wiki/Porting%20Macros%20plan
> >> * USE() - use a particular third-party library or optional OS service
> >> * ENABLE() - turn on a specific feature of WebKit
> > 
> > 
> 
> I would like to know too.

This has been fixed in https://bugs.webkit.org/show_bug.cgi?id=85316

> 
> >>> Source/WebCore/platform/graphics/MediaPlayer.h:330
> >>> +    bool enterFullscreen() const;
> >> 
> >> Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
> > 
> > Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
> 
> In addition of the const I really dislike the asymmetry with exitFullscreen();
> Other question : Why the bool as a return value? Could you elaborate when and which scenario it would return false?
> 

So if you take a look at the change I made in WebViewImpl::enterFullScreenForElement(WebCore::Element* element) in this patch, you will see this return value got used. We need the return value to know whether enterfullscreen actually succeeded, and whether we should set the current m_provisionalFullScreenElement element to that media element.


> > Source/WebKit/chromium/public/WebMediaPlayer.h:187
> > +    // Instuct WebMediaPlayer to enter/exit fullscreen.
> 
> Typo here.

ok, will fix the typo later

[Benjamin Poulain]

> > Why would you change this to be const?? That is not a method free of side effects on the state of the media player.

You have not explained why this is valid regarding const-correctness.

> So this is what could happen for chrome on android, when calling enterFullscreen() it could actually fail for the following 2 reasons:
> 1. if the MediaPlayerPrivate object is gone.
> 2. the native media player is already in fullscreen mode.
> 
> Since this return value is not used when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is set, returning true should just be fine.

This sounds like reasons to have a "canEnterFullScreen()" method or something similar. Now you changed the API so that enterFullScreen() can fail. The fact that the existing code ignore this is no excuse.

[Min Qin]

(In reply to comment #27)
> > > Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
> 
> You have not explained why this is valid regarding const-correctness.

Because when we call this, it won't change the object. If the const-correctness really affects the const-correctness in your implementation, we can remove that const away.  
> 
> > So this is what could happen for chrome on android, when calling enterFullscreen() it could actually fail for the following 2 reasons:
> > 1. if the MediaPlayerPrivate object is gone.
> > 2. the native media player is already in fullscreen mode.
> > 
> > Since this return value is not used when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is set, returning true should just be fine.
> 
> This sounds like reasons to have a "canEnterFullScreen()" method or something similar. Now you changed the API so that enterFullScreen() can fail. The fact that the existing code ignore this is no excuse.

(In reply to comment #27)
> > > Why would you change this to be const?? That is not a method free of side effects on the state of the media player.
> 
> You have not explained why this is valid regarding const-correctness.
> 
> > So this is what could happen for chrome on android, when calling enterFullscreen() it could actually fail for the following 2 reasons:
> > 1. if the MediaPlayerPrivate object is gone.
> > 2. the native media player is already in fullscreen mode.
> > 
> > Since this return value is not used when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is set, returning true should just be fine.
> 
> This sounds like reasons to have a "canEnterFullScreen()" method or something similar. Now you changed the API so that enterFullScreen() can fail. The fact that the existing code ignore this is no excuse.

So in the NATIVE_FULLSCREEN_VIDEO case, there is nothing in the DOM that blocks the media element from going fullscreen. Only the native mediaplayer knows whether it can enter fullscreen or not. And also because that the MediaPlayerPrivate can get deleted when this is called, so enterFullscreen() is not guaranteed to be successful.

[Benjamin Poulain]

> > You have not explained why this is valid regarding const-correctness.
> 
> Because when we call this, it won't change the object. If the const-correctness really affects the const-correctness in your implementation, we can remove that const away.  

In WebKit, we consider the semantic const correctness. not the C++ type checking.

Yes, I think the "const" should not be there.


> > > Since this return value is not used when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is set, returning true should just be fine.
> > 
> > This sounds like reasons to have a "canEnterFullScreen()" method or something similar. Now you changed the API so that enterFullScreen() can fail. The fact that the existing code ignore this is no excuse.
> 
> So in the NATIVE_FULLSCREEN_VIDEO case, there is nothing in the DOM that blocks the media element from going fullscreen. Only the native mediaplayer knows whether it can enter fullscreen or not. And also because that the MediaPlayerPrivate can get deleted when this is called, so enterFullscreen() is not guaranteed to be successful.

If you chance the contract of an API, you have to do it all the way.

Please explain how can the MediaPlayerPrivate get deleted in MediaPlayer::enterFullScreen() because that is relevant to the other use of this API.

[Min Qin]

(In reply to comment #29)
> > > You have not explained why this is valid regarding const-correctness.
> > 
> > Because when we call this, it won't change the object. If the const-correctness really affects the const-correctness in your implementation, we can remove that const away.  
> 
> In WebKit, we consider the semantic const correctness. not the C++ type checking.
> 
> Yes, I think the "const" should not be there.
> 
> 
> > > > Since this return value is not used when ENABLE(PLUGIN_PROXY_FOR_VIDEO) is set, returning true should just be fine.
> > > 
> > > This sounds like reasons to have a "canEnterFullScreen()" method or something similar. Now you changed the API so that enterFullScreen() can fail. The fact that the existing code ignore this is no excuse.
> > 
> > So in the NATIVE_FULLSCREEN_VIDEO case, there is nothing in the DOM that blocks the media element from going fullscreen. Only the native mediaplayer knows whether it can enter fullscreen or not. And also because that the MediaPlayerPrivate can get deleted when this is called, so enterFullscreen() is not guaranteed to be successful.
> 
> If you chance the contract of an API, you have to do it all the way.
>

ok, I will add a canEnterFullscreen() call in another change, and revert the changes to the enterFullscreen() call.

> Please explain how can the MediaPlayerPrivate get deleted in MediaPlayer::enterFullScreen() because that is relevant to the other use of this API.

I am not sure why this happens, but this did happen once a while in our stacktrace. Could be a bug in javascript engine, or could be something else.

[Benjamin Poulain]

> ok, I will add a canEnterFullscreen() call in another change, and revert the changes to the enterFullscreen() call.

Great, thanks for following up on this. Feel free to add me in CC if you need reviews.

> > Please explain how can the MediaPlayerPrivate get deleted in MediaPlayer::enterFullScreen() because that is relevant to the other use of this API.
> 
> I am not sure why this happens, but this did happen once a while in our stacktrace. Could be a bug in javascript engine, or could be something else.

This could be important. Please file a bug next time you encounter this issue.