Bug 63241

Summary: [EFL] Remove overlapping recursive layout function
Product: WebKit Reporter: Eunsol Park <eunsol47.park>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: antognolli+webkit, eunsol47.park, gyuyoung.kim, gyuyoung.kim, hyuki.kim, kenneth, leandro, lucas.de.marchi, rakuco, ryuan.choi, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
proposed patch
none
proposed patch none

Description Eunsol Park 2011-06-23 03:10:31 PDT
In tiled backing store status, the recursive layout function was called twice in _ewk_view_smart_calculate and _ewk_view_tiled_updates_process_pre.
It made unnecessary layouting repeatedly, so I think the call in _ewk_view_smart_calculate have to be removed. 
But in single backing store, the call have to be added before repainting.
So, the call will be added in _ewk_view_single_smart_repaints_process.
Comment 1 Eunsol Park 2011-06-23 03:34:45 PDT
Created attachment 98333 [details]
proposed patch

upload patch
Comment 2 Gyuyoung Kim 2011-06-23 03:41:59 PDT
LGTM. Rafael, how do you think ?
Comment 3 KwangHyuk 2011-06-23 05:17:14 PDT
This patch would reduce the number of forceful layout from ewk and I agree that layout better to be called close to painting time.
Comment 4 Ryuan Choi 2011-06-27 02:53:55 PDT
Now, FrameView::layout is called every _ewk_view_smart_calculate called.
but I believe that it should be called when it really required.

Especially, it will be called frequently although we suspend rendering for faster scrolling.
So, I think that this patch is way to go.
Comment 5 Leandro Pereira 2011-06-27 10:41:30 PDT
Comment on attachment 98333 [details]
proposed patch

Informal r+.
Comment 6 Antonio Gomes 2011-06-27 15:51:30 PDT
Comment on attachment 98333 [details]
proposed patch

Eunsol, could you say what problem the patch fixes?
Comment 7 Eunsol Park 2011-06-27 17:57:18 PDT
As I commented, the function was called twice without the need.
It can make scrolling slow.  And it also can make other operations slow especially in heavy page. It is critical problem to use.
Comment 8 Antonio Gomes 2011-06-27 18:02:54 PDT
It was just strange that a XXXIfNeeded was being executed needlessly.
Comment 9 Eunsol Park 2011-06-27 18:28:05 PDT
When paintsEntireContents() is TRUE, the layout is needed  more frequently.
Comment 10 Eunsol Park 2011-07-18 03:49:57 PDT
Hello,
I explained more specifically. 

First, The recursive layout is called twice in _ewk_view_smart_calculate and _ewk_view_tiled_updates_process_pre, so it has no problem removing one of them.

Second, 
while panning, it is in the rendering suspend status to be fast in EFL port. 
In the status, it doesn't want to be repainted newly.
_ewk_view_smart_calculate  can be called from "webcore- invalidate" when changing the page despite the suspend status, because it is a ewk's status, webcore doesn't know it.
(Especially, _ewk_view_smart_calculate is called more frequently when paintsEntireContents() is TRUE.)

So, I think the recursive layout is unnecessary in _ewk_view_smart_calculate.
Thank you.
Comment 11 Antonio Gomes 2011-07-31 22:13:46 PDT
Comment on attachment 98333 [details]
proposed patch

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

Eunsol, could you say what problem the patch fixes?

> Source/WebKit/efl/ChangeLog:9
> +        ewk_view_layout_if_needed_recursive was called twice in tiled backing store, 
> +        so it was removed. And in single backing store, it was needed and added.

It does not describe what is the problem or what it fixes in practice.

Could you be more wordy? There are some unanswered questions as well...
Comment 12 Eunsol Park 2011-08-01 19:09:41 PDT
Created attachment 102612 [details]
proposed patch
Comment 13 Eunsol Park 2011-08-01 19:10:53 PDT
Comment on attachment 102612 [details]
proposed patch

I modified the changelog.
Comment 14 Raphael Kubo da Costa (:rakuco) 2011-08-02 05:33:54 PDT
You also need to mark the previous patch as obsolete.
Comment 15 Antonio Gomes 2011-08-02 07:59:39 PDT
Comment on attachment 102612 [details]
proposed patch

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

> Source/WebKit/efl/ChangeLog:7
> +        and _ewk_view_tiled_updates_process_pre,so it is no problem removing one of them.

s/,so/, so/

> Source/WebKit/efl/ChangeLog:8
> +        Second, In tiled backingstore configuration, _ewk_view_layout_if_needed_recursive called 

s/In/in
Comment 16 WebKit Review Bot 2011-08-02 08:18:44 PDT
Comment on attachment 102612 [details]
proposed patch

Clearing flags on attachment: 102612

Committed r92190: <http://trac.webkit.org/changeset/92190>
Comment 17 WebKit Review Bot 2011-08-02 08:18:50 PDT
All reviewed patches have been landed.  Closing bug.