Bug 58408 – [chromium] Implement CCLayerTreeHost and CCLayerTreeHostImpl portions of threaded compositor

• Component: New Bugs
• Status: RESOLVED FIXED

[Nat Duca]

[chromium] Add CCView as threaded base class for LayerRendererChromium

[Nat Duca]

Created attachment 89324 [details]
Patch

[Nat Duca]

Here's a new draft of the CCView<->CCViewImpl communication path. Things aren't 100% complete but I think this is a good checkpoint to see if I'm heading in the right direction.

As compared to the previous incarnations of this, this patch draws heavily on how things like FileStreamProxy work. I like it lots more. :)

[WebKit Review Bot]

Attachment 89324 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8403075

[David Levin]

Comment on attachment 89324 [details]
Patch

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

I looked it over. I didn't see any huge issues. I noted a few nits (just because they stand out when I'm trying to get an idea of what the code is trying to do) and one issue that seems non-ideal (the sync call to the worker from the main thread).

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1186
> +    LayerRendererChromiumImpl(CCViewImplClient* client, LayerRendererChromium* lrc)

avoid abbreviations: lrc.

> Source/WebCore/platform/graphics/chromium/cc/CCViewImplProxy.cpp:43
> +// static

We typically don't put in this comment in WebKit land.

> Source/WebCore/platform/graphics/chromium/cc/CCViewImplProxy.cpp:139
> +    completion.wait();

Doing a sync wait on the main thread is bad. How can this be avoided?

In fact since the cc thread may wait for the main thread to complete some actions at times, can't this deadlock?

> Source/WebKit/chromium/tests/CCViewTest.cpp:135
> +#define CC_VIEW_TEST(testCaseName) \

Seems like a minimal macro with not too much value. I'd just avoid having it.

> Source/WebKit/chromium/tests/CCViewTest.cpp:141
> +///////////////////////////////////////////////////////////////////////////

This type of boundary isn't done in WebKit.

> Source/WebKit/chromium/tests/CCViewTest.cpp:149
> +        return adoptRef((GraphicsContext3D*)0);

avoid c style casts

I suspect this would work as well:
  return adoptRef<GraphicsContext3D>(0);

> Source/WebKit/chromium/tests/CCViewTest.cpp:184
> +        return adoptPtr(new MockViewImpl(client, m_test));

Typical style is to make the constructor of MockViewImpl private and only expose a static create method.

> Source/WebKit/chromium/tests/CCViewTest.cpp:226
> +            onEndTest((void*)this);

avoid c style casts. (Use static_casts, etc.)

> Source/WebKit/chromium/tests/CCViewTest.cpp:232
> +// Shortlived views shoudn't die

typo: shoud
and add "."

> Source/WebKit/chromium/tests/CCViewTest.cpp:265
> +// Shortlived views shoudn't die with a redraw in flight

typo: shoudn't
and add "."

> Source/WebKit/chromium/tests/CCViewTest.cpp:268
> +    CCViewTestShortlivedView3() {}

space inbetween {}

> Source/WebKit/chromium/tests/CCViewTest.cpp:284
> +// draw with frame 0

add "."

> Source/WebKit/chromium/tests/CCViewTest.cpp:288
> +            : m_numCommits(0)

odd indent use 4 spaces.

[Nat Duca]

