[chromium] scroll deltas are cleared during commit to the main thread

[Hin-Chung Lam]

Scroll deltas of CCLayerImpl are needed to properly position layers that are fixed to the container layer.

Compositor currently clears scroll deltas when committing the scroll deltas to the main thread. This leave a brief time when scroll deltas are not available and fixed layers are not positioned properly.

[Hin-Chung Lam]

Created attachment 114280 [details]
Patch

[Hin-Chung Lam]

Hi James, please take a look at this patch.

I found that CCLayerTreeHostImpl::subtractScrollDeltas() and CCLayerTreeHost::applyScrollDeltas() have very different behavior and not sensible to share code between them.

CCLayerTreeHost::applyScrollDeltas() looks like it's to notify the client to find the corresponding scrollable element to apply scroll. This code currently goes through WebViewImpl to accomplish this.

CCLayerTreeHost::subtractScrollDeltas() can just subtract scroll deltas from the current scroll.

[James Robinson]

Comment on attachment 114280 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        Reviewed by NOBODY (OOPS!).

this line should go above the long-form patch description, directly below the 1-line desc and bug URL:

[chromium] Foo the blobbizer
https://bugs.webkit.org/show_bug.cgi?id=12345

Reviewed by NOBODY (OOPS!).

> Source/WebCore/ChangeLog:20
> +          are unchanged during commit. And then after commit scroll delts are

delts -> deltas

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:230
> +    if (!m_scrollLayerImpl || !scrollInfo.size())
> +        return;
> +
> +    // FIXME: subtract scroll deltas from layer other than the root.
> +    ASSERT(scrollInfo.size() == 1 && scrollInfo[0].layerId == m_scrollLayerImpl->id());
> +    IntSize scrollDelta = scrollInfo[0].scrollDelta;

this whole part is still copy-pasted, and once we start applying scroll deltas to more than the root layer this will be significantly larger.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:414
> +    m_layerTreeHostImpl->subtractScrollDeltas(*scrollInfo.get());

just *scrollInfo, the .get() is noise

[Hin-Chung Lam]

Created attachment 114357 [details]
Patch

[Hin-Chung Lam]

Hi James, I updated the patch moving applyScrollDeltas to CCLayerTreeHostCommon which calls into the root layer, either LayerChromium or CCLayerImpl. Please have another look.

[James Robinson]

Comment on attachment 114357 [details]
Patch

Thanks, this is exactly what I had in mind. R=me, I think I want https://bugs.webkit.org/show_bug.cgi?id=71595 to land first since this will very slightly conflict so holding off on cq for now.

[Alexandre Elias]

OK, https://bugs.webkit.org/show_bug.cgi?id=71595 has landed, please pull it in and resolve conflicts.  One trivial one is that CCScrollUpdateSet was renamed to CCScrollAndScaleSet and the Vector is now a member ("scrolls").  It looks like semantically speaking, the CLs should be compatible, but please let me know if you run into any bugs after merging it in.

[James Robinson]

Comment on attachment 114357 [details]
Patch

cq- since this patch will have merge conflicts

[Hin-Chung Lam]

Created attachment 114471 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 114471 [details]
Patch for landing

Clearing flags on attachment: 114471

Committed r99844: <http://trac.webkit.org/changeset/99844>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

After more thought I don't like this approach, it's too gross and confusing.  Anything that needs to know the deltas on the impl side should just track that directly.

[Hin-Chung Lam]

Created attachment 114963 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 114963 [details]
Patch

Updated the patch to add m_sentScrollDelta to keep track of scroll delta in flight.

[Adrienne Walker]

Comment on attachment 114963 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:243
> +void CCLayerTreeHostImpl::clearSentScrollDeltas()
> +{
> +    // FIXME: clear scroll deltas other than just the main scroll layer.

I think you could just clear sentScrollDelta in LayerChromium::pushPropertiesTo, and it'd work for all layers.

[Hin-Chung Lam]

Created attachment 114980 [details]
Patch

[Hin-Chung Lam]

Created attachment 114982 [details]
Patch

[Hin-Chung Lam]

Thanks for suggestions! I've updated the patch.

[James Robinson]

Comment on attachment 114982 [details]
Patch

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

since you are storing both deltas, did you consider not baking the delta into the layer's position in processScrollDeltas()?

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:138
> +    IntSize sentScrollDelta() const { return IntSize(); }

this seems unnecessary, who uses this?  this value doesn't make any sense on LayerChromium

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:343
> +    m_scrollLayerImpl->setSentScrollDelta(m_scrollLayerImpl->sentScrollDelta() + m_scrollLayerImpl->scrollDelta());

adding in the current sent scroll delta doesn't make sense, it should always be 0 here since we only have 0 or 1 BFACs pending at any given point in time. this should be just

m_scrollLayerImpl->setSentScrollDelta(m_scrollLayerImpl->scrollDelta())

[Hin-Chung Lam]

Created attachment 115007 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 114982 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/LayerChromium.h:138
>> +    IntSize sentScrollDelta() const { return IntSize(); }
> 
> this seems unnecessary, who uses this?  this value doesn't make any sense on LayerChromium

This is going to be used in CCLayerTreeHostCommon in the template function to calculate layer transforms. I removed this to avoid confusion.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:343
>> +    m_scrollLayerImpl->setSentScrollDelta(m_scrollLayerImpl->sentScrollDelta() + m_scrollLayerImpl->scrollDelta());
> 
> adding in the current sent scroll delta doesn't make sense, it should always be 0 here since we only have 0 or 1 BFACs pending at any given point in time. this should be just
> 
> m_scrollLayerImpl->setSentScrollDelta(m_scrollLayerImpl->scrollDelta())

Fixed this.

[James Robinson]

Comment on attachment 115007 [details]
Patch

Looks good.

For the common code, I think it'd make more sense to have these details hidden in the getter on the CCLayerImpl side rather than having the common code need to be aware of every bit of information. For example, we could add a getter for 'the current position including all of your local scroll stuff' that is trivial on the LayerChromium impl and adds in all the appropriate values on the CCLayerImpl side

[Hin-Chung Lam]

(In reply to comment #22)
> (From update of attachment 115007 [details])
> Looks good.
> 
> For the common code, I think it'd make more sense to have these details hidden in the getter on the CCLayerImpl side rather than having the common code need to be aware of every bit of information. For example, we could add a getter for 'the current position including all of your local scroll stuff' that is trivial on the LayerChromium impl and adds in all the appropriate values on the CCLayerImpl side

Sorry I missed one of your comments about hiding these scrollDelta and other information from common code. The idea of having a function that covers these information sounds good to me. I'll do that in a separate patch.

[Hin-Chung Lam]

Comment on attachment 115007 [details]
Patch

Clearing flags on attachment: 115007

Committed r100258: <http://trac.webkit.org/changeset/100258>

[Hin-Chung Lam]

All reviewed patches have been landed.  Closing bug.