[webkit-reviews] review granted: [Bug 186684] Address fullscreen api CSS env feedback : [Attachment 343113] Patch

[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 186684: Address fullscreen api CSS env feedback
https://bugs.webkit.org/show_bug.cgi?id=186684

Attachment 343113: Patch

https://bugs.webkit.org/attachment.cgi?id=343113&action=review




--- Comment #11 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 343113
  --> https://bugs.webkit.org/attachment.cgi?id=343113
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=343113&action=review

> Source/WebCore/dom/ConstantPropertyMap.h:63
>      void setFullscreenAutoHideDelay(double);
> +    void setFullscreenAutoHideDuration(double);

These should take Seconds

> Source/WebCore/page/Page.cpp:2408
> +    for (Frame* frame = &mainFrame(); frame; frame =
frame->tree().traverseNext()) {
> +	   if (!frame->document())
> +	       continue;
> +	  
frame->document()->constantProperties().setFullscreenAutoHideDuration(duration)
;
> +    }

This is weird; why don't we have a singleton repository of constant properties?
(not for this patch tho).

> Source/WebCore/page/Page.h:356
>      WEBCORE_EXPORT void setFullscreenAutoHideDelay(double);

Seconds

> Source/WebCore/page/Page.h:358
> +    WEBCORE_EXPORT void setFullscreenAutoHideDuration(double);
> +    WEBCORE_EXPORT void setFullscreenControlsHidden(bool);

Seconds

> Source/WebCore/testing/Internals.cpp:503
> +    page.setFullscreenAutoHideDuration(0);

This would be 0_s if you use Seconds.

> Source/WebCore/testing/Internals.cpp:504
> +    page.setFullscreenInsets(FloatBoxExtent());

Can this be page.setFullscreenInsets({ });

> Source/WebKit/UIProcess/WebFullScreenManagerProxy.cpp:141
> +void WebFullScreenManagerProxy::setFullscreenAutoHideDuration(double
duration)
> +{
> +   
m_page->process().send(Messages::WebFullScreenManager::SetFullscreenAutoHideDur
ation(duration), m_page->pageID());
> +}
> +
> +void WebFullScreenManagerProxy::setFullscreenControlsHidden(bool hidden)
> +{
> +   
m_page->process().send(Messages::WebFullScreenManager::SetFullscreenControlsHid
den(hidden), m_page->pageID());
> +}

Do we really need to send a separate IPC message for each of these? Can we pass
them en masse somewhere?

> Source/WebKit/UIProcess/WebFullScreenManagerProxy.h:77
>      void setFullscreenAutoHideDelay(double);
> +    void setFullscreenAutoHideDuration(double);

Seconds

> Source/WebKit/WebProcess/FullScreen/WebFullScreenManager.cpp:184
> +    setFullscreenInsets(FloatBoxExtent());

{ }

> Source/WebKit/WebProcess/FullScreen/WebFullScreenManager.h:86
>      void setFullscreenAutoHideDelay(double);
> +    void setFullscreenAutoHideDuration(double);
> +    void setFullscreenControlsHidden(bool);

Seconds