Bug 108051 - Coordinated Graphics: remove the DidChangeScrollPosition message.
Summary: Coordinated Graphics: remove the DidChangeScrollPosition message.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dongseong Hwang
URL:
Keywords:
Depends on:
Blocks: 103854 108922
  Show dependency treegraph
 
Reported: 2013-01-27 22:06 PST by Dongseong Hwang
Modified: 2013-02-11 21:31 PST (History)
5 users (show)

See Also:


Attachments
Patch (7.14 KB, patch)
2013-01-27 22:08 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (13.32 KB, patch)
2013-02-05 00:51 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (13.32 KB, patch)
2013-02-05 02:39 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (14.22 KB, patch)
2013-02-05 18:33 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (14.22 KB, patch)
2013-02-05 18:42 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff
Patch (14.22 KB, patch)
2013-02-06 00:21 PST, Dongseong Hwang
noam: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (14.27 KB, patch)
2013-02-11 19:19 PST, Dongseong Hwang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dongseong Hwang 2013-01-27 22:06:20 PST
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.
Comment 1 Dongseong Hwang 2013-01-27 22:08:34 PST
Created attachment 184939 [details]
Patch
Comment 2 Noam Rosenthal 2013-01-29 06:45:41 PST
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.
Comment 3 Dongseong Hwang 2013-01-29 14:30:43 PST
(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;
    ...
}
Comment 4 Noam Rosenthal 2013-01-30 03:02:34 PST
Comment on attachment 184939 [details]
Patch

As we talked on IRC, this is not the right solution.
Comment 5 Dongseong Hwang 2013-01-31 22:31:43 PST
(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.
Comment 6 Noam Rosenthal 2013-01-31 22:40:26 PST
(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.
Comment 7 Dongseong Hwang 2013-01-31 23:56:05 PST
(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.
Comment 8 Dongseong Hwang 2013-02-05 00:51:29 PST
Reopening to attach new patch.
Comment 9 Dongseong Hwang 2013-02-05 00:51:33 PST
Created attachment 186568 [details]
Patch
Comment 10 Dongseong Hwang 2013-02-05 01:29:20 PST
(In reply to comment #9)
> Created an attachment (id=186568) [details]
> Patch

I reopen because I found the way to remove some code.
Comment 11 Noam Rosenthal 2013-02-05 02:19:05 PST
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
Comment 12 Dongseong Hwang 2013-02-05 02:39:54 PST
Created attachment 186584 [details]
Patch
Comment 13 Benjamin Poulain 2013-02-05 13:08:59 PST
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.
Comment 14 Dongseong Hwang 2013-02-05 18:33:00 PST
Created attachment 186741 [details]
Patch
Comment 15 Dongseong Hwang 2013-02-05 18:33:56 PST
(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.
Comment 16 Benjamin Poulain 2013-02-05 18:39:55 PST
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.
Comment 17 Dongseong Hwang 2013-02-05 18:42:08 PST
Created attachment 186742 [details]
Patch
Comment 18 Dongseong Hwang 2013-02-05 18:43:37 PST
noam, could you review again?
benjaminp said LGTM :D
Comment 19 Build Bot 2013-02-05 19:51:33 PST
Comment on attachment 186742 [details]
Patch

Attachment 186742 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16378882
Comment 20 Dongseong Hwang 2013-02-06 00:21:03 PST
Created attachment 186776 [details]
Patch
Comment 21 Build Bot 2013-02-06 01:35:33 PST
Comment on attachment 186776 [details]
Patch

Attachment 186776 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16396073
Comment 22 Noam Rosenthal 2013-02-06 01:38:07 PST
Comment on attachment 186776 [details]
Patch

Benjamin has signed this off for WK2.
Comment 23 Dongseong Hwang 2013-02-06 02:28:26 PST
win bot now seems to have problem.
I'll commit later.
Comment 24 Dongseong Hwang 2013-02-11 19:19:33 PST
Created attachment 187757 [details]
Patch for landing
Comment 25 WebKit Review Bot 2013-02-11 21:31:18 PST
Comment on attachment 187757 [details]
Patch for landing

Clearing flags on attachment: 187757

Committed r142578: <http://trac.webkit.org/changeset/142578>
Comment 26 WebKit Review Bot 2013-02-11 21:31:23 PST
All reviewed patches have been landed.  Closing bug.