Bug 70291 – [chromium] Allow CCLayerTreeHostImpl to call back to proxy via CCLayerTreeHostImplClient

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nat Duca]

[chromium] Allow CCLayerTreeHostImpl to call back to proxy via CCLayerTreeHostImplClient

[Nat Duca]

Created attachment 111356 [details]
Patch

[Nat Duca]

Created attachment 111360 [details]
Make LTHI the scroll controller.

[James Robinson]

Comment on attachment 111360 [details]
Make LTHI the scroll controller.

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

R- cos I think this loops forever and thus isn't tested very well, but it's looking great otherwise

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:50
> +    virtual void setNeedsRedrawOnImplThread() = 0;
> +    virtual void setNeedsCommitOnImplThread() = 0;

why are these suffixed OnImplThread() instead of OnCCThread() like the functions in CCThreadProxy are? It's confusing to be similar but slightly different from the convention established there

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:54
> +class CCLayerTreeHostImpl : public CCScrollController {

this line makes CCLayerTreeHostImpl::scrollRootLayer() virtual and an implementation of CCScrollController. Please document this in the header (make it virtual and say what interface it's implementing)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:99
> +    explicit CCLayerTreeHostImpl(const CCSettings&, CCLayerTreeHostImplClient*);

nit: no need explicit any more

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.h:68
> +    virtual void setNeedsCommitOnImplThread() { setNeedsCommitOnImplThread(); }

isn't this an infinite loop?

if these were suffixed OnCCThread() then you wouldn't need these thunks, you could just make the actual functions virtual

[James Robinson]

Maybe this'll be tested by landing https://bugs.webkit.org/show_bug.cgi?id=70161, will find out soon!

[Nat Duca]

I was trying to make the client neutral to whether it was hosted on the cc thread of the main thread... since it can be either. OnImplThread is the suffix I was going to use for cases like these --- though this is the first place where I think we've had that case...

I'll fix up the other crap. I'm surprised tests didn't catch that... do we have a BKM for running DRT with --enable-threaded-compositor? I can hack my build...

[Nat Duca]

Created attachment 111361 [details]
Fix loop and virutal

[Nat Duca]

I'm fine getting rid of the "onImplThread" suffix --- your call --- I put up fixes for the glaringly obvious stuff, will pick this up later tonight.

[James Robinson]

To run the layout tests in threaded mode pass "--threaded-compositing" to run_webkit_tests.sh / new-run-webkit-tests.

[James Robinson]

Comment on attachment 111361 [details]
Fix loop and virutal

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

R=me. As we discussed I think it's not worth trying to draw a distinction between "...OnImplThread" and "...OnCCThread" suffixes - I think life would be a lot better if we used one and stuck to it.

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:71
> +

nit: extra newline

[Nat Duca]

Totally agreed. I'll eradicate OnCCThread variants.

[Nat Duca]

Committed r98025: <http://trac.webkit.org/changeset/98025>