Bug 84122 – [Chromium] Clean up texture ids on the impl side when losing the context

• Component: New Bugs
• Status: RESOLVED FIXED

[Antoine Labour]

[Chromium] Clean up texture ids on the impl side when losing the context

[Antoine Labour]

Created attachment 137466 [details]
Patch

[James Robinson]

Comment on attachment 137466 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137466&action=review

Looks like the correct code but needs tests to prevent regressions

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

this won't fly (will fail an SVN presubmit check). Seems like it shouldn't be too hard to construct a unit test for this - just magick up a CCLayerTreeHostImpl, stick an instance of each CCLayerImpl type touched here into the tree, synthesize a lost context, then recreate with a mock context that explodes if you try to bind a non-zero texture and do a draw pass

[Antoine Labour]

Yep, sounds good - I was starting to look into how to best add a test, your suggestion sounds excellent.

Enne: could you give a look at the CCTiledLayerImpl to make sure I'm not doing anything incredibly stupid. In particular, my understanding is that we're ensured that m_tiler is != NULL when we get a lost context, but wanted to double-check on that.

[Adrienne Walker]

Comment on attachment 137466 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137466&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:240
> +    m_tiler->reset();

I think if you lose the context prior to the first commit, m_tiler would be null.  We should probably just always create a tiler to not need these sorts of checks.

[James Robinson]

The CCTiledLayerImpl wouldn't exist in the impl tree until the first commit, and after the commit we always do a pushPropertiesTo() before trying to draw it. So I think it'll always be there by the time we can notice a lost context (although perhaps this is a fragile assumption)

[Adrienne Walker]

(In reply to comment #5)
> The CCTiledLayerImpl wouldn't exist in the impl tree until the first commit, and after the commit we always do a pushPropertiesTo() before trying to draw it. So I think it'll always be there by the time we can notice a lost context (although perhaps this is a fragile assumption)

Hmm, I'll buy that argument.  It still seems a little sketchy in general, but should be ok here.

[Antoine Labour]

Created attachment 137476 [details]
Patch

[James Robinson]

Comment on attachment 137476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137476&action=review

R=me

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1157
> +// ressource that wasn't created by it (resources created by

typo ressource -> resource

[Antoine Labour]

Created attachment 137478 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 137478 [details]
Patch for landing

Attachment 137478 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12419363

[James Robinson]

Comment on attachment 137478 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=137478&action=review

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1265
> +class FakeVideoFrameProvider: public VideoFrameProvider {

Looks like you collided with http://trac.webkit.org/changeset/114335 - you want to implement WebKit::WebVideoFrameProvider

[Antoine Labour]

Created attachment 137639 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 137639 [details]
Patch for landing

Clearing flags on attachment: 137639

Committed r114473: <http://trac.webkit.org/changeset/114473>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.