[webkit-reviews] review granted: [Bug 43099] Add JavaScript API to allow a page to go fullscreen : [Attachment 63628] FullScreen-WebCore

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 43099: Add JavaScript API to allow a page to go fullscreen
https://bugs.webkit.org/show_bug.cgi?id=43099

Attachment 63628: FullScreen-WebCore
https://bugs.webkit.org/attachment.cgi?id=63628&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>


> +#if ENABLE(FULLSCREEN_API)
> +    ASSERT(n);
> +    if (n->contains(m_fullScreenElement.get())) {
> +	   ASSERT(n != documentElement());
> +	   m_fullScreenElement = documentElement();
> +	  
m_fullScreenElement->dispatchEvent(WebKitFullScreenChangeEvent::create());
> +    }
> +#endif

Who's actually telling the UA that it's leaving fullscreen in this case?

Also, what happens when an <iframe>, <object>, or <embed> is fullscreen, then
one of its parent nodes is removed? I think this deserves a testcase.

> +void Document::webkitRequestFullScreenForElementWithKeys(Element* element,
bool keysEnabled)
> +{
> +    if (!page() || !page()->settings()->fullScreenEnabled())
> +	   return;
> +
> +    if (!element)
> +	   element = documentElement();
> +    
> +    if (!page()->chrome()->client()->supportsFullscreenForElement(element))
> +	   return;
> +    
> +    m_fullScreenElement = 0;
> +    m_areKeysEnabledInFullScreen = keysEnabled;
> +    page()->chrome()->client()->enterFullscreenForElement(element);
> +}

Should this method bail earlier if the doc is already fullscreen?

> +void Document::webkitWillEnterFullScreenForElement(Element* element)
> +{
> +    ASSERT(page() && page()->settings()->fullScreenEnabled());
> +
> +    m_fullScreenElement = element;
> +    m_isFullScreen = true;
> +    documentElement()->setNeedsStyleRecalc(FullStyleChange);

I'd like to see a comment saying that Document::isFullscreen() applies some
pseudoclasses,
so we need to do the style recalc.

> +    if (m_fullScreenElement)

Hadn't we better always have a m_fullScreenElement here, so maybe ASSERT()?

> Index: WebCore/html/HTMLIFrameElement.cpp
> ===================================================================
> --- WebCore/html/HTMLIFrameElement.cpp	(revision 64653)
> +++ WebCore/html/HTMLIFrameElement.cpp	(working copy)
> @@ -38,6 +38,9 @@ using namespace HTMLNames;
>  
>  inline HTMLIFrameElement::HTMLIFrameElement(const QualifiedName& tagName,
Document* document)
>      : HTMLFrameElementBase(tagName, document)
> +#if ENABLE(FULLSCREEN_API)
> +    , m_allowsFullScreen(false)
> +#endif
>  {
>      ASSERT(hasTagName(iframeTag));
>  }
> @@ -132,6 +135,10 @@ void HTMLIFrameElement::parseMappedAttri
>      else if (attr->name() == sandboxAttr)
>	   setSandboxFlags(parseSandboxAttribute(attr));
>  #endif
> +#if ENABLE(FULLSCREEN_API)
> +    if (attr->name() == webkitallowfullscreenAttr)
> +	   m_allowsFullScreen = true;
> +#endif
>      else
>	   HTMLFrameElementBase::parseMappedAttribute(attr);

I think we should postpone the frame/object/emebed-related stuff for a later
patch.
There are lots of complexities we need to figure out, and make tests for.

I think this is generally OK, but I'd like to see more consideration of actions
that should force us to leave fullscreen.