Bug 55021 – [EFL] Scroll doesn't work on tiled backing store

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Eunsol Park]

Scroll doesn't work on tiled backing store, but it works on single.
There is no Scroll bar when receiving mouse wheel event, because paintsEntireContents() set TRUE.
paintsEntireContents() is set on Tiled backing store in QT port and EFL port. 
I analyzed that it wouldn't be needed on EFL port, because tiled backing store process is different from QT.

Patch will be uploaded after more analysis.

[Gyuyoung Kim]

Now, EWebLauncher starts to use single backing store instead of tiled's because of some problems. When you test a patch for this bug, please reference to this.

[Gyuyoung Kim]

Eunsol, when do you upload your patch ?

[Eunsol Park]

Created attachment 90013 [details]
proposed patch

I uploaded patch, please review this.
If ewk_frame_paint_full_set() set True, view size return content size and scroll can't be made. 
Calling ewk_frame_paint_full_set() is removed.

[Gyuyoung Kim]

Demarchi, my colleague makes a patch for this bug. Please review this.

[Lucas De Marchi]

(In reply to comment #4)
> Demarchi, my colleague makes a patch for this bug. Please review this.

CC'ing Antognolli, who knows more about the tiled backing store

[Eric Seidel (no email)]

Comment on attachment 90013 [details]
proposed patch

How do we test this?  I'm not sure the EFL port if far enough along to even run the tests, so maybe it doesn't matter.  But all changes required layout tests if possible in WebKit. :)

[Lucas De Marchi]

Comment on attachment 90013 [details]
proposed patch

I prefer to wait for Antognolli reviewing this. I think it's plain wrong.

[WebKit Commit Bot]

Comment on attachment 90013 [details]
proposed patch

Clearing flags on attachment: 90013

Committed r84155: <http://trac.webkit.org/changeset/84155>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

My appologies.  Feel free to roll out.

sheriff-bot rollout 84155 REASON
 on IRC.

[Rafael Antognolli]