(In reply to comment #4)
> Doing a sync wait on the main thread is bad. How can this be avoided?
> In fact since the cc thread may wait for the main thread to complete some actions at times, can't this deadlock?

In fact, there is a deadlock in my current implementation. Thanks!

We do have one unavoidable sync call, performCommitOnCCThread, which originates from the impl thread via beginFrameAndCommit. This synchronizes the CCViewImpl with the CCView in a locked fashion. We opted to do this over, say, a double-buffering scheme or sending individual change messages across because:
1. The state on each side CCViewImpl is pretty large, so that steered us away from a double-buffering scheme
2. There's a high ratio between amount of mutation and the actual amount of data that actually needs committing. So, pushing each mutation event across as a message would lead to far too many messages crossing between threads.

In addition to this performCommit call, we also have a temporary drawLayersOnMainThread sync call from C->M. Long term, the compositor will never block on the main thread.

In the near term, while we have the drawLayersOnMainThread present, I will add a patch to my code that inhibits drawing as long as anyone has a beginFrameAndCommit in-flight.

[Nat Duca]

Created attachment 89548 [details]
Deadlock and test fixes.

[Nat Duca]

The latest patch addresses the concerns around deadlock as well as cleans up tests.

However, a key issue remains: tasks created via createTask will hold references to ThreadSafeRefCounted objects that are passed as ARGUMENTS to createTask. However, if the actual target object of the task, e.g. createTask(target, &Target::Method) is ThreadSafeRefCounted, the Task* does not hold a reference. This causes the Shortlived unit tests to fail.

I see two options:
1. Change my threading model so that, like the FileStreamProxy, we ensure that the CCthread-side class never outlives the main-thread  class (eew)
2. Change the CCThreadTask system to hold a reference on the target instance [when possible].

David, any thoughts?

[James Robinson]

(In reply to comment #7)
> The latest patch addresses the concerns around deadlock as well as cleans up tests.
> 
> However, a key issue remains: tasks created via createTask will hold references to ThreadSafeRefCounted objects that are passed as ARGUMENTS to createTask. However, if the actual target object of the task, e.g. createTask(target, &Target::Method) is ThreadSafeRefCounted, the Task* does not hold a reference. This causes the Shortlived unit tests to fail.
> 
> I see two options:
> 1. Change my threading model so that, like the FileStreamProxy, we ensure that the CCthread-side class never outlives the main-thread  class (eew)
> 2. Change the CCThreadTask system to hold a reference on the target instance [when possible].
> 
> David, any thoughts?

Seems like 2 is the much better option.  This is how chrome's PostTask/PostDelayedTask work.  Where's the implementation of createTask and where are the argument references being held?

[David Levin]

(In reply to comment #7)

> I see two options:
> 1. Change my threading model so that, like the FileStreamProxy, we ensure that the CCthread-side class never outlives the main-thread  class (eew)

We have tended toward option 1 but that makes things complicated with managing lifetime. On the other hand...

> 2. Change the CCThreadTask system to hold a reference on the target instance [when possible].

The key problem with making things ThreadSafeRefCounted is that they may not hold any objects which are RefCounted. In practice this is pretty tricky. (Considering that many fundamental things like Strings are essentially RefCounted.)

You have to create an object that has almost nothing it in and make sure that in the future no one adds anything to it, so that is challenging.

Is your target really that simple?

If not, you can create an object which is basically a weak ptr to your real object and only use the weak pointer on CCThread and then feel free to make the proxy object ThreadSafeRefCount and ref count the target, which should be a pretty easy modification. fwiw, this is similar to the approach that I'm thinking about making generic in order to make this simpler for everyone.

[David Levin]

When you have a moment, read over http://www.webkit.org/coding/RefPtr.html to help inform your usage of RefPtr/PassRefPtr.

[Nat Duca]

Created attachment 89653 [details]
Shutdown impl before proxy dies.

[James Robinson]

Comment on attachment 89653 [details]
Shutdown impl before proxy dies.

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

Made a diagram of the overall object graph: http://www.diagrammr.com/edit?key=ddiSFcIeRj4

> Source/WebCore/platform/graphics/chromium/cc/CCView.h:50
> +// CCView (equivalent to LayerTreeHost in CA)

Will this actually host the layer tree like LayerTreeHost does?  I think it should (not now, but down the road).

> Source/WebCore/platform/graphics/chromium/cc/CCViewImpl.h:34
> +// Provides scheduling infrasturcture for a CCViewImpl

typo: infrastructure

general nit: WebKit style typically has periods at the end of comments like this.  It doesn't have to be a complete sentence but it should look kinda like one.

[Nat Duca]

Done. Also, I renamed View uses to LayerTreeHost.

I'll hold on uploading a patch assuming you've got more feedback coming.

(In reply to comment #12)
> (From update of attachment 89653 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89653&action=review
> 
> Made a diagram of the overall object graph: http://www.diagrammr.com/edit?key=ddiSFcIeRj4
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCView.h:50
> > +// CCView (equivalent to LayerTreeHost in CA)
> 
> Will this actually host the layer tree like LayerTreeHost does?  I think it should (not now, but down the road).
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCViewImpl.h:34
> > +// Provides scheduling infrasturcture for a CCViewImpl
> 
> typo: infrastructure
> 
> general nit: WebKit style typically has periods at the end of comments like this.  It doesn't have to be a complete sentence but it should look kinda like one.

[James Robinson]

Comment on attachment 89653 [details]
Shutdown impl before proxy dies.

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

Overall I think this is looking awesome.  I've made a bunch more whiteboard diagrams that I hope to electronicize to diagram out the interactions, but this is shaping up pretty nicely IMO.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:281
> +    updateLayers(*m_computedRenderSurfaceLayerList.get());

.get() unnecessary. operator* on WTF smart pointers does what you want here

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:286
> +    ASSERT(m_computedRenderSurfaceLayerList.get());

.get() unnecessary

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:312
> +    drawLayers(*m_computedRenderSurfaceLayerList.get());

.get() unnecessary

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1181
> +    CCView::commit(impl);
> +    impl->setSourceFrameNumber(m_headsUpDisplay->currentFrameNumber());

This is the only LayerRendererChromium function called on the CC thread so I think it deserves a threading ASSERT() and a comment explaining the synchronization going on (when this is called the main thread is blocked).

CCView::commit() also calls impl->setSourceFrameNumber

> Source/WebCore/platform/graphics/chromium/cc/CCView.h:47
> +class CCViewImplFactory {
> +public:
> +    virtual PassOwnPtr<CCViewImpl> createImplOnCCThread(CCViewImplClient*) = 0;

we talked about this some in person but i think the CCViewImplFactory thing is a bit heavyweight for what we really want, which is a way to construct different types of CCViewImpls on the compositor thread.

for the mocking use case, what about providing a MockCCViewImplProxy that constructs a MockCCViewImpl when it handles a createImplOnCCThread task?  CCViewImplProxy::createImplOnCCThread would have to thunk to a virtual function to actually construct the CCViewImpl, but i think that's a lot easier to reason about then having to worry about the lifetime of a factory object, etc

> Source/WebCore/platform/graphics/chromium/cc/CCView.h:60
> +    virtual void afterCommit();

there are no overrides of this function, so no need to be virtual (there are overrides of CCViewImpl::afterCommit())

> Source/WebCore/platform/graphics/chromium/cc/CCView.h:61
> +    virtual void animateAndLayout(double frameBeginTime);

there aren't any overrides of this function either (the only overrides of animateAndLayout() are of CCViewClient::animateAndLayout())

> Source/WebCore/platform/graphics/chromium/cc/CCView.h:67
> +    virtual void updateLayers() { }

rather than have an empty impl i think this should be pure virtual.  not much point in having a CCView implementation without an updateLayer()s implementatation!

> Source/WebCore/platform/graphics/chromium/cc/CCViewImplProxy.cpp:71
> +    ASSERT(!m_viewImpl.get()); // viewClosedOnCCThread() should have already been called

here and many other places call .get() when it's unnecessary.  WTF's smart pointers have operator! overrides that return the bool you expect

> Source/WebCore/platform/graphics/chromium/cc/CCViewImplProxy.cpp:138
> +    TRACE_EVENT("CCViewImplProxy::requestFrameAndCommit", this, 0);
> +    {
> +        TRACE_EVENT("CCView::animateAndLayout", this, 0);
> +        m_view->animateAndLayout(frameBeginTime);
> +    }
> +
> +    m_commitPending = false;
> +    {
> +        TRACE_EVENT("CCView::updateLayers", this, 0);
> +        m_view->updateLayers();
> +    }

This needs to be a little more defensive - both animateAndLayout() and updateLayers() can cause compositing to turn off.  in the current compositor what we do is check if compositing is still on _after_ calling animateAndLayout() and then checking if we still have a layer tree _after_ calling updateLayers().  since the CCViewImplProxy doesn't directly have access to the layer tree i think it needs to ask its m_view if it should continue with the composite operation after calling updateLayers()

> Source/WebCore/platform/graphics/chromium/cc/CCViewImplProxy.cpp:145
> +    m_view->afterCommit();

it's confusing having CCView::afterCommit() and CCViewImpl::afterCommit() which do different things. i'm not sure the main thread afterCommit() is super valuable - couldn't the CCViewImplProxy keep a commit count and expose that to the HUD through a getter rather than calling up to the view after each commit?  i'm having some trouble picturing what we plan to do on the main thread after a commit completes other than noting that we did so

> Source/WebCore/platform/graphics/chromium/cc/CCViewImplProxy.cpp:180
> +    if (m_viewImpl)

i don't think it's ever valid for m_viewImpl to be NULL here unless initialization failed in some way, right?  should this be an ASSERT()?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1076
> +        return;

we gotta fix this before we can turn the USE() on. could you file a bug and add a comment here with a big ole FIXME and a link to it?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2366
> +    if (m_layerRenderer.get())

.get() unnecessary

> Source/WebKit/chromium/src/WebViewImpl.cpp:2376
> +    if (m_layerRenderer.get())

.get() unnecessary

[James Robinson]

note that I only looked at the real impls, i haven't looked at the test code at all yet

[David Levin]

Comment on attachment 89653 [details]
Shutdown impl before proxy dies.

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

Just a few comments.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:265
>  

Why the blank line here?

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:280
> +    m_computedRenderSurfaceLayerList = adoptPtr(new LayerList());

adoptPtr(new...) outside of the a "create" method is a bad sign. (consider making the constructor private and only exposing a static create method.)

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1181
>> +    impl->setSourceFrameNumber(m_headsUpDisplay->currentFrameNumber());
> 
> This is the only LayerRendererChromium function called on the CC thread so I think it deserves a threading ASSERT() and a comment explaining the synchronization going on (when this is called the main thread is blocked).
> 
> CCView::commit() also calls impl->setSourceFrameNumber

Agreed re the assert.  Is this something that will be cleaned up in the future? Having a class used on two threads is bad news.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1231
> +        return adoptPtr(new LayerRendererChromiumImpl(client, m_layerRenderer));

Would ideally be a call to LayerRendererChromiumImpl::create here.

> Source/WebCore/platform/graphics/chromium/cc/CCView.cpp:56
> +}

add a blank line here.

> Source/WebKit/chromium/tests/CCViewTest.cpp:45
> +class MockViewClient;

Not sorted correctly.

> Source/WebKit/chromium/tests/CCViewTest.cpp:269
> +TEST_F(CCViewTestShortlivedView1, run) { runTest(); }

This is a function would ideally be formatted like one :).

> Source/WebKit/chromium/tests/CCViewTest.cpp:271
> +// Shortlived views shoudn't die with a commit in flight.

typo:shoud (and elsewhere too)

> Source/WebKit/chromium/tests/CCViewTest.cpp:306
> +// Constantly redrawing views shouldnt die when they commit

typo: shouldnt

> Source/WebKit/chromium/tests/CCViewTest.cpp:354
> +        , m_numDraws(0) { }

Consider formatting like you did for the other constructors.

> Source/WebKit/chromium/tests/CCViewTest.cpp:455
> +    }

add blank line.

> Source/WebKit/chromium/tests/CCViewTest.cpp:463
> +

too many blank lines.

[David Levin]

I must admit that this change is so large it is hard for me to get a handle on it to understand it well and verify that all of the lifetimes work out, so my comments are more surface level due to that.

I guess you and James have talked it out in person and have a better sense of it, so I guess he will give you the final r+.

[Nat Duca]

Created attachment 90113 [details]
Patch

[Nat Duca]

Created attachment 90116 [details]
Patch

[James Robinson]

Comment on attachment 90116 [details]
Patch

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

R- because I think this will make the !USE(THREADED_COMPOSITOR) code crash in ~LayerRendererChromium() and because I think some parts can be simplified.  I read the whole patch this time, though, and think it's looking really good overall.  Left lots of comments to digest.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:-191
> -    ASSERT(m_hardwareCompositing);
> -

dropping this ASSERT() from updateLayers() makes sense, but please add it back to drawLayers()

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:213
> +    ASSERT(m_computedRenderSurfaceLayerList);

should we clear this vector out at the end of drawLayers() or its caller? doesn't make much sense to leave a vector around between frames if we always throw it away on the next frame

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1096
> +class LayerRendererChromiumCommitter : public CCLayerTreeHostCommitter {

why does this exist?  i don't think the eventual implementation of CCLayerTreeHostCommitter will have anything LayerRendererChromium-specific in it, so i think we should just put the commit logic inside CCLayerTreeHostCommitter itself and not subclass it.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1124
> +class LayerRendererChromiumImpl : public CCLayerTreeHostImpl {

it's very strange to have this class in LayerRendererChromium.cpp.  this should go into its own file with its own header

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1166
> +class LayerRendererChromiumImplProxy : public CCLayerTreeHostImplProxy {

this also deserves its own file and header

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:149
> +    explicit LayerRendererChromium(CCLayerTreeHostClient*, PassRefPtr<GraphicsContext3D>, PassOwnPtr<TilePaintInterface> contentPaint);

don't need explicit when the c'tor takes 3 arguments (didn't need it with 2 either, but might as well fix it now)

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:190
> +    bool m_committing;

since this bool is just for debugging please guard the defn and uses with with #ifndef NDEBUG/#endif.  do you think it's likely that we'll accidentally make commit() reentrant?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:36
> +    , m_frameNumber(0)

why does the frame number start at 0 here and at -1 on the impl?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:53
> +#if USE(THREADED_COMPOSITING)
> +    m_proxy = createLayerTreeHostImplProxy();
> +    m_proxy->start();
> +#endif
> +}
> +
> +CCLayerTreeHost::~CCLayerTreeHost()
> +{
> +    TRACE_EVENT("CCLayerTreeHost::~CCLayerTreeHost", this, 0);
> +    m_proxy->stop();
> +    m_proxy.clear();
> +}

hmm, seems a bit inconsistent to guard the init but not the shutdown.  won't this cause a crash when creating and destroying a LayerRendererChromium without USE(THREADED_COMPOSITING) set?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:42
> +    virtual PassRefPtr<GraphicsContext3D> createLayerTreeHostContext3D() = 0;

this level of indirection doesn't seem necessary or helpful.  In the non-mock codepath WebViewImpl passes a GraphicsContext3D to LayerRendererChromium::create().  In the mock case, you can just pass 0 in to LRC::create().

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:76
> +    m_frameNumber += 1;

++m_frameNumber; ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:70
> +    bool m_redrawEnqueued;

m_redrawPending would be a more consistent and a bit easier to spell for simpletons like me :)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:27
> +#include "CCLayerTreeHostImplProxy.h"

files in the /cc/ directory in this patch are inconsistent about whether they prefix #includes with "cc/".  pick a convention and stick to it (the files i've added in cc/ include the prefix, it looks like most of the files you've added do not).  i don't feel strongly about which convention is better but we need to be consistent.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:39
> +CCThread* ccThread;

should this be an OwnPtr<>?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:49
> +    numProxies += 1;

++ ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:67
> +    ASSERT(!m_layerTreeHostImpl && !m_layerTreeHost);

feels a bit weird to poke at the m_layerTreeHostImpl ptr from the main thread. add a comment for whatever threading convention makes this query threadsafe

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:69
> +    numProxies -= 1;

-- ?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:72
> +        delete ccThread;
> +        ccThread = 0;

if ccThread was an OwnPtr<> then this would be one line

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:102
> +    ASSERT(!m_layerTreeHostImpl); // layerTreeHostClosedOnCCThread() should have already been called

feels a little dodgy to be querying this value from the main thread since it's written from the compositor thread.  are we protected here by the CCCompletionEvent's condvar?  whatever the answer is it deserves a comment

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:38
> +class CCLayerTreeHostImplProxy : public CCLayerTreeHostImplClient {
> +public:

if this isn't meant to be refcounted it should have WTF_MAKE_NONCOPYABLE() blah blah.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:44
> +    void start(); // Must be called before using the proxy.

this is normally called init(). is there any reason that the c'tor or the ::create() function for CCLayerTreeHostImplProxy can't call this for you?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:45
> +    void stop(); // Must be called before deleting the proxy.

why can't this happen in the d'tor? no virtual calls are being made in the implementation

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:73
> +    OwnPtr<CCLayerTreeHostImpl> m_layerTreeHostImpl;

can you add comments for each of these members indicating which thread the data can be accessed on? i'm pretty sure the answer is mainthread/mainthread/ccthread but it doesn't hurt to be sure. might even be worth suffixing the member names just to make it super clear what is going on

also, the class will pack better when more members are added if you sort by size (larger members like ptrs first, smaller members like bools later).

> Source/WebKit/chromium/ChangeLog:8
> +        Stress tests for CCLayerTreeHost.

there are a number of changes here other than just stress tests (the WebWidget and WebViewImpl changes). please describe those in the changelog as well

> Source/WebKit/chromium/public/WebWidget.h:88
> +    // In threaded compositing mode, indicates that the widget should redraw
> +    // itself, for example due to window damage. The redraw will begin
> +    // asynchronously.
> +    virtual void scheduleComposite()  { }
> +

how is this different from WebWidget::composite(false) ?

> Source/WebKit/chromium/src/WebViewImpl.cpp:999
>  void WebViewImpl::animate()
>  {
> +    doAnimate(currentTime());
> +}
> +
> +void WebViewImpl::doAnimate(double frameBeginTime)

oh how awkward :(.  a better solution would be to have animate() always take the timestamp parameter and have the caller be responsible for providing it. even in the software case we really should have the caller provide the timestamp - i wish i had done this way first. feel free to file a cleanup bug on me to sort that out.

in the meantime, a cleaner solution to this problem is to add a default parameter value to WebViewImpl::animate(). callers who specify a timestamp have that value used, callers who don't just get currentTime()

> Source/WebKit/chromium/src/WebViewImpl.cpp:1078
> +        ASSERT(0);

ASSERT_NOT_REACHED();

> Source/WebKit/chromium/src/WebViewImpl.cpp:1121
> +#if USE(THREADED_COMPOSITING)
> +    return; // FIXME: when the RenderWidget is updated, switch to ASSERT_NOT_REACHED.
> +#else

why does this early out? if finish==false, then this should just do m_layerRenderer->setNeedsRedraw(). if finish==true then we definitely need an ASSERT_NOT_REACHED just like we do in ::paint()

> Source/WebKit/chromium/src/WebViewImpl.cpp:2492
> +#if USE(THREADED_COMPOSITING)
> +    updateLayerRendererSettings();
> +#endif

why are we setting these bits only in the USE(THREADED_COMPOSITOR) case?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2531
>  void WebViewImpl::updateLayerRendererViewport()
>  {
> -    ASSERT(m_layerRenderer);
> +    if (!m_layerRenderer)
> +        return;

why does this change?

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:314
> +// Shortlived layerTreeHosts shoudn't die with a commit in flight.

typo: shouldn't

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:334
> +// Shortlived layerTreeHosts shoudn't die with a redraw in flight.

typo: shouldn't

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:372
> +        m_numCompleteCommits += 1;

++

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:399
> +
> +
> +

weird # of newlines

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:419
> +        if (!layerTreeHostImpl->sourceFrameNumber()) // frame 0

comment isn't really that helpful, i'd remove

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:445
> +// first commited frame draws should lead to another commit.

typo: committed

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:461
> +        if (!layerTreeHostImpl->sourceFrameNumber()) // frame 0

comment unnecessary

> Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:531
> +
> +
> +

weird # of newlines

[Nat Duca]

Comment on attachment 90116 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:213
>> +    ASSERT(m_computedRenderSurfaceLayerList);
> 
> should we clear this vector out at the end of drawLayers() or its caller? doesn't make much sense to leave a vector around between frames if we always throw it away on the next frame

There are two "frames" here -- main thread frames and compositor frames.

This list is persisted so we can have N drawLayers for a single update call. It is wrong to clear it, imo.

Hopefully this will be clearer when we get drawing moved to the impl.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1124
>> +class LayerRendererChromiumImpl : public CCLayerTreeHostImpl {
> 
> it's very strange to have this class in LayerRendererChromium.cpp.  this should go into its own file with its own header

Yes, but I suggest we leave it here. LayerRendererChromiumImpl is a temporary class while we fix threading.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1166
>> +class LayerRendererChromiumImplProxy : public CCLayerTreeHostImplProxy {
> 
> this also deserves its own file and header

Disagree. This is temporary functionality.

>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:190
>> +    bool m_committing;
> 
> since this bool is just for debugging please guard the defn and uses with with #ifndef NDEBUG/#endif.  do you think it's likely that we'll accidentally make commit() reentrant?

HHGTWKJVHEFH. You and asked me to add guards so could protect against WebKit being accessed during commit.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:36
>> +    , m_frameNumber(0)
> 
> why does the frame number start at 0 here and at -1 on the impl?

Because the impl has not got a frame yet.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:42

> 
> this level of indirection doesn't seem necessary or helpful.  In the non-mock codepath WebViewImpl passes a GraphicsContext3D to LayerRendererChromium::create().  In the mock case, you can just pass 0 in to LRC::create().

This is for lost context. We call this in LayerRendererChromium::create becausee we have to. But when we need to *Recreate* the context after its been lost, we will call this.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:39
>> +CCThread* ccThread;
> 
> should this be an OwnPtr<>?

Not sure. Given that this is global scope, I felt it was better to be explicit about cleanup than have a magical ~ that ran after main()

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:72
>> +        ccThread = 0;
> 
> if ccThread was an OwnPtr<> then this would be one line

Are you really comfortable with OwnPtrs with global scope?

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:44
>> +    void start(); // Must be called before using the proxy.
> 
> this is normally called init(). is there any reason that the c'tor or the ::create() function for CCLayerTreeHostImplProxy can't call this for you?

WRt the c'tor, we need to make virtual calls to start the proxy. Cant' do that in the proxy.

Start vs init: we have a stop. Start seems a better match than stop. I can change it if you feel strongly. Also, the FileStreamProxy/FileThread seemed to use a start/stop convention

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:45
>> +    void stop(); // Must be called before deleting the proxy.
> 
> why can't this happen in the d'tor? no virtual calls are being made in the implementation

Same comment wrt vtbl

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:73
>> +    OwnPtr<CCLayerTreeHostImpl> m_layerTreeHostImpl;
> 
> can you add comments for each of these members indicating which thread the data can be accessed on? i'm pretty sure the answer is mainthread/mainthread/ccthread but it doesn't hurt to be sure. might even be worth suffixing the member names just to make it super clear what is going on
> 
> also, the class will pack better when more members are added if you sort by size (larger members like ptrs first, smaller members like bools later).

I disagree with both points.

We shouldn't be adding more members to this class. This code concerns the two phase commit process. Nothing else will go in here. The only time we'll add stuff here is if we change the commit algorithm.

It isn't easy to name variables here. The impl and host are already obvious by the rule that "impl lives on ccthread."

Then, for things like commit, the thing is that it really means "m_commitPendingOnMainThread." But its access policy is on CCThread. And frankly, m_commitPendingOnMainThread_AccessONCCThread doesnt really do much for me.

If you look at FileStreamProxy, it follows the convention here.

You'll note we're using a convention on functions. I think this is sufficient.

>> Source/WebKit/chromium/public/WebWidget.h:88
>> +
> 
> how is this different from WebWidget::composite(false) ?

Composite is the single-threaded flow to draw the screen, immediately. It should never be called in threaded mode.

ScheduleComposite will schedule a screen redraw eventually, but no commit. This is for the case where a window receives damage.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:1121

> 
> why does this early out? if finish==false, then this should just do m_layerRenderer->setNeedsRedraw(). if finish==true then we definitely need an ASSERT_NOT_REACHED just like we do in ::paint()

Ugh. You realize this all goes away the second I can make render_widget aware of the threaded compositing mode?

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2492
>> +#endif
> 
> why are we setting these bits only in the USE(THREADED_COMPOSITOR) case?

Because in the regular case, we update the settings before every draw.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:2531
>> +        return;
> 
> why does this change?

Because now we may call it without a layer renderer.

[Nat Duca]

Also, sorry to sound tense. All great feedback, just mentally exhausted: 4 major rewrites on this so far, spanning 16 patches (+ the wtf/NonThreadSafe diversion).

[James Robinson]

(In reply to comment #22)
> Also, sorry to sound tense. All great feedback, just mentally exhausted: 4 major rewrites on this so far, spanning 16 patches (+ the wtf/NonThreadSafe diversion).

No worries.  Large patches tend to have a large chance of breaking things so I'm trying to counteract that by being extra nit-picky.  I hope that the extra attention up front will pay off with less crazy debugging sessions later on.

[James Robinson]

Comment on attachment 90116 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:213
>>> +    ASSERT(m_computedRenderSurfaceLayerList);
>> 
>> should we clear this vector out at the end of drawLayers() or its caller? doesn't make much sense to leave a vector around between frames if we always throw it away on the next frame
> 
> There are two "frames" here -- main thread frames and compositor frames.
> 
> This list is persisted so we can have N drawLayers for a single update call. It is wrong to clear it, imo.
> 
> Hopefully this will be clearer when we get drawing moved to the impl.

gotcha

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:1124
>>> +class LayerRendererChromiumImpl : public CCLayerTreeHostImpl {
>> 
>> it's very strange to have this class in LayerRendererChromium.cpp.  this should go into its own file with its own header
> 
> Yes, but I suggest we leave it here. LayerRendererChromiumImpl is a temporary class while we fix threading.

OK, but you gotta pinky swear to take it out.  we have an awful lot of 'temporary' stuff in LayerRendererChromium.cpp

>>> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:190
>>> +    bool m_committing;
>> 
>> since this bool is just for debugging please guard the defn and uses with with #ifndef NDEBUG/#endif.  do you think it's likely that we'll accidentally make commit() reentrant?
> 
> HHGTWKJVHEFH. You and asked me to add guards so could protect against WebKit being accessed during commit.

Guards are fine, but debugging guards need to be #ifdef guarded, both to avoid the runtime hit in release and to document that this isn't intended to provide functionality but just to help us with debugging.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:39
>>> +CCThread* ccThread;
>> 
>> should this be an OwnPtr<>?
> 
> Not sure. Given that this is global scope, I felt it was better to be explicit about cleanup than have a magical ~ that ran after main()

good point - better to leak this then to try to run its d'tor

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp:72
>>> +        ccThread = 0;
>> 
>> if ccThread was an OwnPtr<> then this would be one line
> 
> Are you really comfortable with OwnPtrs with global scope?

upon reflection, nope :) this way is fine

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:44
>>> +    void start(); // Must be called before using the proxy.
>> 
>> this is normally called init(). is there any reason that the c'tor or the ::create() function for CCLayerTreeHostImplProxy can't call this for you?
> 
> WRt the c'tor, we need to make virtual calls to start the proxy. Cant' do that in the proxy.
> 
> Start vs init: we have a stop. Start seems a better match than stop. I can change it if you feel strongly. Also, the FileStreamProxy/FileThread seemed to use a start/stop convention

ah, i see that initImplOnCCThread() calls a virtual.

can the createLayerTreeHostImplProxy() functions be responsible for calling start() instead of the callers? it's always awkward to be given an object that is not ready to use yet.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:45
>>> +    void stop(); // Must be called before deleting the proxy.
>> 
>> why can't this happen in the d'tor? no virtual calls are being made in the implementation
> 
> Same comment wrt vtbl

what virtual call is being made?  i see this in stop():

 95    TRACE_EVENT("CCLayerTreeHostImplProxy::stop", this, 0);
 96    ASSERT(isMainThread());
 97    CCCompletionEvent completion;
 98    ccThread->postTask(createCCThreadTask(this, &CCLayerTreeHostImplProxy::layerTreeHostClosedOnCCThread, &completion));
 99    completion.wait();
 100
 101    m_layerTreeHost = 0;
 102    ASSERT(!m_layerTreeHostImpl); // layerTreeHostClosedOnCCThread() should have already been called

and this in CCLayerTreeHostImplProxy::layerTreeHostClosedOnCCThread():

 206    TRACE_EVENT("CCLayerTreeHostImplProxy::layerTreeHostClosedOnCCThread", this, 0);
 207    ASSERT(isCCThread());
 208    m_layerTreeHostImpl.clear();
 209    completion->signal();


all looks static to me.

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:73
>>> +    OwnPtr<CCLayerTreeHostImpl> m_layerTreeHostImpl;
>> 
>> can you add comments for each of these members indicating which thread the data can be accessed on? i'm pretty sure the answer is mainthread/mainthread/ccthread but it doesn't hurt to be sure. might even be worth suffixing the member names just to make it super clear what is going on
>> 
>> also, the class will pack better when more members are added if you sort by size (larger members like ptrs first, smaller members like bools later).
> 
> I disagree with both points.
> 
> We shouldn't be adding more members to this class. This code concerns the two phase commit process. Nothing else will go in here. The only time we'll add stuff here is if we change the commit algorithm.
> 
> It isn't easy to name variables here. The impl and host are already obvious by the rule that "impl lives on ccthread."
> 
> Then, for things like commit, the thing is that it really means "m_commitPendingOnMainThread." But its access policy is on CCThread. And frankly, m_commitPendingOnMainThread_AccessONCCThread doesnt really do much for me.
> 
> If you look at FileStreamProxy, it follows the convention here.
> 
> You'll note we're using a convention on functions. I think this is sufficient.

Maybe document the conventions in comments, then?  I think this is a little less clear than it could be. For example, m_layerTreeHostImpl seems like a ccthread data structure from the "impl" in its name but it's being accessed from the main thread in multiple places in this patch.

>>> Source/WebKit/chromium/public/WebWidget.h:88
>>> +
>> 
>> how is this different from WebWidget::composite(false) ?
> 
> Composite is the single-threaded flow to draw the screen, immediately. It should never be called in threaded mode.
> 
> ScheduleComposite will schedule a screen redraw eventually, but no commit. This is for the case where a window receives damage.

composite(false) does not imply anything immediate - the fact that the GL commands are issued before the function returns is an implementation detail, not part of the API contract. composite(true) === please composite a new frame and block until the GPU has executed the rendering commands. composite(false) === please composite a new frame, but don't block.

so the difference is that composite(false) implies commit-then-draw and scheduleComposite() implies draw-but-no-commit?  that feels like something that should be an implementation detail inside WebViewImpl and not something exposed in the API since calls to composite() from the outside will always be in response to window damage since we don't route invalidations from inside WebKit out to the embedder.

>>> Source/WebKit/chromium/src/WebViewImpl.cpp:2492
>>> +#endif
>> 
>> why are we setting these bits only in the USE(THREADED_COMPOSITOR) case?
> 
> Because in the regular case, we update the settings before every draw.

Aha!  I missed that bit.  Can we just call the update...() once per LayerRendererChromium creation for both codepaths?  The settings bits shouldn't change while the process is running, iirc.

>>> Source/WebKit/chromium/src/WebViewImpl.cpp:2531
>>> +        return;
>> 
>> why does this change?
> 
> Because now we may call it without a layer renderer.

sorry to be thick but when?  the m_layerRenderer lifetime does not appear to be different in this patch than it was before.

[James Robinson]

(In reply to comment #21)
> > 
> > this level of indirection doesn't seem necessary or helpful.  In the non-mock codepath WebViewImpl passes a GraphicsContext3D to LayerRendererChromium::create().  In the mock case, you can just pass 0 in to LRC::create().
> 
> This is for lost context. We call this in LayerRendererChromium::create becausee we have to. But when we need to *Recreate* the context after its been lost, we will call this.

Unless this is changing, we never recreate a GraphicsContext3D for a lost context unless we're recreating the whole LayerRendererChromium.  After the GraphicsContext3D is set on an LRC, that's the context for the lifetime of that LayerRendererChromium.

> >> Source/WebKit/chromium/src/WebViewImpl.cpp:1121
> 
> > 
> > why does this early out? if finish==false, then this should just do m_layerRenderer->setNeedsRedraw(). if finish==true then we definitely need an ASSERT_NOT_REACHED just like we do in ::paint()
> 
> Ugh. You realize this all goes away the second I can make render_widget aware of the threaded compositing mode?

Not really - we'll still have cases where the embedder (render_widget or whoever) will want to trigger composite()s either async or synchronously.

[Nat Duca]

Created attachment 90257 [details]
Cleaned up

[WebKit Review Bot]

Attachment 90257 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8471453

[James Robinson]

Comment on attachment 90257 [details]
Cleaned up

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

> Source/WebCore/WebCore.gypi:4079
> +            'platform/graphics/chromium/cc/CCLayerTreeHost.cpp',
> +            'platform/graphics/chromium/cc/CCLayerTreeHost.h',
> +            'platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp',
> +            'platform/graphics/chromium/cc/CCLayerTreeHostImpl.h',
> +            'platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.cpp',
> +            'platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h',

looks like you're missing CCLayerTreeHostCommitter.cpp from here (and possibly other files) which is why this isn't compiling

[Nat Duca]

Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImplProxy.h:45
> >>> +    void stop(); // Must be called before deleting the proxy.
> >> 
> >> why can't this happen in the d'tor? no virtual calls are being made in the implementation
> what virtual call is being made?  i see this in stop():

I started without start and end calls. The thing that steered me toward stop, if I remember right, was that stop forces being blocking might cause additional rendering commands in flight that in turn access  CCLayerTreeHostImplProxy virtual methods, e.g. postDrawLayersTaskOnCCThread.


> >>> Source/WebKit/chromium/public/WebWidget.h:88
> >> how is this different from WebWidget::composite(false) ?

The bool flag on composite is a lingering argument that we don't use at all --- we added it when we were trying to sort out artifacts in window resizing. I'm going to remove it, since I'm dealing with WebWidget changes anyway.

I understand that we could do hide threaded compositing semantics behind a composite() call, but it the semantics of the call are pretty different:
- composite() in regular mode must be preceded by layout/animate and will issue gl calls immediately
- composite() in threaded mode will eventually call layout/animate/draw, but later

Given I'm mucking with the WebWidget already, I think we're better off making this alternative rendering flow explicit. It is going to get more complex, not simpler, once we factor in Darin's "inversion" patch.

[Nat Duca]

Created attachment 90574 [details]
Patch

[WebKit Review Bot]

Attachment 90574 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8493229

[Nat Duca]

Created attachment 90587 [details]
Make builders happy

[Nat Duca]

Created attachment 90593 [details]
Forgot to fix tests.

[Nat Duca]

Created attachment 90630 [details]
Fixes for threading compiled out.

[Nat Duca]

Created attachment 90631 [details]
Death by a thousand corner cases.

[James Robinson]

(note to people in the review hack-a-thon: I'm planning to review this today or tomorrow)

[James Robinson]

Comment on attachment 90631 [details]
Death by a thousand corner cases.

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

Close!  You'll need Darin approval for the WebView changes - probably a good idea to start bothering him in parallel with fixing up the last nits.  Also, be sure this applies to ToT - the draw part of updateAndDrawLayers() has changed a bit and you might have some conflicts there.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:187
> +    bool m_committing;

this version of m_committing is declared and initialized but never use. remove or use

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:43
> +    virtual PassRefPtr<GraphicsContext3D> createLayerTreeHostContext3D() = 0;

This is still here but not needed in the current codepath since the LRC is recreated on lost context.  I'm pretty sure you don't intend to rewrite lost context support in this patch, so please remove this for now.  We can add it if we need it later.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:71
> +#ifndef NDEBUG
> +    bool m_committing;
> +#endif

now this is declared and updated but never actually used (there aren't any ASSERT()s or branches on this variable).  use it somewhere or nuke - it's just confusing this way.

> Source/WebKit/chromium/public/WebWidget.h:67
> +    // FIXME: remove default argument once render_widget has been updated to
> +    // issue the correct value.
> +    virtual void animate(double frameBeginTime) = 0;

there's a FIXME here but no default argument - which is wrong?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2347
> +#if USE(THREADED_COMPOSITyING)

typo here.  this probably doesn't work at all then, right?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2517
> +    if (!m_layerRenderer)
> +        return;

it's still not clear when this is called with a null m_layerRenderer - as far as I can tell, the m_layerRenderer lifetime is not changed by this patch

[David Levin]

fyi as soon as https://bugs.webkit.org/attachment.cgi?id=91390&action=prettypatch lands you should probably sync it and do a build to make sure you don't need to do any fixes.

[Nat Duca]

(In reply to comment #37)
> (From update of attachment 90631 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90631&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:43
> > +    virtual PassRefPtr<GraphicsContext3D> createLayerTreeHostContext3D() = 0;
> 
> This is still here but not needed in the current codepath since the LRC is recreated on lost context.  I'm pretty sure you don't intend to rewrite lost context support in this patch, so please remove this for now.  We can add it if we need it later.

I'd like to keep this init flow in. It is a good step toward what I think it is the right way to do things, though I'm certainly open to the possibility that I'm wrong about what I think is right. :)

10,000', I think the GraphicsContext and context-lost should be sealed inside the CCLayerTreeHost. The WebView should only ever see LayerTreeHosts and provide a factory method for creating a new context, should it be needed.

If we go down the route of re-using the context after context-lost, then I think this puts in a slightly cleaner position anyway --- we just remove this method from the client, rather than mucking with the constructors.

[Nat Duca]

Created attachment 93866 [details]
Rebase and minor updates.

[Nat Duca]

Created attachment 93982 [details]
more rebasing.

[Nat Duca]

Ping.

[James Robinson]

Comment on attachment 93982 [details]
more rebasing.

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

You have some merge conflicts to ToT in WebViewImpl.cpp, but otherwise this looks good.  Please ping darin on the WebWidget API change (although it seems good to me).

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommitter.h:44
> +    explicit CCLayerTreeHostCommitter() { }

nit: the 'explicit' isn't needed for no-arg c'tor

> Source/WebKit/chromium/WebKit.gypi:59
> +            'tests/DragImageTest.cpp',

hm? this change doesn't seem intentional

> Source/WebKit/chromium/src/WebPopupMenuImpl.cpp:156
> +void WebPopupMenuImpl::animate(double frameBeginTime)

nit: to avoid unused variable warnings, omit the parameter name here

> Source/WebKit/chromium/src/WebViewImpl.cpp:2521
> +            context->reshape(std::max(1, m_size.width), std::max(1, m_size.height));

nit: webkit style is to add a 'using namespace std' declaration for the file and have the callsites just be max() (this is different from chromium style, but them's the breaks)

> Source/WebKit/chromium/src/WebViewImpl.h:419
> +

nit: spurious newline here

> Source/WebKit/chromium/tests/CCThreadTest.cpp:54
> +

nit: spurious newline here

[Nat Duca]

Created attachment 98441 [details]
Patch

[Nat Duca]

Comment on attachment 98441 [details]
Patch

All indications by bots and tests are that this is ready... ~crosses fingers~

[James Robinson]

Comment on attachment 98441 [details]
Patch

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

R=me, some nits but looks mostly the same as before.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:40
> +class CCLayerTreeHostClient {

nit: this should probably have a protected, virtual d'tor with an empty body so we can get nice compile warnings if somebody implements CCLayerTreeHostClient but forgets to declare a virtual d'tor

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:37
> +public:
> +    virtual ~CCLayerTreeHostImplClient() { }

nit: i've learned the preferred pattern for client interfaces is to make the d'tor protected

> Source/WebKit/chromium/src/WebViewImpl.cpp:1030
> -        FrameView* view = webframe->frameView();
> +       FrameView* view = webframe->frameView();

the diff makes it look like the indentation is messed up here, is it?

> Source/WebKit/chromium/src/WebViewImpl.cpp:2580
> +

spurious newline

[Nat Duca]

Committed r89694: <http://trac.webkit.org/changeset/89694>

[Nat Duca]

Reverted r89694 for reason:

Test shell still not ready for animate changes.

Committed r89700: <http://trac.webkit.org/changeset/89700>

[Nat Duca]

Committed r89837: <http://trac.webkit.org/changeset/89837>