[webkit-reviews] review denied: [Bug 69034] [chromium] Track separate scroll deltas on the compositor thread : [Attachment 109205] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Adrienne Walker
<enne at google.com>'s request for review:
Bug 69034: [chromium] Track separate scroll deltas on the compositor thread
https://bugs.webkit.org/show_bug.cgi?id=69034

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=109205&action=review


Left a pile o' comments. I think this is on a great track, just some odds and
ends to clean up.

> Source/WebCore/platform/CrossThreadCopier.h:135
> +    template<typename T> struct CrossThreadCopier<Vector<T> > {

this seems like a fine cross-thread vector copier. however it's not quite what
we really want - we are creating a vector of PODs, passing ownership of that
vector to a task, and then taking ownership of that vector from the task on the
other thread.

can we just stuff a Vector<OwnPtr<ScrollUpdateInfo> > into the task and take
out on the other side, and do whatever template voodoo we need in order to make
the verifier not yell at us? that means heap allocating the storage for the
vector but means we don't have to do any extra walks through the thing to copy
it.

i'm not super worried about performance for our particular use case - it's just
that this seems like the wrong bit of infrastructure for our use case, so i'm
hesitant to suggest it as the default way to move vectors around across
threads.

> Source/WebCore/platform/graphics/chromium/NonCompositedContentHost.cpp:69
> +    IntSize maxScroll = contentsSize - viewportSize;
> +    maxScroll.clampNegativeToZero();

could you remind me how scroll offsets work in RTL mode?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:126
> +    m_scrollDelta += scroll;
> +
> +    IntSize maxDelta = m_maxScrollPosition - toSize(m_scrollPosition);
> +    m_scrollDelta = m_scrollDelta.shrunkTo(maxDelta);

mebbe...

m_scrollDelta = (m_scrollDelta + scroll).shrunkTo(maxDelta); ? maybe go
component-wise with min()/max()? this just smells a little funky (maybe it's
because our size/point/rect libraries are awkward)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:154
> +    void scroll(const IntSize& scroll);

ScrollView.h calls this 'scrollBy' to highlight that it's relative.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.h:59
> +template<> struct CrossThreadCopierBase<false, false,
CCLayerTreeHostCommon::ScrollUpdateInfo> : public
CrossThreadCopierPassThrough<CCLayerTreeHostCommon::ScrollUpdateInfo> { };

the fact that we have to declare this and declare the vector thing in
crossthreadcopier.h seems a tad unfortunate

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:174
> +    if (rootLayer())
> +	   bakeScrollDeltasRecursive(scrollInfo, rootLayer());

since we're only doing scroll stuff on the root, can we cheat a bit here and
just collect the rootLayer offsets with a FIXME about getting the offsets for
other layers? i think it's not yet clear if we want to try to optimize that
case or not

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:198
> +    for (size_t i = 0; i < layer->children().size(); ++i)

are we ever gonna have to worry about scroll offsets on mask/reflection layers?
we always forget about those

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:88
> +    CCScrollUpdateSet bakeScrollDeltas();

i know we've been using the term 'bake' for this step when discussion the
algorithm but now that i see it as a function name it doesn't seem quite so
tasty. why would baking the scroll deltas return an UpdateSet? it feels more
like this function is calculating something to give us (and as a side effect
updating its internal state, but that's the implementation's business)

maybe calculateScrollUpdates()? i'm so boring with naming

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.h:83
> +class ScopedSetImplThread {
> +public:
> +    ScopedSetImplThread();
> +    ~ScopedSetImplThread();
> +};

Now that this is in the header by itself it seems less obvious that this is
purely for debugging and has no impact on how code actually works. Can we try
to make this more obvious somehow?  Maybe just put a DEBUG in the name, or use
some sort of macro trickery?

We also might as well stick the impl of the c'tor / d'tor in the header, so
that in release the compiler can see there's nothing except #ifndef NDEBUG
stuff and can really easily compile it all away

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:67
> +    ScopedSetImplThread impl;
> +    CCSettings settings;
> +    OwnPtr<CCLayerTreeHostImpl> hostImpl =
CCLayerTreeHostImpl::create(settings);

rather than having this repeated in every TEST you can create a fixture (an
object derived from testing::Test) that has all the state you want set up in
its c'tor and then declare these tests TEST_F(FixtureType. name) and then use
them. This makes the testcases member functions of a subclass of the fixture
class so you can access protected members