Expose a flag so that fullscreen video on android can work with FULLSCREEN_API
Created attachment 138032 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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.
Created attachment 138034 [details] Patch
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.
Created attachment 138125 [details] Patch
This looks fine to me, but I am not marking r+ pending approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org.
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?
Created attachment 138137 [details] Patch
Comments added for enter and exit fullscreen
Ping, anyone from the chromium side can review this?
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?
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.
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
Created attachment 139467 [details] Patch
Sorry, there is a merge error during the previous commit process. Resolved the conflicts now
Comment on attachment 139467 [details] Patch Clearing flags on attachment: 139467 Committed r115661: <http://trac.webkit.org/changeset/115661>
All reviewed patches have been landed. Closing bug.
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
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
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.
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.
> > 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.
(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.
(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
> > 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.
(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.
> > 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.
(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.
> 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.