(In reply to comment #0)
> Scroll doesn't work on tiled backing store, but it works on single.
> There is no Scroll bar when receiving mouse wheel event, because paintsEntireContents() set TRUE.
> paintsEntireContents() is set on Tiled backing store in QT port and EFL port. 
> I analyzed that it wouldn't be needed on EFL port, because tiled backing store process is different from QT.
> 
> Patch will be uploaded after more analysis.

Eunsol,

I've made the first patch for adding the paintEntireContents flag, and I made it because the EFL port required it. The Qt port also started using it at the same time, because they were also implementing a tiled backing store (the one that is inside WebCore now).

This flag needs to be set if you want to paint anything outside of the screen, otherwise the paint area will be clipped to the viewport and no pre-rendering can be made. So just disabling it will probably break the tiled backing store anyway (I can't check it right now).

The view size returns the content size instead of viewport size when the paintEntireContents is set because of this change: http://trac.webkit.org/changeset/72242/trunk/WebCore/platform/ScrollView.cpp

I believe that the Qt port has a good reason for this change, but it just broke us. So we need to fix this on some other way.

[WebKit Review Bot]

http://trac.webkit.org/changeset/84155 might have broken GTK Linux 32-bit Debug

[Gyuyoung Kim]

(In reply to comment #6)
> (From update of attachment 90013 [details])
> How do we test this?  I'm not sure the EFL port if far enough along to even run the tests, so maybe it doesn't matter.  But all changes required layout tests if possible in WebKit. :)


Bug 44500 is a patch for EFL Layout Test. My colleagues will revise the patch as soon as they finish current work. :-)
 
Bug 44500 - [EFL] API for Dump Render Tree application

[Eunsol Park]

Created attachment 90184 [details]
proposed patch

Scrolling in QT tiled backing store works using delegatedScrollRequested() in webcore.
EFL can't use this function, because it depends on some other functions for QT tiled backing store.
As you know, EFL is implemented differently, so it's difficult to modify as similar to QT backing store. 
So, I want to change Veiw size in ScrollView.

I removed the change below and test, and I checked it worked fine.
http://trac.webkit.org/changeset/72242/trunk/WebCore/platform/ScrollView.cpp

So, this patch is the defence of the change in only EFL port.
akrekde

[Lucas De Marchi]

Comment on attachment 90184 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90184&action=review

> Source/WebCore/platform/ScrollView.cpp:240
> +// FIXME:EFL does not work scrolling with tiled backing store.
> +// https://bugs.webkit.org/show_bug.cgi?id=55021
> +#if !PLATFORM(EFL)
>      if (paintsEntireContents())
>          return IntRect(IntPoint(0, 0), contentsSize());
> +#endif

LGTM, but there's no need to keep this comment in code. Antognolli, what do you think?

[Lucas De Marchi]

Reopen for the evaluation of a new patch.

[Rafael Antognolli]

(In reply to comment #15)
> (From update of attachment 90184 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90184&action=review
> 
> > Source/WebCore/platform/ScrollView.cpp:240
> > +// FIXME:EFL does not work scrolling with tiled backing store.
> > +// https://bugs.webkit.org/show_bug.cgi?id=55021
> > +#if !PLATFORM(EFL)
> >      if (paintsEntireContents())
> >          return IntRect(IntPoint(0, 0), contentsSize());
> > +#endif
> 
> LGTM, but there's no need to keep this comment in code. Antognolli, what do you think?

LGTM too, and the comment could also be removed. I'm adding Andreas Kling, since he's the one who made the patch that changed this.

[Eunsol Park]

OK, I will remove the comment and upload patch again.

[Eunsol Park]

Created attachment 90284 [details]
proposed patch

The comment was removed in this patch.

[Antonio Gomes]

Comment on attachment 90284 [details]
proposed patch

Would not it be clearer if from FrameLoaderClientEFL::transitionToCommittedForNewPage (when frame view is created) you do:

...
m_frame->createView(blablabla);
...
m_frame->view()->setPaintsEntireContents(false);
}

[Rafael Antognolli]

(In reply to comment #20)
> (From update of attachment 90284 [details])
> Would not it be clearer if from FrameLoaderClientEFL::transitionToCommittedForNewPage (when frame view is created) you do:
> 
> ...
> m_frame->createView(blablabla);
> ...
> m_frame->view()->setPaintsEntireContents(false);
> }

But we need paintEntireContents set to True in order to make our tiled backing store work.

[Antonio Gomes]

(In reply to comment #21)
> (In reply to comment #20)
> > (From update of attachment 90284 [details] [details])
> > Would not it be clearer if from FrameLoaderClientEFL::transitionToCommittedForNewPage (when frame view is created) you do:
> > 
> > ...
> > m_frame->createView(blablabla);
> > ...
> > m_frame->view()->setPaintsEntireContents(false);
> > }
> 
> But we need paintEntireContents set to True in order to make our tiled backing store work.

so it needs to be true, but you still need it to clip painting to the viewport? I did not understood the problem, I think ... :(

[Rafael Antognolli]

(In reply to comment #22)
> (In reply to comment #21)
> > (In reply to comment #20)
> > > (From update of attachment 90284 [details] [details] [details])
> > > Would not it be clearer if from FrameLoaderClientEFL::transitionToCommittedForNewPage (when frame view is created) you do:
> > > 
> > > ...
> > > m_frame->createView(blablabla);
> > > ...
> > > m_frame->view()->setPaintsEntireContents(false);
> > > }
> > 
> > But we need paintEntireContents set to True in order to make our tiled backing store work.
> 
> so it needs to be true, but you still need it to clip painting to the viewport? I did not understood the problem, I think ... :(

hmm... sorry, maybe I wasn't clear. But we need it to be true because we *don't want* to clip painting to the viewport. We need to be able to paint things outside of the viewport too.

[Eunsol Park]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > (In reply to comment #20)
> > > > (From update of attachment 90284 [details] [details] [details] [details])
> > > > Would not it be clearer if from FrameLoaderClientEFL::transitionToCommittedForNewPage (when frame view is created) you do:
> > > > 
> > > > ...
> > > > m_frame->createView(blablabla);
> > > > ...
> > > > m_frame->view()->setPaintsEntireContents(false);
> > > > }
> > > 
> > > But we need paintEntireContents set to True in order to make our tiled backing store work.
> > 
> > so it needs to be true, but you still need it to clip painting to the viewport? I did not understood the problem, I think ... :(
> 
> hmm... sorry, maybe I wasn't clear. But we need it to be true because we *don't want* to clip painting to the viewport. We need to be able to paint things outside of the viewport too.

Dear,Antonio Gomes

1st patch I uploaded was modified like your opinion, 
because ewk_frame_paint_full_set() calls setPaintsEntireContents().
I removed ewk_frame_paint_full_set() in 1st patch.

But 1st one had error to paint out of the viewport, so I modified to 2nd patch.
I checked it solved in 2nd patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 90284 [details]:

http/tests/misc/favicon-loads-with-icon-loading-override.html bug 58412 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 90284 [details]
proposed patch

Clearing flags on attachment: 90284

Committed r84505: <http://trac.webkit.org/changeset/84505>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/84505 might have broken GTK Linux 32-bit Release