Bug 133300

Summary: [CoordinatedGraphics][WK2][EFL] Page is moved to (0,0) position before rendering content
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: WebKit EFLAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, bunhere, cdumez, cmarcelo, commit-queue, gyuyoung.kim, hh.kaka, hw1008.kim, hyuki.kim, lucas.de.marchi, luiz, noam, ossy, rniwa, sergio, yoon, zeno, zherczeg
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 136568, 138305    
Attachments:
Description Flags
WIP
none
Patch
none
Patch only for EFL port
none
Patch
none
Patch
none
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
none
Patch none

Description Gyuyoung Kim 2014-05-27 01:34:34 PDT
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
Comment 1 Gyuyoung Kim 2014-05-27 01:35:55 PDT
Created attachment 232117 [details]
WIP
Comment 2 Gyuyoung Kim 2014-05-27 23:46:25 PDT
Created attachment 232173 [details]
Patch
Comment 3 Gyuyoung Kim 2014-05-28 02:07:41 PDT
Noam, could you take a look this patch ?
Comment 4 Gyuyoung Kim 2014-05-28 22:51:38 PDT
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.
Comment 5 Hyowon Kim 2014-05-29 00:31:52 PDT
(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.
Comment 6 Gyuyoung Kim 2014-05-29 02:45:27 PDT
(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.
Comment 7 Gyuyoung Kim 2014-06-02 00:22:43 PDT
CC'ing Ossy. Ossy, could you take a look this patch ? PageViewportController is only being used by EFL port.
Comment 8 Csaba Osztrogonác 2014-06-03 03:47:15 PDT
(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.
Comment 9 Gyuyoung Kim 2014-06-03 21:34:03 PDT
(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.
Comment 10 Gyuyoung Kim 2014-06-09 21:06:06 PDT
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.
Comment 11 Gyuyoung Kim 2014-06-17 23:00:38 PDT
CC'ing yoon, could you take a look this patch ?
Comment 12 Ryuan Choi 2014-06-18 01:00:33 PDT
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?
Comment 13 Gwang Yoon Hwang 2014-06-18 01:24:59 PDT
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.
Comment 14 Gyuyoung Kim 2014-06-19 00:48:18 PDT
(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 ?
Comment 15 Gwang Yoon Hwang 2014-06-19 01:21:29 PDT
(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.
Comment 16 Gyuyoung Kim 2014-06-20 00:24:45 PDT
Created attachment 233411 [details]
Patch only for EFL port
Comment 17 Gyuyoung Kim 2014-06-20 00:34:47 PDT
(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 18 Gyuyoung Kim 2014-06-20 01:40:03 PDT
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 19 Gwang Yoon Hwang 2014-06-20 02:23:41 PDT
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();
}
Comment 20 Gyuyoung Kim 2014-06-20 03:21:42 PDT
Created attachment 233421 [details]
Patch
Comment 21 Gyuyoung Kim 2014-06-20 03:22:23 PDT
Created attachment 233422 [details]
Patch
Comment 22 Gyuyoung Kim 2014-06-20 03:25:26 PDT
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 23 Gwang Yoon Hwang 2014-06-20 04:18:14 PDT
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 24 Build Bot 2014-06-20 11:34:39 PDT
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
Comment 25 Build Bot 2014-06-20 11:34:47 PDT
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
Comment 26 Gyuyoung Kim 2014-06-20 18:47:04 PDT
Created attachment 233495 [details]
Patch
Comment 27 Gyuyoung Kim 2014-06-20 18:49:41 PDT
Ossy, could you review this patch ? This patch gets informal review.
Comment 28 Csaba Osztrogonác 2014-06-21 01:38:08 PDT
Comment on attachment 233495 [details]
Patch

rs=me
Comment 29 WebKit Commit Bot 2014-06-21 08:06:30 PDT
Comment on attachment 233495 [details]
Patch

Clearing flags on attachment: 233495

Committed r170243: <http://trac.webkit.org/changeset/170243>
Comment 30 WebKit Commit Bot 2014-06-21 08:06:43 PDT
All reviewed patches have been landed.  Closing bug.