[webkit-reviews] review denied: [Bug 122016] [CoordinatedGraphics] ASSERTION FAILED: !m_flushingLayers (after r156291) : [Attachment 213691] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Oct 9 02:02:46 PDT 2013


Noam Rosenthal <noam at webkit.org> has denied Sergio Correia
<sergio.correia at openbossa.org>'s request for review:
Bug 122016: [CoordinatedGraphics] ASSERTION FAILED: !m_flushingLayers (after
r156291)
https://bugs.webkit.org/show_bug.cgi?id=122016

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

------- Additional Comments from Noam Rosenthal <noam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=213691&action=review


> Source/WebCore/ChangeLog:11
> +	   To prevent calling scheduleLayerFlush() inside
flushCompositingState(),
> +	   which is incorrect, we now only call m_client->notifyFlushRequired()
--
> +	   which will trigger scheduleLayerFlush() -- if we are not already
flushing
> +	   layer changes.

Please use - instead of --, and remove ", which is incorrect", which is
unnecessary :)

>
Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:
93
> +    notifyFlushRequired(nullptr);

I think we should pass the layer here, even if it's currently not in use.
Having nullptr here is hard to read. Alternatively, have a nullptr default for
CompositingCoordinator::notifyFlushRequired

>
Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:
275
> +    notifyFlushRequired(nullptr);

Ditto

>
Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp:
356
> +    notifyFlushRequired(nullptr);

Ditto

>
Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cp
p:54
> +    if (client() && m_coordinator &&
!m_coordinator->isFlushingLayerChanges()) {

Why do we need && m_coordinator? you already assert for this earlier.


More information about the webkit-reviews mailing list