[webkit-reviews] review denied: [Bug 97394] [TextureMapper] [WebKit2] Crash in WebCore::BitmapTextureGL::updateContents : [Attachment 166368] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 97394: [TextureMapper] [WebKit2] Crash in
WebCore::BitmapTextureGL::updateContents
https://bugs.webkit.org/show_bug.cgi?id=97394

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

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


Wow, kind of complicated, but probably the only way to achieve what you're
trying to do.
At least in coordinated graphics we don't really change the textureMapper from
underneath...

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:67
> +void TextureMapper::notifyDestructionObserversEarly()

How about willBeDestroyed

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:69
> +    HashSet<TextureMapperDestructionObserver*>::iterator stop =
m_destructionObservers.end();

stop -> end

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:203
> +    void observeTextureMapper(TextureMapper* textureMapper)
> +    {
> +	   if (m_observedTextureMapper == textureMapper)
> +	       return;
> +
> +	   if (m_observedTextureMapper)
> +	       m_observedTextureMapper->removeDestructionObserver(this);
> +
> +	   m_observedTextureMapper = textureMapper;
> +
> +	   if (m_observedTextureMapper)
> +	       m_observedTextureMapper->addDestructionObserver(this);
> +    }

This is a bit verbose.
I'd rather this be an interface, with the virtual function something like
willDestroyTextureMapper(TextureMapper*), and then the client can check if it's
the right one.

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:211
> +    virtual void notifyTextureMapperDestroyed() = 0;

willDestroyTextureMapper

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:532
> +    // If our TextureMapper changed the backing store will be invalid, so we
need to
> +    // create a new one and upload the layer contents again.
> +    if (m_backingStore && !m_backingStore->isValid()) {
> +	   m_backingStore.clear();
> +	   m_state.needsDisplay = true;
> +    }

Why not observe the textureMapper from TextureMapperLayer, and simply clear the
backingStore when it's destroyed?