Bug 133300 – [CoordinatedGraphics][WK2][EFL] Page is moved to (0,0) position before rendering content

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Gyuyoung Kim]

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

[Gyuyoung Kim]

Created attachment 232117 [details]
WIP

[Gyuyoung Kim]

Created attachment 232173 [details]
Patch

[Gyuyoung Kim]

Noam, could you take a look this patch ?

[Gyuyoung Kim]

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.

[Hyowon Kim]

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

[Gyuyoung Kim]

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

[Gyuyoung Kim]

CC'ing Ossy. Ossy, could you take a look this patch ? PageViewportController is only being used by EFL port.

[Csaba Osztrogonác]

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

[Gyuyoung Kim]

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

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

CC'ing yoon, could you take a look this patch ?

[Ryuan Choi]

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?

[Gwang Yoon Hwang]

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.

[Gyuyoung Kim]

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

[Gwang Yoon Hwang]

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

[Gyuyoung Kim]

Created attachment 233411 [details]
Patch only for EFL port

[Gyuyoung Kim]

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

[Gyuyoung Kim]

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.

[Gwang Yoon Hwang]

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();
}

[Gyuyoung Kim]

Created attachment 233421 [details]
Patch

[Gyuyoung Kim]

Created attachment 233422 [details]
Patch

[Gyuyoung Kim]

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.

[Gwang Yoon Hwang]

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.

[Build Bot]

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

[Build Bot]

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

[Gyuyoung Kim]

Created attachment 233495 [details]
Patch

[Gyuyoung Kim]

Ossy, could you review this patch ? This patch gets informal review.

[Csaba Osztrogonác]

Comment on attachment 233495 [details]
Patch

rs=me

[WebKit Commit Bot]

Comment on attachment 233495 [details]
Patch

Clearing flags on attachment: 233495

Committed r170243: <http://trac.webkit.org/changeset/170243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.