[webkit-reviews] review granted: [Bug 43099] Add JavaScript API to allow a page to go fullscreen : [Attachment 64545] Part 1-3: FullScreen-WebCore-NewAPI

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson 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 64545: Part 1-3: FullScreen-WebCore-NewAPI
https://bugs.webkit.org/attachment.cgi?id=64545&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
> +++ WebCore/ChangeLog (working copy)
>
> +	   * html/HTMLIFrameElement.h:

Looks like this is left over from a previous version.

> +	   * rendering/MediaControlElements.cpp:
> +	   (WebCore::MediaControlFullscreenButtonElement::defaultEventHandler):
The full screen button now toggles full screen mode (previously, it only
entered).
> +	   * rendering/style/RenderStyleConstants.h: Added new style constants.

Ditto.

> +    
> +#if ENABLE(FULLSCREEN_API)
> +void Document::webkitRequestFullScreenForElement(Element* element, unsigned
short flags)
> +{
> +    if (!page() || !page()->settings()->fullScreenEnabled())
> +	   return;
> +
> +    if (!element)
> +	   element = documentElement();
> +    
> +    if (!page()->chrome()->client()->supportsFullScreenForElement(element))
> +	   return;
> +    
> +    m_fullScreenElement = 0;
> +    m_areKeysEnabledInFullScreen = flags & Element::ALLOW_KEYBOARD_INPUT;
> +    page()->chrome()->client()->enterFullScreenForElement(element);

If enterFullScreenForElement fails and m_fullScreenElement is non-NULL, we
won't ever fire a webkitfullscreenchange event. Is this worth worrying about?

> +void Document::webkitDidExitFullScreenForElement(Element* element)
> +{
> +    ASSERT(element);
> +    m_isFullScreen = false;
> +    m_areKeysEnabledInFullScreen = false;
> +    // m_fullScreenElement has already been cleared; recalc the style of 
> +    // the passed in element instead.
> +    
> +    element->setNeedsStyleRecalc(FullStyleChange);

The blank line should go above the comment instead of below it.


> Index: WebCore/dom/Element.cpp
> ===================================================================
> --- WebCore/dom/Element.cpp	(revision 65463)
> +++ WebCore/dom/Element.cpp	(working copy)
> @@ -1441,7 +1441,7 @@ void Element::normalizeAttributes()
>	       attr->normalize();
>      }
>  }
> -
> +    
Don't need the new white space.

r=m with these minor changes.