[webkit-reviews] review canceled: [Bug 126022] [CoordinatedGraphics][WK2] Scale factor and scroll position is not being restored properly in a back/forward load : [Attachment 230159] Patch

[bugzilla-daemon at webkit.org]

Gyuyoung Kim <gyuyoung.kim at samsung.com> has canceled Gyuyoung Kim
<gyuyoung.kim at samsung.com>'s request for review:
Bug 126022: [CoordinatedGraphics][WK2] Scale factor and scroll position is not
being restored properly in a back/forward load
https://bugs.webkit.org/show_bug.cgi?id=126022

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

------- Additional Comments from Gyuyoung Kim <gyuyoung.kim at samsung.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=230159&action=review


>>> Source/WebCore/page/Page.cpp:718
>>>		 if (!view->delegatesScrolling())
>> 
>> Simon, if we don't call view->setScrollPosition(origin), we can't pass
previous scroll position to WK2 side.
>> 
>> Below view->setScrollPosition(origin) calls
ScrollView::setScrollPosition(scrollPoint) eventually. Then, it calls
WebChromeClient::delegatedScrollRequested() in WK2. Then, WK2 UIProcess can
handle updated scroll position and scale. However, r165652 and r165913 have
prohibited to call it. It looks WK2 ports(which use tiled backing store) need
to call "view->setScrollPosition(origin)" in order to pass stored scroll
position to WK2 implementation.
>> 
>> 
>>     void ScrollView::setScrollPosition(const IntPoint& scrollPoint) {
>>     ...
>>     #if USE(TILED_BACKING_STORE)
>>	   if (delegatesScrolling()) {
>>	       hostWindow()->delegatedScrollRequested(scrollPoint);
>>	       return;
>>	   }
>>     #endif
>>     ...
>>     }
>> 
>>     #if USE(TILED_BACKING_STORE)
>>     void WebChromeClient::delegatedScrollRequested(const IntPoint&
scrollOffset)
>>     {
>>	   m_page->pageDidRequestScroll(scrollOffset);
>>     }
>>     #endif
> 
> There is a general conflict about how the scroll position is set when using
delegateScrolling. With delegateScrolling, there are two sources that can set
the scroll position: the UI side or the engine side.
> 
> There are races between the two, there is a need for differentiating the
source, and we need a way to solve conflicts. There is such things in WebKit
today.
> 
> I do agree with Simon that changing the scroll position when changing the
scale factor on page does not seem to make much sense with delegate scrolling.
> 
> I agree there is a bug here. I disagree the #ifdef USE(TILED_BACKING_STORE)
of this patch is a solution.
> 
> I think we may need to split the paths between scrolling requested from the
WebKit layers against scrolling requested from the WebCore layer. Scrolling
from the WebKit layer is imperative. Scrolling from the WebCore layer needs to
go through conflicts resolution.
> 
> I don't have the code in front of me to find a solution, but I can have a
look next week.

Benjamin, will you upload your patch to this bug or file a new bug ? Anyway, I
agree to use #if USE(TILED_BACKING_STORE) is not good solution. Please let me
know your way to me. Thanks.