[Webkit-unassigned] [Bug 95057] [chromium] Allow impl-thread deletion of only some resources

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=95057


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #162358|review?                     |review-
               Flag|                            |




--- Comment #40 from James Robinson <jamesr at chromium.org>  2012-09-05 20:32:19 PST ---
(From update of attachment 162358)
View in context: https://bugs.webkit.org/attachment.cgi?id=162358&action=review

Overall this seems nice.  I think there are a few things that need more attention - I really want to make sure that the threading stuff is solid and robust for future changes.

The ack passing seems needlessly complicated and error prone.  It seems like what you are trying to determine in CCThreadProxy is the answer to the following question: "Have I evicted any textures from the impl thread since the last time I sent a beginFrame message to the main thread?"  If that's the case then we know when we get a beginFrameComplete on the impl thread it's out of date.  That seems like it's much easier to track directly in CCThreadProxy with a bool rather than passing an int back+forth across threads.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:425
> +

extra newline here

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:430
> +    m_contentsTextureManager->destroyEvictedBackings(purgeCounterAck);

So this is the change that requires the mutex?  This call can't actually issue any GL calls, so what is it touching?

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTexture.h:110
> +            : m_texture(id, size, format), m_resourceDeleted(false), m_owner(0) { }

initialize one thing per line. I think you should define the c'tor and d'tor in the .cpp, not the header

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:228
> +        if ((*it)->resourceDeleted()) {

so much (*it). Can we put that in a temporary with the proper type and a good name so we know what the heck it is?

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:273
> +    bool result = false;

what does "result" mean? could you give this a more descriptive name - it seems to be indicating if we've evicted anything

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:277
> +    if (!backingsToEvict.isEmpty()) {
> +        result = true;
> +        for (BackingVector::iterator it = backingsToEvict.begin(); it != backingsToEvict.end(); ++it) {

instead of the loop, why not just m_evictedBackings.append(m_unevictedBackings); m_unevictedBackings.clear(); ?

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.cpp:297
> +        destroyBacking((*it));

this call does a GL call if i understand it correctly - can you not do it inside the mutex? a fairly efficient way to do that would be to make a new vector, .swap() m_evictedBackings into the new vector under the lock, then outside the lock iterate through and actually destroy the backings

> Source/WebCore/platform/graphics/chromium/cc/CCPrioritizedTextureManager.h:167
> +
> +

spurious newlines

> Source/WebKit/chromium/tests/CCTextureUpdateControllerTest.cpp:90
> +    TextureForUploadTest() : LayerTextureUpdater::Texture(adoptPtr<CCPrioritizedTexture>(0)), m_deleted(false) { }

One initialization entry per line.  The WebKit style way to line this up would be:

TextureForUploadTest()
    : LayerTextureUpd..())
    , m_deleted(false)
{
}

You can also use 'nullptr' instead of adoptPtr<T>(0)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.