[webkit-reviews] review denied: [Bug 77655] Support detaching TextureManager from ManagedTexture : [Attachment 125254] proposed patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Alok Priyadarshi
<alokp at chromium.org>'s request for review:
Bug 77655: Support detaching TextureManager from ManagedTexture
https://bugs.webkit.org/show_bug.cgi?id=77655

Attachment 125254: proposed patch
https://bugs.webkit.org/attachment.cgi?id=125254&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125254&action=review


Very close.

I'd really like to see unit tests for this. Specifically, construct
TextureManager / ManagedTexture pairs and destroy them in both orders to verify
that we null out the right pointers and don't crash.

>> Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:66
>> +	m_textureManager = 0;
> 
> Move this into the body of reset() so steal() also gets it?

Yes, you definitely need to do this

> Source/WebCore/platform/graphics/chromium/ManagedTexture.h:49
> +    const TextureManager* manager() const { return m_textureManager; }

why do you need to expose a getter for this? that doesn't seem helpful

> Source/WebCore/platform/graphics/chromium/ManagedTexture.h:50
> +    void managerWillDie();

a more idiomatic name for this would be "clearManager()", it's what we use
elsewhere.

> Source/WebCore/platform/graphics/chromium/TextureManager.cpp:139
> +    // Finding in reverse should be in general faster in this case.
> +    // Textures for dynamic layers should be newer compared to those for
> +    // static layers and hence at the end of the array.

If you use a set then this goes away

> Source/WebCore/platform/graphics/chromium/TextureManager.h:112
> +    Vector<ManagedTexture*> m_registeredTextures;

this should be a HashSet<>. We don't care about order but we do care about fast
existence checks.