Bug 92596 – [Chromium] Refactor CCTextureUpdater into CCTextureUpdater and CCTextureUpdateController.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[David Reveman]

CCTextureUpdater class so that more complicated texture upload logic that uses timers can be added without making LayerRendererChromium initialization too complicated or exposing inappropriate interfaces to the main thread.

[David Reveman]

Created attachment 155181 [details]
Patch

[James Robinson]

170kb is unmanagable.  Can you stage this into smaller patches somehow, or perhaps don't rename CCTextureUpdater -> CCTextureUpdaterQueue (the new name doesn't seem any better to me at first glance).

[Nat Duca]

Comment on attachment 155181 [details]
Patch

This is really nice. Thanks David.

[Nat Duca]

Could we maybe reduce the patch size, would it be possible to introducing the new classes first but not integrated into the rest of the code? Then as a followup, do the refactoring to use the new classes?

Alternatively, if we try the poroposal by james, can we eventually rename it to a queue? It really is a queue.

[David Reveman]

I agree, the patch is not very manageable at the current size. I think moving the s/CCTextureUpdater/CCTextureUpdateQueue/ change into a separate patch as jamesr suggested will make things much better. I'll work on that.

[David Reveman]

Created attachment 155341 [details]
Patch

[David Reveman]

New patch doesn't include the CCTextureUpdater -> CCTextureUpdateQueue change. The size of the patch is still pretty high but most of it is just from moving CCTextureUpdaterTest.cpp code into CCTextureUpdateControllerTest.cpp.

[James Robinson]

Comment on attachment 155341 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155341&action=review

> Source/WebCore/platform/graphics/chromium/TextureCopier.h:41
> +    struct Parameters {

"parameters" is a pretty crappy name and I'm not sure it's necessary for a function that takes just 3 arguments. What's the motivation - do you plan to expand this to take more parameters or something like that?

> Source/WebCore/platform/graphics/chromium/TextureCopier.h:49
> +        unsigned m_sourceTexture;
> +        unsigned m_destTexture;
> +        IntSize m_size;

If you keep this struct, don't use the m_ prefix for public members - just 'sourceTexture', 'destTexture', 'size'.

[David Reveman]

(In reply to comment #8)
> (From update of attachment 155341 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155341&action=review
> 
> > Source/WebCore/platform/graphics/chromium/TextureCopier.h:41
> > +    struct Parameters {
> 
> "parameters" is a pretty crappy name and I'm not sure it's necessary for a function that takes just 3 arguments. What's the motivation - do you plan to expand this to take more parameters or something like that?

I think it makes the CCTextureUpdateQueue (CCTextureUpdater in current patch) interface a bit nicer to work with:

class CCTextureUpdateQueue {
  void appendUpload(TextureUploader::Parameters);
  TextureUploader::Parameters takeFirstUpload();

  void appendCopy(TextureCopier::Parameters);
  TextureCopier::Parameters takeFirstCopy();
};

and we can write code similar to this:

while (m_updateQueue.copySize())
    m_textureCopier.copyTexture(m_updateQueue.takeFirstCopy());

> 
> > Source/WebCore/platform/graphics/chromium/TextureCopier.h:49
> > +        unsigned m_sourceTexture;
> > +        unsigned m_destTexture;
> > +        IntSize m_size;
> 
> If you keep this struct, don't use the m_ prefix for public members - just 'sourceTexture', 'destTexture', 'size'.

OK.

[David Reveman]

Created attachment 155564 [details]
Patch

Remove m_ from struct members and cleanup CCTextureUpdater.h

[Nat Duca]

Comment on attachment 155564 [details]
Patch

I like this. Thanks David.

[Adrienne Walker]

Comment on attachment 155564 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155564&action=review

Looks good in general.  If you could rebase this and address a few minor comments, I think this is good to go.

> Source/WebCore/platform/graphics/chromium/TextureCopier.h:42
> +        Parameters(unsigned _sourceTexture, unsigned _destTexture, IntSize _size)

You don't actually need the underscores here; constructor parameter names can match member names and C++ will do the right thing.  Also, since it's a POD struct, you could just initialize with {} syntax and drop this needless constructor entirely.  Same with the other Parameters struct.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:517
> +    OwnPtr<CCTextureUpdater> updater = adoptPtr(new CCTextureUpdater);

Why does this need to be heap allocated in beginFrame? Can you make it a stack variable?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:548
> +void CCThreadProxy::beginFrameCompleteOnImplThread(CCCompletionEvent* completion, PassOwnPtr<CCTextureUpdater> updater)

Please don't have a PassOwnPtr as a function arg unless there's an ownership transfer.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:147
> +    void textureUpdateOnImplThread();

?

[David Reveman]

(In reply to comment #12)
> (From update of attachment 155564 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155564&action=review
> 
> Looks good in general.  If you could rebase this and address a few minor comments, I think this is good to go.
> 
> > Source/WebCore/platform/graphics/chromium/TextureCopier.h:42
> > +        Parameters(unsigned _sourceTexture, unsigned _destTexture, IntSize _size)
> 
> You don't actually need the underscores here; constructor parameter names can match member names and C++ will do the right thing.  Also, since it's a POD struct, you could just initialize with {} syntax and drop this needless constructor entirely.  Same with the other Parameters struct.

ok, added the constructors so I wouldn't need temporary variables but I don't mind initializing using {} syntax if you prefer that. I'll fix this.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:517
> > +    OwnPtr<CCTextureUpdater> updater = adoptPtr(new CCTextureUpdater);
> 
> Why does this need to be heap allocated in beginFrame? Can you make it a stack variable?

I think it's more appropriate to pass the ownership of the updater to the impl thread rather than doing a deep copy.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:548
> > +void CCThreadProxy::beginFrameCompleteOnImplThread(CCCompletionEvent* completion, PassOwnPtr<CCTextureUpdater> updater)
> 
> Please don't have a PassOwnPtr as a function arg unless there's an ownership transfer.

there's an ownership transfer taking place here.

> 
> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:147
> > +    void textureUpdateOnImplThread();
> 
> ?

oops, that should not be there.

[David Reveman]

Created attachment 156706 [details]
Patch

Make CCTextureUpdater noncopyable, remove struct constructors and remove CCThreadProxy::textureUpdateOnImplThread

[Adrienne Walker]

Comment on attachment 156706 [details]
Patch

R=me.  Sorry about misreading the ownership semantics in the last patch.  You're totally right.

[WebKit Review Bot]

Comment on attachment 156706 [details]
Patch

Clearing flags on attachment: 156706

Committed r124784: <http://trac.webkit.org/changeset/124784>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.