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

[bugzilla-daemon at webkit.org]

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

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
> +#if ENABLE(FULLSCREEN_API)
> +#include "WebKitFullScreenChangeEvent.h"
> +#endif

Not necessary.


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

It took me a minute to figure out why this is necessary, so a comment would be
nice.

> +#if ENABLE(FULLSCREEN_API)
> +    else if (eventType == "WebKitFullScreenChange")
> +	   event = WebKitFullScreenChangeEvent::create();
> +#endif

Not necessary.

> +#if ENABLE(FULLSCREEN_API)
> +    else if (eventType == eventNames().webkitfullscreenchangeEvent)
> +	   addListenerType(FULLSCREENCHANGE_LISTENER);
> +#endif

Ditto.

+void Document::webkitRequestFullScreenForElementWithKeys(Element* element,
bool keysEnabled)

Shouldn't this take the flags word so we don't have to create a new method once
there are new flags?

+void Document::webkitRequestFullScreenForElementWithKeys(Element* element,
bool keysEnabled)

This should be "webkitRequestFullScreenForElementWithFlags".


> +void Document::webkitWillEnterFullScreenForElement(Element* element)
> +{
> +    ASSERT(page() && page()->settings()->fullScreenEnabled());
> +
> +    m_fullScreenElement = element;
> +    m_isFullScreen = true;
> +    documentElement()->setNeedsStyleRecalc(FullStyleChange);
> +    if (m_fullScreenElement)
> +	   m_fullScreenElement->setNeedsStyleRecalc(FullStyleChange);
> +    updateStyleIfNeeded();
> +    
> +    if (m_fullScreenElement)
> +	  
m_fullScreenElement->dispatchEvent(WebKitFullScreenChangeEvent::create());
> +    else 
> +	  
documentElement()->dispatchEvent(WebKitFullScreenChangeEvent::create());
> +}

It shouldn't be legal to pass a NULL element, I would ASSERT and remove support
for it.


> +void Document::webkitDidExitFullScreenForElement(Element* element)
> +{
> +    m_isFullScreen = false;
> +    m_areKeysEnabledInFullScreen = false;
> +    // m_fullScreenElement has already been cleared; recalc the style of 
> +    // the passed in element instead.
> +    
> +    if (element)
> +	   element->setNeedsStyleRecalc(FullStyleChange);
> +    documentElement()->setNeedsStyleRecalc(FullStyleChange);
> +    updateStyleIfNeeded();
> +    
> +    if (element)
> +	   element->dispatchEvent(WebKitFullScreenChangeEvent::create());
> +    else
> +	  
documentElement()->dispatchEvent(WebKitFullScreenChangeEvent::create());
> +}

Ditto.

> +	   FULLSCREENCHANGE_LISTENER		= 0x8000,

Not necessary once you get rid of the custom event type.


> +void Element::webkitRequestFullScreen(unsigned short flags)
> +{
> +    document()->webkitRequestFullScreenForElement(this, flags ==
ALLOW_KEYBOARD_INPUT);
> +}

Should just pass the flags through to webkitRequestFullScreenForElement



> +#if ENABLE(FULLSCREEN_API)
> +bool Event::isWebKitFullScreenChangeEvent() const
> +{
> +    return false;
> +}

Not needed.

> +#if ENABLE(FULLSCREEN_API)
> +	   virtual bool isWebKitFullScreenChangeEvent() const;
> +#endif

Ditto.

r= for now since a fair amount of cleanup is still needed.