Bug 55021 - [EFL] Scroll doesn't work on tiled backing store
Summary: [EFL] Scroll doesn't work on tiled backing store
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 58802
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-22 21:03 PST by Eunsol Park
Modified: 2011-04-21 10:46 PDT (History)
14 users (show)

See Also:


Attachments
proposed patch (1.42 KB, patch)
2011-04-18 03:56 PDT, Eunsol Park
no flags Details | Formatted Diff | Diff
proposed patch (1.51 KB, patch)
2011-04-19 05:39 PDT, Eunsol Park
no flags Details | Formatted Diff | Diff
proposed patch (1.44 KB, patch)
2011-04-19 17:55 PDT, Eunsol Park
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eunsol Park 2011-02-22 21:03:08 PST
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.
Comment 1 Gyuyoung Kim 2011-03-10 21:29:47 PST
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.
Comment 2 Gyuyoung Kim 2011-04-13 20:38:09 PDT
Eunsol, when do you upload your patch ?
Comment 3 Eunsol Park 2011-04-18 03:56:08 PDT
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.
Comment 4 Gyuyoung Kim 2011-04-18 03:59:42 PDT
Demarchi, my colleague makes a patch for this bug. Please review this.
Comment 5 Lucas De Marchi 2011-04-18 06:48:57 PDT
(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
Comment 6 Eric Seidel (no email) 2011-04-18 09:09:43 PDT
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. :)
Comment 7 Lucas De Marchi 2011-04-18 10:12:52 PDT
Comment on attachment 90013 [details]
proposed patch

I prefer to wait for Antognolli reviewing this. I think it's plain wrong.
Comment 8 WebKit Commit Bot 2011-04-18 10:13:25 PDT
Comment on attachment 90013 [details]
proposed patch

Clearing flags on attachment: 90013

Committed r84155: <http://trac.webkit.org/changeset/84155>
Comment 9 WebKit Commit Bot 2011-04-18 10:13:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Eric Seidel (no email) 2011-04-18 10:19:11 PDT
My appologies.  Feel free to roll out.

sheriff-bot rollout 84155 REASON
 on IRC.
Comment 11 Rafael Antognolli 2011-04-18 10:26:47 PDT
(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.
Comment 12 WebKit Review Bot 2011-04-18 12:21:08 PDT
http://trac.webkit.org/changeset/84155 might have broken GTK Linux 32-bit Debug
Comment 13 Gyuyoung Kim 2011-04-18 16:19:27 PDT
(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
Comment 14 Eunsol Park 2011-04-19 05:39:51 PDT
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
Comment 15 Lucas De Marchi 2011-04-19 05:58:20 PDT
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?
Comment 16 Lucas De Marchi 2011-04-19 06:02:45 PDT
Reopen for the evaluation of a new patch.
Comment 17 Rafael Antognolli 2011-04-19 07:51:00 PDT
(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.
Comment 18 Eunsol Park 2011-04-19 17:31:31 PDT
OK, I will remove the comment and upload patch again.
Comment 19 Eunsol Park 2011-04-19 17:55:47 PDT
Created attachment 90284 [details]
proposed patch

The comment was removed in this patch.
Comment 20 Antonio Gomes 2011-04-20 09:42:26 PDT
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);
}
Comment 21 Rafael Antognolli 2011-04-20 09:56:26 PDT
(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.
Comment 22 Antonio Gomes 2011-04-20 10:15:43 PDT
(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 ... :(
Comment 23 Rafael Antognolli 2011-04-20 10:19:18 PDT
(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.
Comment 24 Eunsol Park 2011-04-20 22:46:51 PDT
(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.
Comment 25 WebKit Commit Bot 2011-04-21 08:55:06 PDT
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: alice.liu@apple.com)
The commit-queue is continuing to process your patch.
Comment 26 WebKit Commit Bot 2011-04-21 08:58:31 PDT
Comment on attachment 90284 [details]
proposed patch

Clearing flags on attachment: 90284

Committed r84505: <http://trac.webkit.org/changeset/84505>
Comment 27 WebKit Commit Bot 2011-04-21 08:58:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 WebKit Review Bot 2011-04-21 10:46:03 PDT
http://trac.webkit.org/changeset/84505 might have broken GTK Linux 32-bit Release