[Chromium] Changing Canvas2DLayerBridge to use SkDeferredCanvas's notification client API
Created attachment 158868 [details] Patch
Comment on attachment 158868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158868&action=review R=me. When did this roll into skia - have you checked that it shows up in the chr->skia rev in Source/WebKit/chromium/DEPS ? > Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:80 > + SkDeferredCanvas* deferred = deferredCanvas(); > + if (deferred) FYI, you could fold the assignment into the if() if you want, since "deferred" isn't used outside the scope of the if
Yep, the dependency has rolled in earlier today, as evidenced by the green cr-linux. Will re-upload with suggested change. (In reply to comment #2) > (From update of attachment 158868 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158868&action=review > > R=me. When did this roll into skia - have you checked that it shows up in the chr->skia rev in Source/WebKit/chromium/DEPS ? > > > Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:80 > > + SkDeferredCanvas* deferred = deferredCanvas(); > > + if (deferred) > > FYI, you could fold the assignment into the if() if you want, since "deferred" isn't used outside the scope of the if
Created attachment 158877 [details] Patch
Comment on attachment 158877 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158877&action=review > Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.h:46 > +class Canvas2DLayerBridge : public WebKit::WebExternalTextureLayerClient, public SkDeferredCanvas::NotificationClient { Wait a second - SkDeferredCanvas::NotificationClient derives from SkRefCnt. This class is held in an OwnPtr<> by ImageBufferData. That's a problem, isn't it? I believe this is the reason for the extra class indirection before.
(In reply to comment #5) > (From update of attachment 158877 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158877&action=review > > > Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.h:46 > > +class Canvas2DLayerBridge : public WebKit::WebExternalTextureLayerClient, public SkDeferredCanvas::NotificationClient { > > Wait a second - SkDeferredCanvas::NotificationClient derives from SkRefCnt. This class is held in an OwnPtr<> by ImageBufferData. That's a problem, isn't it? I believe this is the reason for the extra class indirection before. Ahah, you've an eagle eye! This is intentional and temporary because I have a chicken/egg problem. The layer bridge gets destroyed before the canvas and the layer bridge destructor unregisters itself as notification client, so the SkRefCnt will never kick-in to destroy the object. Therefore, this change won't crash. My plan is to remove the SkRefCnt, but I can't do that safely until this webkit change rolls into chromium.
Comment on attachment 158877 [details] Patch OK, that makes sense. Sounds good then - please don't leave it ref counted for long :)
Comment on attachment 158877 [details] Patch Clearing flags on attachment: 158877 Committed r125804: <http://trac.webkit.org/changeset/125804>
All reviewed patches have been landed. Closing bug.
Reverted r125804 for reason: Made threaded tests in performance_browser_tests start timing out on Chromium GPU bots Committed r125833: <http://trac.webkit.org/changeset/125833>
Reverted r125833 for reason: Was not the cause of the test failures. Per http://crbug.com/143311 , it is probably WebKit r125800. Committed r125841: <http://trac.webkit.org/changeset/125841>
Closing as fixed again after rollout of the rollout.