Bug 63241 – [EFL] Remove overlapping recursive layout function

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

[Eunsol Park]

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.

[Eunsol Park]

Created attachment 98333 [details]
proposed patch

upload patch

[Gyuyoung Kim]

LGTM. Rafael, how do you think ?

[KwangHyuk]

This patch would reduce the number of forceful layout from ewk and I agree that layout better to be called close to painting time.

[Ryuan Choi]

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.

[Leandro Pereira]

Comment on attachment 98333 [details]
proposed patch

Informal r+.

[Antonio Gomes]

Comment on attachment 98333 [details]
proposed patch

Eunsol, could you say what problem the patch fixes?

[Eunsol Park]

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.

[Antonio Gomes]

It was just strange that a XXXIfNeeded was being executed needlessly.

[Eunsol Park]

When paintsEntireContents() is TRUE, the layout is needed  more frequently.

[Eunsol Park]

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.

[Antonio Gomes]

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

[Eunsol Park]

Created attachment 102612 [details]
proposed patch

[Eunsol Park]

Comment on attachment 102612 [details]
proposed patch

I modified the changelog.

[Raphael Kubo da Costa (:rakuco)]

You also need to mark the previous patch as obsolete.

[Antonio Gomes]

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

[WebKit Review Bot]

Comment on attachment 102612 [details]
proposed patch

Clearing flags on attachment: 102612

Committed r92190: <http://trac.webkit.org/changeset/92190>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.