[webkit-reviews] review denied: [Bug 68911] Sync requestAnimationFrame callback to CVDisplayLink on Mac : [Attachment 110172] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Chris Marrin
<cmarrin at apple.com>'s request for review:
Bug 68911: Sync requestAnimationFrame callback to CVDisplayLink on Mac
https://bugs.webkit.org/show_bug.cgi?id=68911

Attachment 110172: Patch
https://bugs.webkit.org/attachment.cgi?id=110172&action=review

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


Does this all work when subframe is calling rAF?

> Source/WebKit/mac/WebView/WebView.mm:6109
> +    WebView* webView = (WebView*) data;

The * goes on the right for Obj-C pointers. No space after the ). You should
also use a C++-style cast.

> Source/WebKit/mac/WebView/WebView.mm:6114
> +	   webView->_private->animationScheduleState = AnimationStateIdle;

Is there any danger that webView->_private may have gone away already?

> Source/WebKit/mac/WebView/WebView.mm:6125
> +    WebView* webView = (WebView*) data;

Ditto.

> Source/WebKit/mac/WebView/WebView.mm:6128
> +    double outputTimeSeconds = static_cast<double>(outputTime->videoTime) /
static_cast<double>(outputTime->videoTimeScale);

Can you not just use the hostTime field for these?

> Source/WebKit/mac/WebView/WebView.mm:6129
> +    double webKitNow = currentTime();

Is this using the same timebase as the CVTimeStamp times?

> Source/WebKit/mac/WebView/WebView.mm:6146
> +	   ASSERT(!error);

I think you should actually handle this error (e.g. return). There may be
situations where it fails (e.g. a headless machine).

> Source/WebKit/mac/WebView/WebView.mm:6152
> +	   error = CVDisplayLinkStart(_private->displayLink);
> +	   ASSERT(!error);

Ditto.

> Source/WebKit2/UIProcess/WebPageProxy.h:416
> +    void changeScreen(CGDirectDisplayID);

I think this name is a bit too generic. Maybe windowScreenDidChange().

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:275
> +    shutdownDisplayLink();

This feels like it might be too late. Can a WebPage be disconnected from the
view but stay alive?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:602
> +    double nowSeconds = static_cast<double>(now->videoTime) /
static_cast<double>(now->videoTimeScale);
> +    double outputTimeSeconds = static_cast<double>(outputTime->videoTime) /
static_cast<double>(outputTime->videoTimeScale);
> +    double webKitNow = currentTime();

Same comments about time as above.

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:626
> +    if (webPage->corePage()->mainFrame() &&
webPage->corePage()->mainFrame()->view())
> +	  
webPage->corePage()->mainFrame()->view()->serviceScriptedAnimations(convertSeco
ndsToDOMTimeStamp(timestamp));

Stash webPage->corePage()->mainFrame() in a local var?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:633
> +	   if (!m_displayID)
> +	       m_displayID = CGMainDisplayID();

Is the display ID always non-zero?

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:642
> +	   CVReturn error = CVDisplayLinkCreateWithCGDisplay(m_displayID,
&m_displayLink);
> +	   ASSERT(!error);
> +    
> +	   error = CVDisplayLinkSetOutputCallback(m_displayLink,
displayLinkCallback, this);
> +	   ASSERT(!error);
> +    
> +	   error = CVDisplayLinkStart(m_displayLink);
> +	   ASSERT(!error);

I think you need to handle these errors.