[Webkit-unassigned] [Bug 84414] Expose a flag so that fullscreen video on android can work with FULLSCREEN_API

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=84414





--- Comment #26 from Min Qin <qinmin at chromium.org>  2012-05-09 13:31:21 PST ---
(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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.