[webkit-reviews] review canceled: [Bug 43099] Add JavaScript API to allow a page to go fullscreen : [Attachment 63936] Part 1: FullScreen-ChangeLogs

[bugzilla-daemon at webkit.org]

Jer Noble <jer.noble at apple.com> has canceled 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 63936: Part 1: FullScreen-ChangeLogs
https://bugs.webkit.org/attachment.cgi?id=63936&action=review

------- Additional Comments from Jer Noble <jer.noble at apple.com>
This patch makes obsolete and integrates patches Part 1, 2, & 3 above. 
WebKitFullScreenChangeEvent has been removed, and Eric's reviews of Parts 2 & 3
have been addressed:


(In reply to comment #81)
> (From update of attachment 63955 [details])
> > +#if ENABLE(FULLSCREEN_API)
> > +#include "WebKitFullScreenChangeEvent.h"
> > +#endif
> 
> Not necessary.

Removed.

> > +#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.

Added a comment here.

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

Removed.

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

Removed.

> +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".

Actually, in a previous review, it was decided drop the "WithFlags/WithKeys"
part entirely.	Regardless, the Document::webkitRequestFullScreenForElement()
function now takes a flags parameter.

> > +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.

Done.

> > +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.

Done.

> > +	     FULLSCREENCHANGE_LISTENER		  = 0x8000,
> 
> Not necessary once you get rid of the custom event type.

Removed.

> > +void Element::webkitRequestFullScreen(unsigned short flags)
> > +{
> > +	 document()->webkitRequestFullScreenForElement(this, flags ==
ALLOW_KEYBOARD_INPUT);
> > +}
> 
> Should just pass the flags through to webkitRequestFullScreenForElement

Done.

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

Removed.

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

Removed.