Bug 55021

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.

Eunsol, when do you upload your patch ?

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.

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

(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 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 on attachment 90013 [details] proposed patch I prefer to wait for Antognolli reviewing this. I think it's plain wrong.

Comment on attachment 90013 [details] proposed patch Clearing flags on attachment: 90013 Committed r84155: <http://trac.webkit.org/changeset/84155>

All reviewed patches have been landed. Closing bug.

My appologies. Feel free to roll out. sheriff-bot rollout 84155 REASON on IRC.

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

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

(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

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

Reopen for the evaluation of a new patch.

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

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

Created attachment 90284 [details] proposed patch The comment was removed in this patch.

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

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

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

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

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

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 on attachment 90284 [details] proposed patch Clearing flags on attachment: 90284 Committed r84505: <http://trac.webkit.org/changeset/84505>

All reviewed patches have been landed. Closing bug.

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