[webkit-reviews] review granted: [Bug 70367] [chromium] Canvas2D should rate-limit drawing to prevent swamping the GPU process : [Attachment 112376] new patch; added rate limiter cancelling

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Stephen White
<senorblanco at chromium.org>'s request for review:
Bug 70367: [chromium] Canvas2D should rate-limit drawing to prevent swamping
the GPU process
https://bugs.webkit.org/show_bug.cgi?id=70367

Attachment 112376: new patch; added rate limiter cancelling
https://bugs.webkit.org/attachment.cgi?id=112376&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=112376&action=review


R=me, usual assortment of nits

> Source/WebCore/platform/graphics/chromium/RateLimiter.cpp:8
> + *	  * Redistributions of source code must retain the above copyright

should use 2-clause license for new files

> Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:130
> +    if (layerTreeHost() && contextChanged)
> +	   layerTreeHost()->stopRateLimiter(m_context);

do we call setContext(NULL) before destroying a WebGLLayerChromium?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:438
> +    RateLimiterMap::iterator i = m_rateLimiters.find(context);

we normally use 'it' for iterators and reserve 'i' for loop counters

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:450
> +    RateLimiterMap::iterator i = m_rateLimiters.find(context);

same here - i->it

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:53
> +typedef HashMap<GraphicsContext3D*, RefPtr<RateLimiter> > RateLimiterMap;

you could put this typedef inside the class next to the declaration so it's not
in the WebCore namespace