Bug 108051 – Coordinated Graphics: remove the DidChangeScrollPosition message.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dongseong Hwang]

UI Process is source as well as destination of a scroll position, so Web Process
is not needed to send a scroll position via the DidChangeScrollPosition message.

[Dongseong Hwang]

Created attachment 184939 [details]
Patch

[Noam Rosenthal]

Comment on attachment 184939 [details]
Patch

I think this might cause a timing issue; setVisibleContentsRect can happen more rapidly than didChangeScrollPosition.
This message is supposed to make sure we compensate for the different speed. Either I'm missing something, or we shouldn't remove it.

[Dongseong Hwang]

(In reply to comment #2)
> (From update of attachment 184939 [details])
> I think this might cause a timing issue; setVisibleContentsRect can happen more rapidly than didChangeScrollPosition.
> This message is supposed to make sure we compensate for the different speed. Either I'm missing something, or we shouldn't remove it.

Yeah, I thought the same to you when I saw code at the first time.
But LayerTreeRenderer already has code to deal with the timing issue.

In detail,
CoordinatedLayerTreeHostProxy calls dispatchUpdate(bind(&LayerTreeRenderer::didChangeScrollPosition, m_renderer.get(), rect.location()));

void LayerTreeRenderer::didChangeScrollPosition(const FloatPoint& position)
{
    m_pendingRenderedContentsScrollPosition = position;
}

and it is applied at time to flush.

void LayerTreeRenderer::flushLayerChanges()
{
    m_renderedContentsScrollPosition = m_pendingRenderedContentsScrollPosition;
    ...
}

[Noam Rosenthal]

Comment on attachment 184939 [details]
Patch

As we talked on IRC, this is not the right solution.

[Dongseong Hwang]

(In reply to comment #4)
> (From update of attachment 184939 [details])
> As we talked on IRC, this is not the right solution.

This patch changes the behavior, so this bug is invalid.

btw, IMO we have another problem.
From your explanation, scroll of coordinated graphics has so long latency.

There are 4 IPC messages to draw scrolled contents, as I mentioned in https://bugs.webkit.org/show_bug.cgi?id=105978#c7

      UI Process                      Message               Web Process
WebPageProxy                       (wheel ->)                WebPage
PageViewportController    (<- PageDidRequestScroll)   WebPage
PageViewportController    (setVisibleContentsRect ->) CoordinatedLayerTree
PageViewportController        (<- didRenderFrame)      CoordinatedLayerTree
(thread communication)
LayerTreeRenderer::paint

We need to reduce the latency of scroll event. We will do it soon.

[Noam Rosenthal]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 184939 [details] [details])
> > As we talked on IRC, this is not the right solution.
> 
> This patch changes the behavior, so this bug is invalid.
> 
> btw, IMO we have another problem.
> From your explanation, scroll of coordinated graphics has so long latency.
> 
> There are 4 IPC messages to draw scrolled contents, as I mentioned in https://bugs.webkit.org/show_bug.cgi?id=105978#c7
> 
>       UI Process                      Message               Web Process
> WebPageProxy                       (wheel ->)                WebPage
> PageViewportController    (<- PageDidRequestScroll)   WebPage
> PageViewportController    (setVisibleContentsRect ->) CoordinatedLayerTree
> PageViewportController        (<- didRenderFrame)      CoordinatedLayerTree
> (thread communication)
> LayerTreeRenderer::paint
> 
> We need to reduce the latency of scroll event. We will do it soon.
With wheel, yes.
But with touch we handle the scroll immediately by painting the view with an offset right away.
The IPC sends messages asynchronously, but the feedback is immediate.

[Dongseong Hwang]

(In reply to comment #6)
> With wheel, yes.
> But with touch we handle the scroll immediately by painting the view with an offset right away.
> The IPC sends messages asynchronously, but the feedback is immediate.

Yeah, you are right. I missed that.
It is why I filed Bug 105497 for EFL.

[Dongseong Hwang]

Reopening to attach new patch.

[Dongseong Hwang]

Created attachment 186568 [details]
Patch

[Dongseong Hwang]

(In reply to comment #9)
> Created an attachment (id=186568) [details]
> Patch

I reopen because I found the way to remove some code.

[Noam Rosenthal]

Comment on attachment 186568 [details]
Patch

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

Looks great! I thought of doing this a while back.
Please get a WK2 owner to sign off.

> Source/WebCore/ChangeLog:9
> +        at the moment of flushing, so there is not a behavior change if a scrollPosition is sent

not a -> no

[Dongseong Hwang]

Created attachment 186584 [details]
Patch

[Benjamin Poulain]

Comment on attachment 186584 [details]
Patch

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

I have no problem with the change.
I sign off on this if the ChangeLog is changed to have a good explanation of the change.

> Source/WebKit2/ChangeLog:10
> +        Coordinated Graphics: remove the DidChangeScrollPosition message.
> +        https://bugs.webkit.org/show_bug.cgi?id=108051
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Currently, CoordinatedGraphicsScene::m_renderedContentsScrollPosition is updated
> +        at the moment of flushing, so there is no behavior change if a scrollPosition is sent
> +        via DidRenderFrame message instead of DidChangeScrollPosition message.

This is not a good ChangeLog.

You should explain why you remove DidChangeScrollPosition and how the new design solve the original problem better than DidChangeScrollPosition.
You also have all the fields bellow the message to help explain the changes.

Great changelogs explain both the "Why" and "What" of the change.

[Dongseong Hwang]

Created attachment 186741 [details]
Patch

[Dongseong Hwang]

(In reply to comment #13)
> (From update of attachment 186584 [details])
> This is not a good ChangeLog.
> 
> You should explain why you remove DidChangeScrollPosition and how the new design solve the original problem better than DidChangeScrollPosition.
> You also have all the fields bellow the message to help explain the changes.
> 
> Great changelogs explain both the "Why" and "What" of the change.

You're right. I changed changelog.

[Benjamin Poulain]

Comment on attachment 186741 [details]
Patch

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

> Source/WebKit2/ChangeLog:28
> +          Don't send a scroll position because flushPendingLayerChanges() dose

Typo: dose -> does.

[Dongseong Hwang]

Created attachment 186742 [details]
Patch

[Dongseong Hwang]

noam, could you review again?
benjaminp said LGTM :D

[Build Bot]

Comment on attachment 186742 [details]
Patch

Attachment 186742 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16378882

[Dongseong Hwang]

Created attachment 186776 [details]
Patch

[Build Bot]

Comment on attachment 186776 [details]
Patch

Attachment 186776 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16396073

[Noam Rosenthal]

Comment on attachment 186776 [details]
Patch

Benjamin has signed this off for WK2.

[Dongseong Hwang]

win bot now seems to have problem.
I'll commit later.

[Dongseong Hwang]

Created attachment 187757 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 187757 [details]
Patch for landing

Clearing flags on attachment: 187757

Committed r142578: <http://trac.webkit.org/changeset/142578>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.