[webkit-reviews] review granted: [Bug 49481] Implement WebKit Full Screen support : [Attachment 77134] NewFullScreen-Part-2-The-Document

[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 49481: Implement WebKit Full Screen support
https://bugs.webkit.org/show_bug.cgi?id=49481

Attachment 77134: NewFullScreen-Part-2-The-Document
https://bugs.webkit.org/attachment.cgi?id=77134&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=77134&action=review

> WebCore/dom/Document.cpp:3375
>	  
m_fullScreenElement->dispatchEvent(Event::create(eventNames().webkitfullscreenc
hangeEvent, true, false));

Is this a safe time to be calling script?

> WebCore/dom/Document.cpp:4838
> +    setNeedsStyleRecalc(SyntheticStyleChange);
> +    documentElement()->setNeedsStyleRecalc(SyntheticStyleChange);

Why are these synthetic style changes needed?

> WebCore/dom/Document.cpp:4839
> +    m_fullScreenElement->setNeedsStyleRecalc(SyntheticStyleChange);

I assume this one is required to get m_fullScreenElement a layer and
compositing layer?

> WebCore/dom/Document.cpp:4851
> +   
m_fullScreenElement->dispatchEvent(Event::create(eventNames().webkitfullscreenc
hangeEvent, true, false));

Is this a safe time to be calling script? Who calls
webkitDidEnterFullScreenForElement()?

It might be safer to dispatch the event on a zero-delay timer.

> WebCore/dom/Document.cpp:4860
> +    setNeedsStyleRecalc(SyntheticStyleChange);
> +    documentElement()->setNeedsStyleRecalc(SyntheticStyleChange);

Ditto.

> WebCore/dom/Document.cpp:4896
> +void Document::setFullScreenRendererSize(int width, int height)

Use const IntSize&