Bug 94234 – [Chromium] Changing Canvas2DLayerBridge to use SkDeferredCanvas's notification client API

• Component: New Bugs
• Status: RESOLVED FIXED

[Justin Novosad]

[Chromium] Changing Canvas2DLayerBridge to use SkDeferredCanvas's notification client API

[Justin Novosad]

Created attachment 158868 [details]
Patch

[James Robinson]

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

[Justin Novosad]

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

[Justin Novosad]

Created attachment 158877 [details]
Patch

[James Robinson]

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.

[Justin Novosad]

(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.

[James Robinson]

Comment on attachment 158877 [details]
Patch

OK, that makes sense.  Sounds good then - please don't leave it ref counted for long :)

[WebKit Review Bot]

Comment on attachment 158877 [details]
Patch

Clearing flags on attachment: 158877

Committed r125804: <http://trac.webkit.org/changeset/125804>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

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>

[Kenneth Russell]

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>

[Kenneth Russell]

Closing as fixed again after rollout of the rollout.