[webkit-reviews] review requested: [Bug 84308] [chromium] Adding PrioritizedTexture and replacing ContentsTextureManager : [Attachment 145156] minor_tweak

[bugzilla-daemon at webkit.org]

Nat Duca <nduca at chromium.org> has asked  for review:
Bug 84308: [chromium] Adding PrioritizedTexture and replacing
ContentsTextureManager
https://bugs.webkit.org/show_bug.cgi?id=84308

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

------- Additional Comments from Nat Duca <nduca at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145156&action=review


Its very hard to see what the PrioritizedTextureManager calls versus what the
user of the PrioritizedTexture call. Can we make PrioritizedTexture have a
PrioritizedTextureInternal m_internal which is the internal state of the
prioritized texture that is only called by the manager? And, and and, only have
the interal one be registered to the textureManager?

Any reason we have PriorityCalculator and not CCPriorityCalculator? Same goes
for the new code not being in CC and the new classes not being called CCxxx.
The old naming predates our cc convention but I understood that we were trying
to adopt the new convention...

I see this as an important feature-level refactoring. But, as followon, I think
we need to consider making a manager for resources that is aware of both:
* A more-unified resource manager that spans both threads
* Aware of the fact that resources get moved over to the "other side" (and
can't be updated the same way once they're on the impl side)
* Aware of the difference between "I need a texture and its the same as last
time" versus "I need a texture so that I can draw into it"
* Draws a distinction between a request and an allocation. I think
PrioritizedTexture is actually a PrioritizedTextureRequest, but the way we use
it implies that it is a retained resource whereas a "request" implies something
ephemeral a la appendQuads//QuadLists.
* A request/reservation system that uses passes to make the managment process
easier to understand
* Moving more of the "how much to paint this pass" logic out of the tiled
layers and into an overall managment class (resource manager, etc)

This having been said, and lets keep refining our solution in this area.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.cpp:65
> +    if (manager)

We might want to mention that resgiterTexture calls back to us to set m_manager
= manager... that was very not clear to me.

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:64
> +    int priority() const { return m_priority; }

Out of curiosity, rationale behind int vs float?

> Source/WebCore/platform/graphics/chromium/PrioritizedTexture.h:77
> +    // Is the texture still valid since allocate was called (still has the
same size, format and data).

for clarity, move these functions to before line 52 so the comment about
invalidate makes sense to novices?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:550
> +    m_contentsTextureManager->prioritizeTextures();

Why