Bug 108051

Created attachment 184939 [details] Patch

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.

(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 on attachment 184939 [details] Patch As we talked on IRC, this is not the right solution.

(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.

(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.

(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.

Reopening to attach new patch.

Created attachment 186568 [details] Patch

(In reply to comment #9) > Created an attachment (id=186568) [details] > Patch I reopen because I found the way to remove some code.

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

Created attachment 186584 [details] Patch

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.

Created attachment 186741 [details] Patch

(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 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.

Created attachment 186742 [details] Patch

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

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

Created attachment 186776 [details] Patch

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

Comment on attachment 186776 [details] Patch Benjamin has signed this off for WK2.

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

Created attachment 187757 [details] Patch for landing

Comment on attachment 187757 [details] Patch for landing Clearing flags on attachment: 187757 Committed r142578: <http://trac.webkit.org/changeset/142578>

All reviewed patches have been landed. Closing bug.