[Chromium] Avoid ASSERT_NOT_REACHED() from creating FBO with content texture of size 0
Created attachment 116849 [details] Patch
Hrm on second though, we might want to have that check also before this early return, right? // If neither this layer nor any of its children were added, early out. if (sortingStartIndex == descendants.size()) return; Do we need to duplicate the logic instead where we call renderSurface->clearLayerList()? (In reply to comment #1) > Created an attachment (id=116849) [details] > Patch
Comment on attachment 116849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116849&action=review > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:383 > // If neither this layer nor any of its children were added, early out. > if (sortingStartIndex == descendants.size()) > return; This little patch of code should move too, because descendants.size() could be zero, causing a problem for descendants.at(0) in the sort at the end.
Comment on attachment 116849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116849&action=review Don't you also need to clear the layer's RS at the drawOpacity == 0 early-out? > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:382 > if (sortingStartIndex == descendants.size()) You'll also want to clear the RS here. Maybe it would be best to split the cleanup into a separate function that calls: if (layer->renderSurface() && layer != rootLayer) renderSurfaceList.remove(layer->renderSurface()); layer->clearRenderSurface(); > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:446 > + renderSurfaceLayerList.removeLast(); You can nest this inside the previous if (layer->renderSurface() && layer != rootLayer) {} clause .
Created attachment 116863 [details] Patch
(In reply to comment #3) > (From update of attachment 116849 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116849&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:383 > > // If neither this layer nor any of its children were added, early out. > > if (sortingStartIndex == descendants.size()) > > return; > > This little patch of code should move too, because descendants.size() could be zero, causing a problem for descendants.at(0) in the sort at the end. Done.
(In reply to comment #4) > (From update of attachment 116849 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116849&action=review > > Don't you also need to clear the layer's RS at the drawOpacity == 0 early-out? > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:382 > > if (sortingStartIndex == descendants.size()) > > You'll also want to clear the RS here. Maybe it would be best to split the cleanup into a separate function that calls: > I'll deal with resetting the render surfaces for the early returns separately in https://bugs.webkit.org/show_bug.cgi?id=73270. I uploaded a different test there that exemplifies the problem. > if (layer->renderSurface() && layer != rootLayer) > renderSurfaceList.remove(layer->renderSurface()); > layer->clearRenderSurface(); > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:446 > > + renderSurfaceLayerList.removeLast(); > > You can nest this inside the previous > if (layer->renderSurface() && layer != rootLayer) {} clause . Done.
Comment on attachment 116863 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116863&action=review Vangelis or Enne, could you give this another look? > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:480 > + renderSurface1->setOpacity(0.9f); drop the trailing 'f' - see http://www.webkit.org/coding/coding-style.html "Floating point literals" > Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:483 > + setLayerPropertiesForTesting(renderSurface1.get(), identityMatrix, identityMatrix, FloatPoint(0.0f, 0.0f), FloatPoint(0.0f, 0.0f), IntSize(10, 10), false); the 0.0f isn't needed here. FloatPoint() or FloatPoint::zero() would be even better
Comment on attachment 116863 [details] Patch Looks good to me.
Comment on attachment 116863 [details] Patch Looks good
Comment on attachment 116863 [details] Patch R=me in that case
Created attachment 116996 [details] Patch
James, could you cq+ for me in case your comments are addressed? thanks! (In reply to comment #12) > Created an attachment (id=116996) [details] > Patch
Comment on attachment 116996 [details] Patch Clearing flags on attachment: 116996 Committed r101472: <http://trac.webkit.org/changeset/101472>
All reviewed patches have been landed. Closing bug.