[webkit-reviews] review denied: [Bug 95057] [chromium] Allow impl-thread deletion of only some resources : [Attachment 162358] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Christopher Cameron
<ccameron at chromium.org>'s request for review:
Bug 95057: [chromium] Allow impl-thread deletion of only some resources
https://bugs.webkit.org/show_bug.cgi?id=95057

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
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:22
8
> +	   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:27
3
> +    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:27
7
> +    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:29
7
> +	   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)