[webkit-reviews] review denied: [Bug 68911] Sync requestAnimationFrame callback to CVDisplayLink on Mac : [Attachment 110782] Patch attempting to fix cr-linux build issue

[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 110782: Patch attempting to fix cr-linux build issue
https://bugs.webkit.org/attachment.cgi?id=110782&action=review

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


This is pretty close; just some nits to tidy up.

> Source/WebCore/dom/Document.cpp:5129
>	   m_scriptedAnimationController =
ScriptedAnimationController::create(this);
> +	  
m_scriptedAnimationController->windowScreenDidChange(page()->displayID());

I think it would be better to pass in the displayID to the
ScriptedAnimationController ctor.

> Source/WebCore/dom/ScriptedAnimationController.cpp:156
> +#if PLATFORM(MAC)
> +   
DisplayRefreshMonitorManager::sharedManager()->windowScreenDidChange(displayID,
this);
> +#else

Shame to have Mac #ifdefs in this file. Maybe define USE_DISPLAY_MONITOR
somewhere?

> Source/WebCore/dom/ScriptedAnimationController.cpp:166
> +	   m_useTimer =
!DisplayRefreshMonitorManager::sharedManager()->scheduleAnimation(this);

This is a little hard to grok. Maybe 
if (DisplayRefreshMonitorManager::sharedManager()->scheduleAnimation(this))
  return;
 
m_useTimer = true
...

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:78
> +    for (size_t i = 0; i < monitor->m_clients.size(); ++i)
> +	   monitor->m_clients[i]->fireDisplayRefreshIfNeeded(timestamp);

Can fireDisplayRefreshIfNeeded modify the m_clients array?

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:84
> +    static DisplayRefreshMonitorManager* staticManager = new
DisplayRefreshMonitorManager;
> +    return staticManager;

DEFINE_STATIC_LOCAL perhaps, and return a reference?

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:87
> +size_t DisplayRefreshMonitorManager::findMonitor(PlatformDisplayID
displayID) const

Why not just
DisplayRefreshMonitor* monitorForDisplay(PlatformDisplayID)
?

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:101
> +    size_t i = findMonitor(client->m_displayID);

I'd use monitorIndex or index, reserving i for use in loops.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:106
> +	   monitor = new DisplayRefreshMonitor(client->m_displayID);
> +	   m_monitors.append(monitor);

m_monitors could contain OwnPtrs to avoid manual new/delete.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:123
> +    size_t i = findMonitor(client->m_displayID);
> +    if (i == notFound)
> +	   return;
> +    
> +    DisplayRefreshMonitor* monitor = m_monitors[i];
> +    if (monitor->removeClient(client)) {

You've done two array walks here; should be able to do it in one.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:140
> +    client->scheduleAnimation();
> +    return m_monitors[i]->scheduleAnimation();

Maybe too many scheduleAnimation() methods which don't really all mean the same
thing. Reading the code, I'm not sure why there are two calls here.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.cpp:150
> +    client->m_displayID = displayID;
> +    client->m_displayIDIsSet = true; 

Have a setDisplayID method?

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:60
> +    bool m_scheduled;
> +    PlatformDisplayID m_displayID;
> +    bool m_displayIDIsSet;

Put the bools next to eachother to optimize packing.

> Source/WebCore/platform/graphics/DisplayRefreshMonitor.h:118
> +    bool scheduleAnimation(DisplayRefreshMonitorClient*);

Should say what the return value means.

> Source/WebKit/mac/WebView/WebView.mm:3296
> +    _private->page->windowScreenDidChange((PlatformDisplayID)[[[[[self
window] screen] deviceDescription] objectForKey:@"NSScreenNumber"] intValue]);

Might want to do some null checking on private and private->page.

> Source/WebKit/mac/WebView/WebView.mm:3324
> +    [self _windowDidChangeScreen:notification];

You shouldn't send an unrelated notification to the _windowDidChangeScreen
method.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:1886
> +    [self _windowDidChangeScreen:notification];

Ditto.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1167
> +    // FIXME: IPC has trouble sending params it doesn't understand, so cast
into a common type
> +    process()->send(Messages::WebPage::WindowScreenDidChange(displayID),
m_pageID);

It's unclear what is actionable about this FIXME. Does it not work here?