Summary: | [EFL] Remove overlapping recursive layout function | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eunsol Park <eunsol47.park> | ||||||
Component: | WebKit EFL | Assignee: | 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
Eunsol Park
2011-06-23 03:10:31 PDT
Created attachment 98333 [details]
proposed patch
upload patch
LGTM. Rafael, how do you think ? This patch would reduce the number of forceful layout from ewk and I agree that layout better to be called close to painting time. 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 on attachment 98333 [details]
proposed patch
Informal r+.
Comment on attachment 98333 [details]
proposed patch
Eunsol, could you say what problem the patch fixes?
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. It was just strange that a XXXIfNeeded was being executed needlessly. When paintsEntireContents() is TRUE, the layout is needed more frequently. 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 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... Created attachment 102612 [details]
proposed patch
Comment on attachment 102612 [details]
proposed patch
I modified the changelog.
You also need to mark the previous patch as obsolete. 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 on attachment 102612 [details] proposed patch Clearing flags on attachment: 102612 Committed r92190: <http://trac.webkit.org/changeset/92190> All reviewed patches have been landed. Closing bug. |