WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
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
Summary
[CoordinatedGraphics][WK2] Scale factor and scroll position is not being rest...
Thiago de Barros Lacerda
Reported
2013-12-19 15:44:55 PST
When user is navigating back/forward to a page that has been scrolled and/or scaled, that page must be shown with its last scroll position and scale factor. Also, when a user was using the WKView API to set the scale factor (WKViewSetContentScaleFactor) this scale was not being forwarded to WebCore. Then, when going back/forward to a page HistoryController was not knowing about its real scale factor. A WebKitAPI test is being added to validate this behaviour.
Attachments
Patch
(19.47 KB, patch)
2013-12-19 16:02 PST
,
Thiago de Barros Lacerda
no flags
Details
Formatted Diff
Diff
Patch
(20.67 KB, patch)
2013-12-20 15:03 PST
,
Thiago de Barros Lacerda
no flags
Details
Formatted Diff
Diff
Work-in-progress
(17.00 KB, patch)
2014-03-07 00:20 PST
,
Gyuyoung Kim
no flags
Details
Formatted Diff
Diff
Patch
(3.59 KB, patch)
2014-03-15 09:08 PDT
,
Gyuyoung Kim
no flags
Details
Formatted Diff
Diff
Patch
(15.38 KB, patch)
2014-03-15 18:15 PDT
,
Gyuyoung Kim
no flags
Details
Formatted Diff
Diff
Patch
(16.65 KB, patch)
2014-03-19 08:14 PDT
,
Gyuyoung Kim
no flags
Details
Formatted Diff
Diff
Patch
(16.99 KB, patch)
2014-03-20 00:49 PDT
,
Gyuyoung Kim
no flags
Details
Formatted Diff
Diff
Patch
(12.53 KB, patch)
2014-04-25 03:19 PDT
,
Gyuyoung Kim
no flags
Details
Formatted Diff
Diff
Patch
(13.42 KB, patch)
2014-04-30 07:04 PDT
,
Gyuyoung Kim
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Thiago de Barros Lacerda
Comment 1
2013-12-19 16:02:36 PST
Created
attachment 219697
[details]
Patch
Thiago de Barros Lacerda
Comment 2
2013-12-19 16:06:32 PST
***
Bug 119063
has been marked as a duplicate of this bug. ***
Noam Rosenthal
Comment 3
2013-12-20 05:47:06 PST
Comment on
attachment 219697
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=219697&action=review
> Source/WebCore/ChangeLog:17 > + * loader/HistoryController.cpp: > + (WebCore::HistoryController::restoreScrollPositionAndViewState):
Explain what you're actually doing to rectify this.
> Source/WebKit2/ChangeLog:13 > + When user is navigating back/forward to a page that has been scrolled and/or scaled, that page must be shown > + with its last scroll position and scale factor. > + Also, when a user was using the WKView API to set the scale factor (WKViewSetContentScaleFactor) this scale was > + not being forwarded to WebCore. Then, when going back/forward to a page HistoryController was not knowing about > + its real scale factor. > + A WebKitAPI test is being added to validate this behaviour.
This is a description of the bug. Also describe the solution otherwise the code is hard to follow.
> Source/WebCore/loader/HistoryController.cpp:159 > + double currentScaleFactor = m_currentItem->pageScaleFactor(); > + IntPoint currentScrollPoint = m_currentItem->scrollPoint(); > +#if PLATFORM(EFL) || PLATFORM(NIX) > + double previousScaleFactor = m_previousItem ? m_previousItem->pageScaleFactor() : currentScaleFactor; > + IntPoint previousScrollPoint = m_previousItem ? m_previousItem->scrollPoint() : currentScrollPoint; > + > + if (previousScaleFactor != currentScaleFactor || previousScrollPoint != currentScrollPoint) { > +#else > if (!view->wasScrolledByUser()) { > - if (page && m_frame.isMainFrame() && m_currentItem->pageScaleFactor()) > - page->setPageScaleFactor(m_currentItem->pageScaleFactor(), m_currentItem->scrollPoint()); > +#endif // PLATFORM(EFL) || PLATFORM(NIX) > + if (page && m_frame.isMainFrame() && currentScaleFactor) > + page->setPageScaleFactor(currentScaleFactor, currentScrollPoint); > else > - view->setScrollPosition(m_currentItem->scrollPoint()); > + view->setScrollPosition(currentScrollPoint); > } > }
This snippet is a bit spaghettified... I would rather try to figure out how ports where this already works (Mac?) do this, and try to consolidate rather than have port #ifdefs in WebCore.
> Source/WebKit2/UIProcess/PageClient.h:127 > +#if PLATFORM(EFL) || PLATFORM(NIX) > + virtual void didChangePageScaleFactor(double scaleFactor) = 0; > +#endif
I think this can be an empty implementation rather than some #ifdefed thing.
> Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1147 > +#if PLATFORM(EFL) || PLATFORM(NIX) > + if (m_frame == m_frame->page()->mainWebFrame()) > + m_frame->page()->send(Messages::WebPageProxy::PageDidRequestScroll(m_frame->coreFrame()->loader().history().currentItem()->scrollPoint())); > +#endif
How is this done in Mac? If it's not, maybe it's better to make this into a Settings flag rather than an #ifdef block.
Thiago de Barros Lacerda
Comment 4
2013-12-20 14:38:12 PST
(In reply to
comment #3
)
> (From update of
attachment 219697
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=219697&action=review
> > > Source/WebCore/ChangeLog:17 > > + * loader/HistoryController.cpp: > > + (WebCore::HistoryController::restoreScrollPositionAndViewState): > > Explain what you're actually doing to rectify this.
OK
> > > Source/WebKit2/ChangeLog:13 > > + When user is navigating back/forward to a page that has been scrolled and/or scaled, that page must be shown > > + with its last scroll position and scale factor. > > + Also, when a user was using the WKView API to set the scale factor (WKViewSetContentScaleFactor) this scale was > > + not being forwarded to WebCore. Then, when going back/forward to a page HistoryController was not knowing about > > + its real scale factor. > > + A WebKitAPI test is being added to validate this behaviour. > > This is a description of the bug. Also describe the solution otherwise the code is hard to follow.
OK
> > > Source/WebCore/loader/HistoryController.cpp:159 > > + double currentScaleFactor = m_currentItem->pageScaleFactor(); > > + IntPoint currentScrollPoint = m_currentItem->scrollPoint(); > > +#if PLATFORM(EFL) || PLATFORM(NIX) > > + double previousScaleFactor = m_previousItem ? m_previousItem->pageScaleFactor() : currentScaleFactor; > > + IntPoint previousScrollPoint = m_previousItem ? m_previousItem->scrollPoint() : currentScrollPoint; > > + > > + if (previousScaleFactor != currentScaleFactor || previousScrollPoint != currentScrollPoint) { > > +#else > > if (!view->wasScrolledByUser()) { > > - if (page && m_frame.isMainFrame() && m_currentItem->pageScaleFactor()) > > - page->setPageScaleFactor(m_currentItem->pageScaleFactor(), m_currentItem->scrollPoint()); > > +#endif // PLATFORM(EFL) || PLATFORM(NIX) > > + if (page && m_frame.isMainFrame() && currentScaleFactor) > > + page->setPageScaleFactor(currentScaleFactor, currentScrollPoint); > > else > > - view->setScrollPosition(m_currentItem->scrollPoint()); > > + view->setScrollPosition(currentScrollPoint); > > } > > } > > This snippet is a bit spaghettified... I would rather try to figure out how ports where this already works (Mac?) do this, and try to consolidate rather than have port #ifdefs in WebCore.
I could only reproduce this on ports using CoordinatedGraphics, with TILED_BACKING_STORE and fixed layout enabled. Then a very specific code path occurs, where FrameView::setWasScrolledByUser is set between Frame::reset and HistoryController::restoreViewStateAndScrollPosition, which makes the zoom and the scrollPosition to do not be restored (since they will not enter in the condition if (!view->wasScrolledByUser()). I will remove the #if code to a separated method.
> > > Source/WebKit2/UIProcess/PageClient.h:127 > > +#if PLATFORM(EFL) || PLATFORM(NIX) > > + virtual void didChangePageScaleFactor(double scaleFactor) = 0; > > +#endif > > I think this can be an empty implementation rather than some #ifdefed thing.
OK
> > > Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:1147 > > +#if PLATFORM(EFL) || PLATFORM(NIX) > > + if (m_frame == m_frame->page()->mainWebFrame()) > > + m_frame->page()->send(Messages::WebPageProxy::PageDidRequestScroll(m_frame->coreFrame()->loader().history().currentItem()->scrollPoint())); > > +#endif > > How is this done in Mac? > If it's not, maybe it's better to make this into a Settings flag rather than an #ifdef block.
I figured out that this will not be needed
Thiago de Barros Lacerda
Comment 5
2013-12-20 15:03:53 PST
Created
attachment 219798
[details]
Patch
Benjamin Poulain
Comment 6
2013-12-20 15:31:40 PST
Comment on
attachment 219798
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=219798&action=review
r- due to platform #ifdefs in HistoryController.
> Source/WebKit2/ChangeLog:3 > + scalebackhistory
?
> Source/WebCore/loader/HistoryController.cpp:162 > +#if PLATFORM(EFL) || PLATFORM(NIX) > + double previousScaleFactor = m_previousItem ? m_previousItem->pageScaleFactor() : m_currentItem->pageScaleFactor(); > + IntPoint previousScrollPoint = m_previousItem ? m_previousItem->scrollPoint() : m_currentItem->scrollPoint(); > + return previousScaleFactor != m_currentItem->pageScaleFactor() || previousScrollPoint != m_currentItem->scrollPoint(); > +#else > + return !view->wasScrolledByUser(); > +#endif
This #ifdef is not okay. Why can't the code be made common exactly? The changelog has a little explanation but not enough to determine what is correct.
> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:88 > + if (isSuspended()) > + return;
This is odd. An explanation in the changelog would be welcome.
> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:106 > + m_contentScaleFactor = scaleFactor;
This is a bad idea. The view presents the information of WebPageProxy. The information about contentScaleFactor already exists on WebPageProxy, I don't think you should duplicate it.
Thiago de Barros Lacerda
Comment 7
2013-12-22 14:42:31 PST
(In reply to
comment #6
)
> (From update of
attachment 219798
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=219798&action=review
> > r- due to platform #ifdefs in HistoryController. > > > Source/WebKit2/ChangeLog:3 > > + scalebackhistory > > ? > > > Source/WebCore/loader/HistoryController.cpp:162 > > +#if PLATFORM(EFL) || PLATFORM(NIX) > > + double previousScaleFactor = m_previousItem ? m_previousItem->pageScaleFactor() : m_currentItem->pageScaleFactor(); > > + IntPoint previousScrollPoint = m_previousItem ? m_previousItem->scrollPoint() : m_currentItem->scrollPoint(); > > + return previousScaleFactor != m_currentItem->pageScaleFactor() || previousScrollPoint != m_currentItem->scrollPoint(); > > +#else > > + return !view->wasScrolledByUser(); > > +#endif > > This #ifdef is not okay. Why can't the code be made common exactly? > > The changelog has a little explanation but not enough to determine what is correct. > > > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:88 > > + if (isSuspended()) > > + return; > > This is odd. An explanation in the changelog would be welcome. > > > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:106 > > + m_contentScaleFactor = scaleFactor; > > This is a bad idea. The view presents the information of WebPageProxy. The information about contentScaleFactor already exists on WebPageProxy, I don't think you should duplicate it.
I completely agree with you. This is legacy code, so I just kept it as is. But having two "scale factors" (one in the WebView and another in the WebPageProxy) really annoys me. I would be more than happy to make a patch to unify those (WebView always getting this info from WebPageProxy). What do you think?
Gyuyoung Kim
Comment 8
2014-03-04 20:06:50 PST
Thiago de Barros Lacerda, any update ? EFL port has same problem when using fixed layout. So, if you can't follow up this patch, can I finish this patch ?
Thiago de Barros Lacerda
Comment 9
2014-03-05 18:30:42 PST
(In reply to
comment #8
)
> Thiago de Barros Lacerda, any update ? > > EFL port has same problem when using fixed layout. So, if you can't follow up this patch, can I finish this patch ?
Hi Gyuyoung, I have forgotten this bug. I will update the patch and let you know. Thanks
Gyuyoung Kim
Comment 10
2014-03-05 19:01:49 PST
(In reply to
comment #9
)
> (In reply to
comment #8
) > > Thiago de Barros Lacerda, any update ? > > > > EFL port has same problem when using fixed layout. So, if you can't follow up this patch, can I finish this patch ? > > Hi Gyuyoung, I have forgotten this bug. I will update the patch and let you know. > Thanks
Nice, thank you.
Gyuyoung Kim
Comment 11
2014-03-07 00:20:32 PST
Created
attachment 226091
[details]
Work-in-progress
Gyuyoung Kim
Comment 12
2014-03-07 00:22:36 PST
(In reply to
comment #11
)
> Created an attachment (id=226091) [details] > Work-in-progress
Thiago said he doesn't have time to finish this bug. So, let me finish this patch. Thanks. Next patch is going to be uploaded.
Gyuyoung Kim
Comment 13
2014-03-15 09:08:18 PDT
Created
attachment 226822
[details]
Patch
Early Warning System Bot
Comment 14
2014-03-15 09:10:40 PDT
Attachment 226822
[details]
did not pass style-queue: Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
Csaba Osztrogonác
Comment 15
2014-03-15 10:39:51 PDT
Comment on
attachment 226822
[details]
Patch it's not a patch
Gyuyoung Kim
Comment 16
2014-03-15 18:15:18 PDT
Created
attachment 226827
[details]
Patch
Gyuyoung Kim
Comment 17
2014-03-15 19:20:35 PDT
(In reply to
comment #15
)
> (From update of
attachment 226822
[details]
) > it's not a patch
Oops, something was weird. I upload patch again.
Gyuyoung Kim
Comment 18
2014-03-17 01:32:35 PDT
Benjamin, Noam, could you take a look latest patch ?
Thiago de Barros Lacerda
Comment 19
2014-03-17 07:19:06 PDT
Comment on
attachment 226827
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=226827&action=review
The problem on EFL port seems to be on the PageViewportController, it is setting the scale to 1 every time the page is loaded in didRenderFrame method
> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:104 > +
I think we don't need this callback, due to
http://trac.webkit.org/changeset/162382
. updateViewportSize() call can be moved to WebView::setContentScaleFactor.
Gyuyoung Kim
Comment 20
2014-03-17 09:24:06 PDT
Comment on
attachment 226827
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=226827&action=review
> The problem on EFL port seems to be on the PageViewportController, it is setting the scale to 1 every time the page is loaded in didRenderFrame method
This problem is not fixed by this patch yet. Let me check it soon.
>> Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:104 >> + > > I think we don't need this callback, due to
http://trac.webkit.org/changeset/162382
. > updateViewportSize() call can be moved to WebView::setContentScaleFactor.
> I think we don't need this callback, due to
http://trac.webkit.org/changeset/162382
.
Looks like that.
r162382
already updates m_pageScaleFactor with new scale factor. I will remove it.
Gyuyoung Kim
Comment 21
2014-03-19 08:14:26 PDT
Created
attachment 227183
[details]
Patch
Gyuyoung Kim
Comment 22
2014-03-19 08:35:07 PDT
(In reply to
comment #19
)
> (From update of
attachment 226827
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=226827&action=review
> > The problem on EFL port seems to be on the PageViewportController, it is setting the scale to 1 every time the page is loaded in didRenderFrame method
It looks it is a problem that didRenderFrame() resets existing pageScaleFactor which is not updated. It is really problem when we backward to previous page. The didRenderFrame() overwrites a pageScaleFactor of current scale factor though HistoryController::restoreScrollPositionAndViewState() restores the scale factor of previous page. So, the reset function calls are removed in latest patch. Please let me know if we need to keep those calls.
> > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:104 > > + > > I think we don't need this callback, due to
http://trac.webkit.org/changeset/162382
. > updateViewportSize() call can be moved to WebView::setContentScaleFactor.
didChangePageScaleFactor() is needed to set an updated pageScaleFactor if we remove the function calls to set an outdated pageScaleFactor.
Thiago de Barros Lacerda
Comment 23
2014-03-19 10:52:00 PDT
(In reply to
comment #22
)
> (In reply to
comment #19
) > > (From update of
attachment 226827
[details]
[details]) > > View in context:
https://bugs.webkit.org/attachment.cgi?id=226827&action=review
> > > > The problem on EFL port seems to be on the PageViewportController, it is setting the scale to 1 every time the page is loaded in didRenderFrame method > > It looks it is a problem that didRenderFrame() resets existing pageScaleFactor which is not updated. It is really problem when we backward to previous page. The didRenderFrame() overwrites a pageScaleFactor of current scale factor though HistoryController::restoreScrollPositionAndViewState() restores the scale factor of previous page. So, the reset function calls are removed in latest patch. Please let me know if we need to keep those calls.
I think there is no problem, only EFL uses PageViewportController :)
> > > > > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:104 > > > + > > > > I think we don't need this callback, due to
http://trac.webkit.org/changeset/162382
. > > updateViewportSize() call can be moved to WebView::setContentScaleFactor. > > didChangePageScaleFactor() is needed to set an updated pageScaleFactor if we remove the function calls to set an outdated pageScaleFactor.
There is no need for that, since there is no more duplicated scaleFactor in both WebPageProxy and WebView, like in the past. WebView always get the updated scaleFactor because it retrieves directly from WebPageProxy (see WebView::contentScaleFactor). So, didChangeScaleFactor does nothing
Gyuyoung Kim
Comment 24
2014-03-20 00:49:19 PDT
Created
attachment 227269
[details]
Patch
Gyuyoung Kim
Comment 25
2014-03-20 00:54:18 PDT
(In reply to
comment #23
)
> > > > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:104 > > > > + > > > > > > I think we don't need this callback, due to
http://trac.webkit.org/changeset/162382
. > > > updateViewportSize() call can be moved to WebView::setContentScaleFactor. > > > > didChangePageScaleFactor() is needed to set an updated pageScaleFactor if we remove the function calls to set an outdated pageScaleFactor. > > There is no need for that, since there is no more duplicated scaleFactor in both WebPageProxy and WebView, like in the past. WebView always get the updated scaleFactor because it retrieves directly from WebPageProxy (see WebView::contentScaleFactor). So, didChangeScaleFactor does nothing
I see. I verified we don't need to add didChangeScaleFactor(). However, there is another problem. scroll position is not restored in backward load. According to my investigation, Page::setPageScaleFactor() has not updated scroll position when delegatesScrolling() is enabled. So, WK2 can't know previous scroll position in backward page load. Page::setPageScaleFactor(float scale, const IntPoint& origin) { if (!view->delegatesScrolling()) view->setScrollPosition(origin); } I would call "view->setScrollPosition(origin)" regardless of delegatesScrolling() in order to let WK2 know new scroll position. When I do that, is there any issue ?
Thiago de Barros Lacerda
Comment 26
2014-03-20 14:41:29 PDT
(In reply to
comment #25
)
> (In reply to
comment #23
) > > > > > > Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp:104 > > > > > + > > > > > > > > I think we don't need this callback, due to
http://trac.webkit.org/changeset/162382
. > > > > updateViewportSize() call can be moved to WebView::setContentScaleFactor. > > > > > > didChangePageScaleFactor() is needed to set an updated pageScaleFactor if we remove the function calls to set an outdated pageScaleFactor. > > > > There is no need for that, since there is no more duplicated scaleFactor in both WebPageProxy and WebView, like in the past. WebView always get the updated scaleFactor because it retrieves directly from WebPageProxy (see WebView::contentScaleFactor). So, didChangeScaleFactor does nothing > > I see. I verified we don't need to add didChangeScaleFactor(). However, there is another problem. scroll position is not restored in backward load. According to my investigation, Page::setPageScaleFactor() has not updated scroll position when delegatesScrolling() is enabled. So, WK2 can't know previous scroll position in backward page load. > > Page::setPageScaleFactor(float scale, const IntPoint& origin) { > if (!view->delegatesScrolling()) > view->setScrollPosition(origin); > } > > I would call "view->setScrollPosition(origin)" regardless of delegatesScrolling() in order to let WK2 know new scroll position. When I do that, is there any issue ?
Huum... I don't know, better ask benjaminp, he did this patch. I remember that I faced that problem in Nix as well, what I did that fixed the problem was to change this part of code: #if USE(TILED_BACKING_STORE) if (m_frame->coreFrame()->view()) fixedVisibleContentRect = m_frame->coreFrame()->view()->fixedVisibleContentRect(); #endif to something like this: #if USE(TILED_BACKING_STORE) #if PLATFORM(NIX) fixedVisibleContentRect = IntRect(); #else if (m_frame->coreFrame()->view()) fixedVisibleContentRect = m_frame->coreFrame()->view()->fixedVisibleContentRect(); #endif #endif Thus, view->wasScrolledByUser (in HistoryController) was never set and the scroll position is restored properly. See if that works.
Gyuyoung Kim
Comment 27
2014-03-20 22:07:30 PDT
(In reply to
comment #26
)
> > Thus, view->wasScrolledByUser (in HistoryController) was never set and the scroll position is restored properly. > See if that works.
Currently, m_wasScrolledByUser() is set as false by FrameView::reset() before calling HistoryController::restoreScrollPositionAndViewState(). if (!view->wasScrolledByUser()) { if (page && m_frame.isMainFrame() && m_currentItem->pageScaleFactor()) page->setPageScaleFactor(m_currentItem->pageScaleFactor(), m_currentItem->scrollPoint()); else view->setScrollPosition(m_currentItem->scrollPoint()); Thus, though page->setPageScaleFactor(m_currentItem->pageScaleFactor(), m_currentItem->scrollPoint()) is called, Page::setPageScaleFactor() can't pass the updated scrollPoint() value to WK2 side. That's why I wanna remove "if (!view->delegatesScrolling())" condition in the Page::setPageScaleFactor().
Gyuyoung Kim
Comment 28
2014-03-22 17:33:28 PDT
(In reply to
comment #27
)
> (In reply to
comment #26
) > > > > Thus, view->wasScrolledByUser (in HistoryController) was never set and the scroll position is restored properly. > > See if that works. > > Currently, m_wasScrolledByUser() is set as false by FrameView::reset() before calling HistoryController::restoreScrollPositionAndViewState(). > > if (!view->wasScrolledByUser()) { > if (page && m_frame.isMainFrame() && m_currentItem->pageScaleFactor()) > page->setPageScaleFactor(m_currentItem->pageScaleFactor(), m_currentItem->scrollPoint()); > else > view->setScrollPosition(m_currentItem->scrollPoint()); > > Thus, though page->setPageScaleFactor(m_currentItem->pageScaleFactor(), m_currentItem->scrollPoint()) is called, Page::setPageScaleFactor() can't pass the updated scrollPoint() value to WK2 side. That's why I wanna remove "if (!view->delegatesScrolling())" condition in the Page::setPageScaleFactor().
Benjamin, what do you think about above comments ?
Gyuyoung Kim
Comment 29
2014-03-31 01:08:27 PDT
Comment on
attachment 227269
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=227269&action=review
> Source/WebCore/page/Page.cpp:-702 > - if (!view->delegatesScrolling())
Tim, could you check if this change may influence on mac and iOS ports ?
Gyuyoung Kim
Comment 30
2014-04-08 22:46:29 PDT
(In reply to
comment #29
)
> (From update of
attachment 227269
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=227269&action=review
> > > Source/WebCore/page/Page.cpp:-702 > > - if (!view->delegatesScrolling()) > > Tim, could you check if this change may influence on mac and iOS ports ?
Horton, any comment about my question ?
Gyuyoung Kim
Comment 31
2014-04-08 22:47:33 PDT
(In reply to
comment #29
)
> (From update of
attachment 227269
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=227269&action=review
> > > Source/WebCore/page/Page.cpp:-702 > > - if (!view->delegatesScrolling()) > > Tim, could you check if this change may influence on mac and iOS ports ?
Horton, any comment about my question ?
Simon Fraser (smfr)
Comment 32
2014-04-22 23:25:09 PDT
Comment on
attachment 227269
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=227269&action=review
> Source/WebCore/page/Page.cpp:702 > + view->setScrollPosition(origin);
Did you look at the history of this code? I'm pretty sure we added the if (!view->delegatesScrolling()) test to fix iOS.
Gyuyoung Kim
Comment 33
2014-04-23 01:25:20 PDT
Comment on
attachment 227269
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=227269&action=review
>> Source/WebCore/page/Page.cpp:702 >> + view->setScrollPosition(origin); > > Did you look at the history of this code? I'm pretty sure we added the if (!view->delegatesScrolling()) test to fix iOS.
Simon, if so, let me try to find other way to fix this problem for EFL port. I'm going to update my opinion. Thanks.
Gyuyoung Kim
Comment 34
2014-04-25 03:19:10 PDT
Created
attachment 230159
[details]
Patch
Gyuyoung Kim
Comment 35
2014-04-25 03:28:09 PDT
Comment on
attachment 230159
[details]
Patch 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
Benjamin Poulain
Comment 36
2014-04-26 17:54:39 PDT
(In reply to
comment #35
)
> (From update of
attachment 230159
[details]
) > 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.
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.
Gyuyoung Kim
Comment 37
2014-04-27 18:12:04 PDT
Comment on
attachment 230159
[details]
Patch 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.
Gyuyoung Kim
Comment 38
2014-04-30 07:04:49 PDT
Created
attachment 230477
[details]
Patch
Gyuyoung Kim
Comment 39
2014-04-30 07:11:42 PDT
Comment on
attachment 230477
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=230477&action=review
> Source/WebCore/page/Page.cpp:755 > + view->hostWindow()->delegatedScrollRequested(origin);
>> I think we may need to split the paths between scrolling requested from the WebKit layers against scrolling requested from the WebCore layer.
Benjamin, I'm not sure if this is one of the paths you think though, can't we split the path like this ? I think we don't need to call the delegatedScrollRequested() in ScrollView::setScrollPosition(). If we call it here, we may make a path to split between scrolling requests from Webkit layer, isn't it ?
Benjamin Poulain
Comment 40
2014-05-07 15:55:53 PDT
Comment on
attachment 230477
[details]
Patch We should really get rid of TILED_BACKING_STORE. OS X and iOS also have tiled backing store, TILED_BACKING_STORE is mostly duplicated code at this point. A quick grep for TILED_BACKING_STORE shows it is actually duplicating a lot of features that have been upstreamed when iOS was upstreamed. In some cases, the code behind TILED_BACKING_STORE looks very suspicious. Now is not the right time though. I have serious doubt this is patch correct. For example, the call to Page::setPageScaleFactor() could come from WebPage::scalePage(), in which case calling back into scroll requested does not make any sense. I don't want to block you on this regression forever, the impact not any worse than the other TILED_BACKING_STORE stuff -> r+ for now.
Gyuyoung Kim
Comment 41
2014-05-07 18:10:31 PDT
(In reply to
comment #40
)
> (From update of
attachment 230477
[details]
) > We should really get rid of TILED_BACKING_STORE. > OS X and iOS also have tiled backing store, TILED_BACKING_STORE is mostly duplicated code at this point. > > A quick grep for TILED_BACKING_STORE shows it is actually duplicating a lot of features that have been upstreamed when iOS was upstreamed. In some cases, the code behind TILED_BACKING_STORE looks very suspicious. > Now is not the right time though.
I also think we need to do refactoring for the TILED_BACKING_STORE align with iOS or mac port's implementation. I'm going to talk with my co-worker how to refactor the tiled backing store.
> I have serious doubt this is patch correct. For example, the call to Page::setPageScaleFactor() could come from WebPage::scalePage(), in which case calling back into scroll requested does not make any sense. > I don't want to block you on this regression forever, the impact not any worse than the other TILED_BACKING_STORE stuff -> r+ for now.
Thank you for helping to my development. When we modify it, let me try to consider your concern together.
WebKit Commit Bot
Comment 42
2014-05-07 18:42:46 PDT
Comment on
attachment 230477
[details]
Patch Clearing flags on attachment: 230477 Committed
r168458
: <
http://trac.webkit.org/changeset/168458
>
WebKit Commit Bot
Comment 43
2014-05-07 18:42:57 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug