When fixed layout is enabled, scroll position is moved to 0,0 as soon as a hyperlink is clicked. Reproduce step on EFL MiniBrowser 1. WebKitBuild/Release/bin/MiniBrowser -L 1 http://www.nytimes.com 2. Scroll down to half of page 3. Click an any link
Created attachment 232117 [details] WIP
Created attachment 232173 [details] Patch
Noam, could you take a look this patch ?
Comment on attachment 232173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232173&action=review > Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.cpp:274 > + // function. However, in the final analysis, didChangeVisibleContents() should show white page before drawing next content. // However, in the final analysis, didChangeVisibleContents() should show white page before drawing next content. Hyowon, do you know if there is a function we can clear image data of tiled backing store ? It it is exist, we can use it to fix this problem instead of this workaround patch.
(In reply to comment #4) > (From update of attachment 232173 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232173&action=review > > > Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.cpp:274 > > + // function. However, in the final analysis, didChangeVisibleContents() should show white page before drawing next content. > > // However, in the final analysis, didChangeVisibleContents() should show white page before drawing next content. > > Hyowon, do you know if there is a function we can clear image data of tiled backing store ? It it is exist, we can use it to fix this problem instead of this workaround patch. I think that setting position(0, 0) timing is not appropriate. I'm worried that to clear tiles or layers may make another bug. Please give me some time to look into this.
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 232173 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=232173&action=review > > > > > Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.cpp:274 > > > + // function. However, in the final analysis, didChangeVisibleContents() should show white page before drawing next content. > > > > // However, in the final analysis, didChangeVisibleContents() should show white page before drawing next content. > > > > Hyowon, do you know if there is a function we can clear image data of tiled backing store ? It it is exist, we can use it to fix this problem instead of this workaround patch. > > I think that setting position(0, 0) timing is not appropriate. > I'm worried that to clear tiles or layers may make another bug. > Please give me some time to look into this. If there is no method, I would like to land this patch for now.
CC'ing Ossy. Ossy, could you take a look this patch ? PageViewportController is only being used by EFL port.
(In reply to comment #7) > CC'ing Ossy. Ossy, could you take a look this patch ? PageViewportController is only being used by EFL port. Sorry, but I don't have expertise for Coordinated Graphics System, maybe Zoltán can review this patch.
(In reply to comment #8) > (In reply to comment #7) > > CC'ing Ossy. Ossy, could you take a look this patch ? PageViewportController is only being used by EFL port. > > Sorry, but I don't have expertise for Coordinated Graphics System, > maybe Zoltán can review this patch. CC'ing Zoltan. Though this is a workaround patch, this problem always happens on EFL WK2 browser with when enabling fixed layout. We need it.
Zoltan, could you take a look this patch ? Though this patch is a workaround patch, currently I don't have correct solution. So, I would like to land this patch until find correct way. CoordinatedGraphics is only used by EFL port.
CC'ing yoon, could you take a look this patch ?
Comment on attachment 232173 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232173&action=review >>>> Source/WebKit2/UIProcess/CoordinatedGraphics/PageViewportController.cpp:274 >>>> + // function. However, in the final analysis, didChangeVisibleContents() should show white page before drawing next content. >>> >>> // However, in the final analysis, didChangeVisibleContents() should show white page before drawing next content. >>> >>> Hyowon, do you know if there is a function we can clear image data of tiled backing store ? It it is exist, we can use it to fix this problem instead of this workaround patch. >> >> I think that setting position(0, 0) timing is not appropriate. >> I'm worried that to clear tiles or layers may make another bug. >> Please give me some time to look into this. > > If there is no method, I would like to land this patch for now. In my opinion, I think that we should stop to update display until we get first didFirstVisuallyNonEmptyLayoutForFrame. What do you think about it?
To remove this behavior, it seems enough to remove a line calling "scheduleUpdateDisplay" from: void ViewClientEfl::didCompletePageTransition(WKViewRef, const void* clientInfo) { EwkView* ewkView = toEwkView(clientInfo); if (WKPageUseFixedLayout(ewkView->wkPage())) { ewkView->pageViewportController().pageTransitionViewportReady(); return; } ewkView->scheduleUpdateDisplay(); } and block "scheduleUpdateDisplay()" from : void PageViewportControllerClientEfl::didChangeVisibleContents() { m_view->scheduleUpdateDisplay(); } until next ViewClientEfl::didRenderFrame call.
(In reply to comment #13) > To remove this behavior, it seems enough to remove a line calling "scheduleUpdateDisplay" from: > > and block "scheduleUpdateDisplay()" from : > > void PageViewportControllerClientEfl::didChangeVisibleContents() > { > m_view->scheduleUpdateDisplay(); > } > > until next ViewClientEfl::didRenderFrame call. Thank you for your comment. But, if we need to block to call scheduleUpdateDisplay() in PageViewportControllerClientEfl::didChangeVisibleContents() until next call of ViewClientEfl::didRenderFrame(), we may need to add a new flag to some functions of PageViewportController. I don't prefer to add a flag to do this. Do you have any suggestion to block scheduleUpdateDisplay() until next didRenderFrame() call ?
(In reply to comment #14) > (In reply to comment #13) > > To remove this behavior, it seems enough to remove a line calling "scheduleUpdateDisplay" from: > > > > > and block "scheduleUpdateDisplay()" from : > > > > void PageViewportControllerClientEfl::didChangeVisibleContents() > > { > > m_view->scheduleUpdateDisplay(); > > } > > > > until next ViewClientEfl::didRenderFrame call. > > Thank you for your comment. But, if we need to block to call scheduleUpdateDisplay() in PageViewportControllerClientEfl::didChangeVisibleContents() until next call of ViewClientEfl::didRenderFrame(), we may need to add a new flag to some functions of PageViewportController. I don't prefer to add a flag to do this. Do you have any suggestion to block scheduleUpdateDisplay() until next didRenderFrame() call ? Well, It is hard to decide which class should manage the flag, because PageViewController and its related classes lie scattered. :) I think it is about the polish related with the behavior during page transition, so EwkView could have the status flag (ex isWaitingForNewPage). By doing that, ViewClientEfl could set this flag and PageViewportControllerClientEfl could read / reset this flag.
Created attachment 233411 [details] Patch only for EFL port
(In reply to comment #15) > Well, It is hard to decide which class should manage the flag, because PageViewController and its related classes lie scattered. :) > > I think it is about the polish related with the behavior during page transition, > so EwkView could have the status flag (ex isWaitingForNewPage). > By doing that, ViewClientEfl could set this flag and PageViewportControllerClientEfl could read / reset this flag. I upload a patch according to your suggestion. However I don't prefer to fix this issue like this patch. I think current problem is that PageViewportController orders to draw current page for a while before next page is shown. I think PageViewportController::syncVisibleContents() should not call didChangeVisibleContents() until starting to draw next page. Or, we may draw white page instead of drawing current page as soon as user press a hyperlink. Let me consider what is best for this issue further.
Comment on attachment 233411 [details] Patch only for EFL port It looks there is no function to clear backing store that I can call in PageLoadClientEfl::didCommitLoadForFrame(). So, I need to land this patch for EFL port for now.
Comment on attachment 233411 [details] Patch only for EFL port View in context: https://bugs.webkit.org/attachment.cgi?id=233411&action=review I agree that it is the problem that PageViewportController resets its position too early, but I think we need to refactor PVC and related classes first. If you can prevent the problem by this way, I think it is a good way to go for now. I'm not sure whether I can comment about this patch from now on (Because it is now EFL patch), But I found some nitpicks.. > Source/WebKit2/ChangeLog:8 > + When new page starts to be loaded, didCommitLoad() calls this function with (0, 0) position via applyPositionAfterRenderingContents() I think we need to clarify what is "this". > Source/WebKit2/UIProcess/API/efl/EwkView.h:187 > + void setWaitingForNewPage(bool wait) { m_isWaitingForNewPage = wait; } I think it makes more readable to use separate methods to set a false flag. ex) setWaitingForNewPage() { m_isWaitingForNewPage = true; } didCommitNewPage() { m_isWaitingForNewPage = false; } > Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:65 > } How about to use early return to improve readability? ex) { if (m_view->waitingForNewPage()) return; m_view->scheduleUpdateDisplay(); }
Created attachment 233421 [details] Patch
Created attachment 233422 [details] Patch
Comment on attachment 233411 [details] Patch only for EFL port View in context: https://bugs.webkit.org/attachment.cgi?id=233411&action=review Gwang Yoon Hwang, Nice comments ! I agree with your suggestion. >> Source/WebKit2/ChangeLog:8 >> + When new page starts to be loaded, didCommitLoad() calls this function with (0, 0) position via applyPositionAfterRenderingContents() > > I think we need to clarify what is "this". I needed to revise description. Next patch updates description according to new patch. >> Source/WebKit2/UIProcess/API/efl/EwkView.h:187 >> + void setWaitingForNewPage(bool wait) { m_isWaitingForNewPage = wait; } > > I think it makes more readable to use separate methods to set a false flag. > ex) > setWaitingForNewPage() { m_isWaitingForNewPage = true; } > didCommitNewPage() { m_isWaitingForNewPage = false; } Ok, this seems better name :) >> Source/WebKit2/UIProcess/efl/PageViewportControllerClientEfl.cpp:65 >> } > > How about to use early return to improve readability? > > ex) > { > if (m_view->waitingForNewPage()) > return; > > m_view->scheduleUpdateDisplay(); > } Yes, this is better idea.
Comment on attachment 233422 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=233422&action=review I'm okay to this patch. Let's think about how to refactor PVC things in a different bug or thread. > Source/WebKit2/ChangeLog:9 > + with initial position via applyPositionAfterRenderingContents() defore starting to render Small typo, /defore/before I think.
Comment on attachment 233422 [details] Patch Attachment 233422 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5433569014972416 New failing tests: media/W3C/video/networkState/networkState_during_loadstart.html
Created attachment 233436 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 233495 [details] Patch
Ossy, could you review this patch ? This patch gets informal review.
Comment on attachment 233495 [details] Patch rs=me
Comment on attachment 233495 [details] Patch Clearing flags on attachment: 233495 Committed r170243: <http://trac.webkit.org/changeset/170243>
All reviewed patches have been landed. Closing bug.